Bug 232818 - Make it possible to toggle several experimental media features when GPU Process is enabled
Summary: Make it possible to toggle several experimental media features when GPU Proce...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-08 09:35 PST by Wenson Hsieh
Modified: 2021-11-08 18:12 PST (History)
5 users (show)

See Also:


Attachments
For EWS (10.62 KB, patch)
2021-11-08 10:50 PST, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch (11.34 KB, patch)
2021-11-08 15:31 PST, 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 2021-11-08 09:35:46 PST
In particular, these features:

• OpusDecoderEnabled (Opus audio decoder)
• VorbisDecoderEnabled (Vorbis audio decoder)
• WebMFormatReaderEnabled (WebM format reader)
• WebMParserEnabled (WebM MSE parser)
• MediaSourceInlinePaintingEnabled (Experimental MediaSource Inline Painting)
Comment 1 Wenson Hsieh 2021-11-08 10:50:21 PST
Created attachment 443572 [details]
For EWS
Comment 2 Tim Horton 2021-11-08 15:17:17 PST
Comment on attachment 443572 [details]
For EWS

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

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:590
> +    for (auto page : webProcess.pages()) {

While this is still a wild layering problem, it's strictly better than using the WebPageGroup prefs.

I think you removed a bit too much of the "omg here be dragons" comment though; the caveats in your changelog should be mentioned here too.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:-670
> -    setScreenProperties(WebCore::collectScreenProperties());

as you pointed out yourself, you lost canSendMessage here.
Comment 3 Wenson Hsieh 2021-11-08 15:28:14 PST
Comment on attachment 443572 [details]
For EWS

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

Thanks for the review!

>> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:590
>> +    for (auto page : webProcess.pages()) {
> 
> While this is still a wild layering problem, it's strictly better than using the WebPageGroup prefs.
> 
> I think you removed a bit too much of the "omg here be dragons" comment though; the caveats in your changelog should be mentioned here too.

Yes — definitely agreed! I'll leave this comment behind:

// FIXME: It's not ideal that these features are controlled by preferences-level feature flags (i.e. per-web view), but there is only one GPU process and the runtime-enabled features backing these preferences are process-wide. We should refactor each of these features so that they aren't process-global, and then reimplement this feature flag propagation to the GPU Process in a way that respects the settings of the page that is hosting each media element.
// For the time being, each of the below features are enabled in the GPU Process if it is enabled by at least one web page's preferences. In practice, all web pages' preferences should agree on these feature flag values.

>> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:-670
>> -    setScreenProperties(WebCore::collectScreenProperties());
> 
> as you pointed out yourself, you lost canSendMessage here.

Fixed!
Comment 4 Wenson Hsieh 2021-11-08 15:31:13 PST
Created attachment 443619 [details]
Patch
Comment 5 EWS 2021-11-08 18:11:21 PST
Committed r285477 (244001@main): <https://commits.webkit.org/244001@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443619 [details].
Comment 6 Radar WebKit Bug Importer 2021-11-08 18:12:21 PST
<rdar://problem/85182754>