Bug 238607 – [iOS] [WK2] Add plumbing for extracting video frames in element fullscreen

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

.

[Radar WebKit Bug Importer]

<rdar://problem/91102888>

[Wenson Hsieh]

Created attachment 456242 [details]
Depends on #238579

[Wenson Hsieh]

Created attachment 456364 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 456368 [details]
Fix non-GPUP build

[Eric Carlson]

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!

[Wenson Hsieh]

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!

👍🏻

[Chris Dumez]

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() }`

[Wenson Hsieh]

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?)

[Chris Dumez]

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 :)

[Wenson Hsieh]

Created attachment 456413 [details]
Patch for landing

[Wenson Hsieh]

(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>

[EWS]

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].