Bug 224469 – [GPUP] WebContent process should not create AVOutputContext instances when media in GPU Process is enabled

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

We enabled the access to CoreMedia routing service in order to fix bug 224328. Ideally, the access to the CoreMedia routing service should be disabled.

[Peng Liu]

<rdar://76403302>

[Peng Liu]

Created attachment 425902 [details]
WIP patch

[Peng Liu]

Created attachment 425904 [details]
WIP patch

[Peng Liu]

Created attachment 425943 [details]
Fix layout test failures

[Peng Liu]

Created attachment 425950 [details]
Rebase the patch

[Peng Liu]

Created attachment 425955 [details]
Fix issues found in manual tests

[Eric Carlson]

Comment on attachment 425955 [details]
Fix issues found in manual tests

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

> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.h:63
> +        , m_hasActiveRoute(hasActiveRoute)
> +        , m_supportsRemoteVideoPlayback(supportsRemoteVideoPlayback)

There is no guarantee that this state won't change while the output context is serialized, so we shouldn't cache it. See my comment below about how to handle it.

> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.mm:79
> +    if (![m_outputContext supportsMultipleOutputDevices])
> +        m_deviceName = [m_outputContext deviceName];
> +    else {
> +        auto outputDeviceNames = adoptNS([[NSMutableArray alloc] init]);
> +        for (AVOutputDevice *outputDevice in [m_outputContext outputDevices])
> +            [outputDeviceNames addObject:[outputDevice deviceName]];
> +
> +        m_deviceName = [outputDeviceNames componentsJoinedByString:@" + "];
> +    }
> +
> +    if ([m_outputContext respondsToSelector:@selector(supportsMultipleOutputDevices)] && [m_outputContext supportsMultipleOutputDevices] && [m_outputContext respondsToSelector:@selector(outputDevices)]) {
> +        for (AVOutputDevice *outputDevice in [m_outputContext outputDevices]) {
> +            if (outputDevice.deviceFeatures & (AVOutputDeviceFeatureVideo | AVOutputDeviceFeatureAudio))
> +                m_hasActiveRoute = true;
> +        }
> +    } else if ([m_outputContext respondsToSelector:@selector(outputDevice)]) {
> +        if (auto *outputDevice = [m_outputContext outputDevice])
> +            m_hasActiveRoute = outputDevice.deviceFeatures & (AVOutputDeviceFeatureVideo | AVOutputDeviceFeatureAudio);
> +    } else
> +        m_hasActiveRoute = m_outputContext.get().deviceName;
> +
> +    if (![m_outputContext respondsToSelector:@selector(supportsMultipleOutputDevices)] || ![m_outputContext supportsMultipleOutputDevices] || ![m_outputContext respondsToSelector:@selector(outputDevices)]) {
> +        if (auto *outputDevice = [m_outputContext outputDevice]) {
> +            if (outputDevice.deviceFeatures & AVOutputDeviceFeatureVideo)
> +                m_supportsRemoteVideoPlayback = true;
> +        }
> +    } else {
> +        for (AVOutputDevice *outputDevice in [m_outputContext outputDevices]) {
> +            if (outputDevice.deviceFeatures & AVOutputDeviceFeatureVideo)
> +                m_supportsRemoteVideoPlayback = true;
> +        }
> +    }

I don't think we should do this work in the constructor, as we don't know if this information will ever be accessed. I would put it in the accessor methods instead, and ASSERT if they are called on an object with a serialized context.

> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.mm:112
> +        ASSERT(!result || [result conformsToProtocol:@protocol(NSSecureCoding)]);

Shouldn't this fail in a release build if it would assert in a debug build?

[Peng Liu]

Created attachment 426035 [details]
Fix issues on iPad

[Peng Liu]

Comment on attachment 425955 [details]
Fix issues found in manual tests

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

>> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.h:63
>> +        , m_supportsRemoteVideoPlayback(supportsRemoteVideoPlayback)
> 
> There is no guarantee that this state won't change while the output context is serialized, so we shouldn't cache it. See my comment below about how to handle it.

Based on an offline discussion, only caching "hasActiveRoute" seems a reasonable approach. Because `HTMLMediaElement::setWirelessPlaybackTarget()` will access that property of a serialized `AVOutputContext`.

>> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.mm:112
>> +        ASSERT(!result || [result conformsToProtocol:@protocol(NSSecureCoding)]);
> 
> Shouldn't this fail in a release build if it would assert in a debug build?

