Bug 238607 - [iOS] [WK2] Add plumbing for extracting video frames in element fullscreen
Summary: [iOS] [WK2] Add plumbing for extracting video frames in element fullscreen
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 238579
Blocks:
  Show dependency treegraph
 
Reported: 2022-03-31 07:48 PDT by Wenson Hsieh
Modified: 2022-04-01 19:11 PDT (History)
15 users (show)

See Also:


Attachments
Depends on #238579 (15.78 KB, patch)
2022-03-31 08:53 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (15.61 KB, patch)
2022-04-01 08:56 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix non-GPUP build (15.71 KB, patch)
2022-04-01 09:50 PDT, Wenson Hsieh
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (15.70 KB, patch)
2022-04-01 15:57 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].