Bug 238607

Summary: [iOS] [WK2] Add plumbing for extracting video frames in element fullscreen
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, megan_gardner, philipj, sergio, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 238579    
Bug Blocks:    
Attachments:
Description Flags
Depends on #238579
none
Rebase on trunk
ews-feeder: commit-queue-
Fix non-GPUP build
eric.carlson: review+
Patch for landing none

Description Wenson Hsieh 2022-03-31 07:48:15 PDT 
.
Comment 1 Radar WebKit Bug Importer 2022-03-31 07:49:09 PDT 
<rdar://problem/91102888>
Comment 2 Wenson Hsieh 2022-03-31 08:53:24 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2022-04-01 08:56:32 PDT Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2022-04-01 09:50:22 PDT 
Created attachment 456368 [details]
Fix non-GPUP build
Comment 5 Eric Carlson 2022-04-01 11:18:29 PDT 
Comment on attachment 456368 [details]
Fix non-GPUP build

View in context: https://bugs.webkit.org/attachment.cgi?id=456368&action=review

r=me once the bots are happy

> Source/WebCore/html/HTMLMediaElement.cpp:626
> +    return m_player ? std::make_optional(m_player->identifier()) : std::nullopt;

Thank you!
Comment 6 Wenson Hsieh 2022-04-01 15:46:23 PDT 
Comment on attachment 456368 [details]
Fix non-GPUP build

View in context: https://bugs.webkit.org/attachment.cgi?id=456368&action=review

Thank you for the review!

Hm…it looks like EWS is still lagging behind :(

In the interim, I ran a subset of media and fullscreen layout tests locally on iOS sim and macOS, and verified that this doesn't cause any new tests to fail. I did find some existing, unrelated debug assertions, which I'll fix investigate in a followup bug.

>> Source/WebCore/html/HTMLMediaElement.cpp:626
>> +    return m_player ? std::make_optional(m_player->identifier()) : std::nullopt;
> 
> Thank you!

👍🏻
Comment 7 Chris Dumez 2022-04-01 15:49:37 PDT 
Comment on attachment 456368 [details]
Fix non-GPUP build

View in context: https://bugs.webkit.org/attachment.cgi?id=456368&action=review

>>> Source/WebCore/html/HTMLMediaElement.cpp:626
>>> +    return m_player ? std::make_optional(m_player->identifier()) : std::nullopt;
>> 
>> Thank you!
> 
> 👍🏻

Note that with C++17, you should be able to just `std::optional { m_player->identifier() }`
Comment 8 Wenson Hsieh 2022-04-01 15:50:57 PDT 
Comment on attachment 456368 [details]
Fix non-GPUP build

View in context: https://bugs.webkit.org/attachment.cgi?id=456368&action=review

>>>> Source/WebCore/html/HTMLMediaElement.cpp:626
>>>> +    return m_player ? std::make_optional(m_player->identifier()) : std::nullopt;
>>> 
>>> Thank you!
>> 
>> 👍🏻
> 
> Note that with C++17, you should be able to just `std::optional { m_player->identifier() }`

Sounds good — I'll go with that!

(I wasn't sure whether we prefer std::optional { } or std::make_optional() — it sounds like the former is preferred?)
Comment 9 Chris Dumez 2022-04-01 15:52:33 PDT 
Comment on attachment 456368 [details]
Fix non-GPUP build

View in context: https://bugs.webkit.org/attachment.cgi?id=456368&action=review

>>>>> Source/WebCore/html/HTMLMediaElement.cpp:626
>>>>> +    return m_player ? std::make_optional(m_player->identifier()) : std::nullopt;
>>>> 
>>>> Thank you!
>>> 
>>> 👍🏻
>> 
>> Note that with C++17, you should be able to just `std::optional { m_player->identifier() }`
> 
> Sounds good — I'll go with that!
> 
> (I wasn't sure whether we prefer std::optional { } or std::make_optional() — it sounds like the former is preferred?)

It's more concise so I think std::optional { } is better. Darin made a similar comment on one of my patches recently so I am passing it along :)
Comment 10 Wenson Hsieh 2022-04-01 15:57:57 PDT 
Created attachment 456413 [details]
Patch for landing
Comment 11 Wenson Hsieh 2022-04-01 17:39:09 PDT 
(In reply to Wenson Hsieh from comment #6)
> Hm…it looks like EWS is still lagging behind :(
> 
> In the interim, I ran a subset of media and fullscreen layout tests locally
> on iOS sim and macOS, and verified that this doesn't cause any new tests to
> fail. I did find some existing, unrelated debug assertions, which I'll
> investigate in a followup bug.


Tracking these existing failures in: <https://webkit.org/b/238687>
Comment 12 EWS 2022-04-01 19:11:07 PDT 
Committed r292252 (249150@main): <https://commits.webkit.org/249150@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456413 [details].