Agree. Will fix it. And actually, we need to assert that result is not nil.

[Peng Liu]

Created attachment 426066 [details]
Revise the patch after a discussion with Eric

[Eric Carlson]

Comment on attachment 426066 [details]
Revise the patch after a discussion with Eric

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

> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.mm:41
> +    if (m_outputContext)

If the context us null m_type will be ‘unknown’, which doesn’t seem right. I think we should always set m_type or assert if it isn’t valid to have a null context

> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.mm:97
> +    ASSERT(m_type == MediaPlaybackTargetContext::Type::AVOutputContext);

This same assertion is done above

> Source/WebCore/platform/mock/MediaPlaybackTargetMock.h:46
> +    bool hasActiveRoute() const final { return m_context.hasActiveRoute(); }

If we did this in the base class, the two derived classes wouldn’t have to do anything

> Source/WebCore/platform/mock/MediaPlaybackTargetMock.h:47
> +    String deviceName() const final { return m_context.deviceName(); }

Ditto

> Source/WebCore/platform/mock/MediaPlaybackTargetMock.h:48
> +    bool supportsRemoteVideoPlayback() const final { return m_context.supportsRemoteVideoPlayback(); }

Ditto

[Peng Liu]

Created attachment 426117 [details]
Revise the patch based on Eric's comments

[Peng Liu]

Created attachment 426123 [details]
Rebase the patch

[Peng Liu]

Comment on attachment 426066 [details]
Revise the patch after a discussion with Eric

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

>> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.mm:41
>> +    if (m_outputContext)
> 
> If the context us null m_type will be ‘unknown’, which doesn’t seem right. I think we should always set m_type or assert if it isn’t valid to have a null context

Agree, fixed!

>> Source/WebCore/platform/graphics/cocoa/MediaPlaybackTargetContext.mm:97
>> +    ASSERT(m_type == MediaPlaybackTargetContext::Type::AVOutputContext);
> 
> This same assertion is done above

Fixed.

>> Source/WebCore/platform/mock/MediaPlaybackTargetMock.h:46
>> +    bool hasActiveRoute() const final { return m_context.hasActiveRoute(); }
> 
> If we did this in the base class, the two derived classes wouldn’t have to do anything

Good point! And these virtual methods can be removed.
Fixed this one the following ones.

[Eric Carlson]

Comment on attachment 426123 [details]
Rebase the patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2470
> +    encoder << static_cast<uint8_t>(target.mockState());

The cast shouldn't be necessary.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2495
> +    uint8_t mockState;
>      if (!decoder.decode(mockState))
>          return false;
>  
> -    target = MediaPlaybackTargetContext(mockDeviceName, static_cast<MediaPlaybackTargetContext::State>(mockState));
> +    target = MediaPlaybackTargetContext(deviceName, static_cast<MediaPlaybackTargetContext::MockState>(mockState));

Ditto

> Source/WebKit/UIProcess/WebPageProxy.cpp:9413
> +    if (preferences().useGPUProcessForMediaEnabled() && context.type() == MediaPlaybackTargetContext::Type::AVOutputContext && !context.serializeOutputContext())
> +        return;

I don't think this ever happen, can it?

[Peng Liu]

Comment on attachment 426123 [details]
Rebase the patch

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

>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2470
>> +    encoder << static_cast<uint8_t>(target.mockState());
> 
> The cast shouldn't be necessary.

Ah, you are right! Fixed.

>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2495
>> +    target = MediaPlaybackTargetContext(deviceName, static_cast<MediaPlaybackTargetContext::MockState>(mockState));
> 
> Ditto

Fixed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:9413
>> +        return;
> 
> I don't think this ever happen, can it?

Hmm, in the current implementation of `MediaPlaybackTargetContext::serializeOutputContext()`, it won't happen because we check "context.type() == MediaPlaybackTargetContext::Type::AVOutputContext" before calling it. Maybe we can remove one of the two protections?

[Peng Liu]

Created attachment 426130 [details]
[fast-cq] Patch for landing

[Peng Liu]

Comment on attachment 426123 [details]
Rebase the patch

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

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:9413
>>> +        return;
>> 
>> I don't think this ever happen, can it?
> 
> Hmm, in the current implementation of `MediaPlaybackTargetContext::serializeOutputContext()`, it won't happen because we check "context.type() == MediaPlaybackTargetContext::Type::AVOutputContext" before calling it. Maybe we can remove one of the two protections?

