Bug 216534

Created attachment 408812 [details]
Patch

Comment on attachment 408812 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProviderCocoa.cpp:53
> +LibWebRTCProviderCocoa::LibWebRTCProviderCocoa()
> +{
> +    setH264HardwareEncoderAllowed(RuntimeEnabledFeatures::sharedFeatures().webRTCH264HardwareEncoderEnabled());
> +}
> +

This is a layering violation. Code in platform/ should not know about/call code in page/.  While people have recently not been observing this rule, platform code is meant to wrap a platform concept, so should not be directly modifiable by WebKit level preferences like this.

Instead, the model that works is have the non-platform part of the code that uses this set this setting.

OK, will think about it more.

Created attachment 409023 [details]
Patch

Created attachment 409028 [details]
Patch

While this is no longer a layering violation, this still maintains the issue of not being really working with the page-based model of WebKit preferences. 

Is the H264 hardware encoder something that has to be enabled/disable on a process wide basis? Could we always enable it but not filter our uses when disabled for a page?

(In reply to Sam Weinig from comment #6)
> While this is no longer a layering violation, this still maintains the issue
> of not being really working with the page-based model of WebKit preferences. 
> 
> Is the H264 hardware encoder something that has to be enabled/disable on a
> process wide basis? Could we always enable it but not filter our uses when
> disabled for a page?

We could change the implementation to do like we do for VP9 and HEVC (have an encoder parameter). Slightly more complex but doable.
Given this is for testing only though, the process wide parameter seems good enough.

Committed r267249: <https://trac.webkit.org/changeset/267249>

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

<rdar://problem/69160129>

(In reply to youenn fablet from comment #7)
> (In reply to Sam Weinig from comment #6)
> > While this is no longer a layering violation, this still maintains the issue
> > of not being really working with the page-based model of WebKit preferences. 
> > 
> > Is the H264 hardware encoder something that has to be enabled/disable on a
> > process wide basis? Could we always enable it but not filter our uses when
> > disabled for a page?
> 
> We could change the implementation to do like we do for VP9 and HEVC (have
> an encoder parameter). Slightly more complex but doable.
> Given this is for testing only though, the process wide parameter seems good
> enough.

This is very discouraging. I have been working to remove process wide state like this, and adding more against my objection only adds to my work.

> This is very discouraging. I have been working to remove process wide state
> like this, and adding more against my objection only adds to my work.

I understand the desire for purity.
I do not really see the impact of how this is slowing down your work.
Strictly speaking, this patch is not introducing any new process wide state, the process wide state was there before this patch.

I can remove the pre-existing process wide state as a follow-up but would like to understand the benefit in this particular case.

FYI, the purpose of the flag is to help a team reproing an ongoing issue more easily.