Fixed by removing "context.type() == MediaPlaybackTargetContext::Type::AVOutputContext" here.

[Peng Liu]

Comment on attachment 426123 [details]
Rebase the patch

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

>>>> Source/WebKit/UIProcess/WebPageProxy.cpp:9413
>>>> +        return;
>>> 
>>> I don't think this ever happen, can it?
>> 
>> Hmm, in the current implementation of `MediaPlaybackTargetContext::serializeOutputContext()`, it won't happen because we check "context.type() == MediaPlaybackTargetContext::Type::AVOutputContext" before calling it. Maybe we can remove one of the two protections?
> 
> Fixed by removing "context.type() == MediaPlaybackTargetContext::Type::AVOutputContext" here.

Oops, just found that we need a protection here to avoid serializing a mock context for test.

[Peng Liu]

Created attachment 426149 [details]
[fast-cq] Patch for landing (v2)

[EWS]

Committed r276107 (236607@main): <https://commits.webkit.org/236607@main>

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

[Fujii Hironori]

This breaks non-Cocoa ports.
Filed: Bug 224649 – WebCoreArgumentCoders.h: error: 'WebCore/MediaPlaybackTargetContext.h' file not found since r276107

[WebKit Commit Bot]

Re-opened since this is blocked by bug 224653

[Peng Liu]

Created attachment 426194 [details]
Patch for landing (v3): fix clean build failures on some ports

[Fujii Hironori]

Comment on attachment 426194 [details]
Patch for landing (v3): fix clean build failures on some ports

WinCairo clean build fails. Looks like the same failure of bug #224649 comment #3.

> FAILED: Source/WebKit/CMakeFiles/WebKit.dir/__/__/WebKit/DerivedSources/WebPageProxyMessageReceiver.cpp.obj
> C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x64\cl.exe  (...) -c WebKit\DerivedSources\WebPageProxyMessageReceiver.cpp
> C:\home\webkit\gb\WebKitBuild\Debug\WebKit\DerivedSources\WebPageProxyMessages.h(65): fatal error C1083: Cannot open include file: 'WebCore/MediaPlaybackTargetContext.h': No such file or directory

[Peng Liu]

(In reply to Fujii Hironori from comment #25)
> Comment on attachment 426194 [details]
> Patch for landing (v3): fix clean build failures on some ports
> 
> WinCairo clean build fails. Looks like the same failure of bug #224649
> comment #3.
> 
> > FAILED: Source/WebKit/CMakeFiles/WebKit.dir/__/__/WebKit/DerivedSources/WebPageProxyMessageReceiver.cpp.obj
> > C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x64\cl.exe  (...) -c WebKit\DerivedSources\WebPageProxyMessageReceiver.cpp
> > C:\home\webkit\gb\WebKitBuild\Debug\WebKit\DerivedSources\WebPageProxyMessages.h(65): fatal error C1083: Cannot open include file: 'WebCore/MediaPlaybackTargetContext.h': No such file or directory

Thanks for your help!

Looks like we need a change like below:

--- a/Source/WebKit/Scripts/webkit/messages.py
+++ b/Source/WebKit/Scripts/webkit/messages.py
@@ -352,6 +352,7 @@ def conditions_for_header(header):
         '"InputMethodState.h"': ["PLATFORM(GTK)", "PLATFORM(WPE)"],
         '"LayerHostingContext.h"': ["PLATFORM(COCOA)", ],
         '"GestureTypes.h"': ["PLATFORM(IOS_FAMILY)"],
+        '<WebCore/MediaPlaybackTargetContext.h>': ["ENABLE(WIRELESS_PLAYBACK_TARGET)"],
     }
     if not header in conditions:
         return None

[Peng Liu]

Created attachment 426242 [details]
[fast-cq] Patch for landing (v4): fix clean build failures on the WinCairo port

[Peng Liu]

Comment on attachment 426242 [details]
[fast-cq] Patch for landing (v4): fix clean build failures on the WinCairo port

Verified that this patch works fine with a clean WinCairo build.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Peng Liu]

Created attachment 426272 [details]
[fast-cq] Rebase the patch again for landing

[EWS]

Committed r276177 (236661@main): <https://commits.webkit.org/236661@main>

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