Bug 224469 - [GPUP] WebContent process should not create AVOutputContext instances when media in GPU Process is enabled
Summary: [GPUP] WebContent process should not create AVOutputContext instances when me...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on: 224653
Blocks: 224649
  Show dependency treegraph
 
Reported: 2021-04-12 20:38 PDT by Peng Liu
Modified: 2021-04-16 15:54 PDT (History)
13 users (show)

See Also:


Attachments
WIP patch (80.57 KB, patch)
2021-04-13 12:43 PDT, Peng Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (81.15 KB, patch)
2021-04-13 12:59 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix layout test failures (81.52 KB, patch)
2021-04-13 20:14 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Rebase the patch (79.79 KB, patch)
2021-04-13 21:26 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix issues found in manual tests (80.95 KB, patch)
2021-04-13 23:32 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix issues on iPad (80.95 KB, patch)
2021-04-14 12:39 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Revise the patch after a discussion with Eric (81.33 KB, patch)
2021-04-14 18:44 PDT, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Revise the patch based on Eric's comments (83.35 KB, patch)
2021-04-15 10:56 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Rebase the patch (82.97 KB, patch)
2021-04-15 11:58 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
[fast-cq] Patch for landing (82.85 KB, patch)
2021-04-15 13:11 PDT, Peng Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
[fast-cq] Patch for landing (v2) (82.84 KB, patch)
2021-04-15 16:02 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch for landing (v3): fix clean build failures on some ports (83.23 KB, patch)
2021-04-16 01:17 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
[fast-cq] Patch for landing (v4): fix clean build failures on the WinCairo port (83.98 KB, patch)
2021-04-16 09:47 PDT, Peng Liu
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
[fast-cq] Rebase the patch again for landing (83.01 KB, patch)
2021-04-16 14:28 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2021-04-12 20:38:46 PDT
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.
Comment 1 Peng Liu 2021-04-12 20:39:07 PDT
<rdar://76403302>
Comment 2 Peng Liu 2021-04-13 12:43:04 PDT
Created attachment 425902 [details]
WIP patch
Comment 3 Peng Liu 2021-04-13 12:59:41 PDT
Created attachment 425904 [details]
WIP patch
Comment 4 Peng Liu 2021-04-13 20:14:23 PDT
Created attachment 425943 [details]
Fix layout test failures
Comment 5 Peng Liu 2021-04-13 21:26:09 PDT
Created attachment 425950 [details]
Rebase the patch
Comment 6 Peng Liu 2021-04-13 23:32:46 PDT
Created attachment 425955 [details]
Fix issues found in manual tests
Comment 7 Eric Carlson 2021-04-14 09:03:20 PDT
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?
Comment 8 Peng Liu 2021-04-14 12:39:27 PDT
Created attachment 426035 [details]
Fix issues on iPad
Comment 9 Peng Liu 2021-04-14 18:25:08 PDT
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.
Comment 10 Peng Liu 2021-04-14 18:44:33 PDT
Created attachment 426066 [details]
Revise the patch after a discussion with Eric
Comment 11 Eric Carlson 2021-04-15 08:29:11 PDT
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
Comment 12 Peng Liu 2021-04-15 10:56:55 PDT
Created attachment 426117 [details]
Revise the patch based on Eric's comments
Comment 13 Peng Liu 2021-04-15 11:58:25 PDT
Created attachment 426123 [details]
Rebase the patch
Comment 14 Peng Liu 2021-04-15 12:01:54 PDT
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.
Comment 15 Eric Carlson 2021-04-15 12:19:23 PDT
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?
Comment 16 Peng Liu 2021-04-15 12:50:05 PDT
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?
Comment 17 Peng Liu 2021-04-15 13:11:17 PDT
Created attachment 426130 [details]
[fast-cq] Patch for landing
Comment 18 Peng Liu 2021-04-15 13:12:54 PDT
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.
Comment 19 Peng Liu 2021-04-15 14:29:14 PDT
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.
Comment 20 Peng Liu 2021-04-15 16:02:30 PDT
Created attachment 426149 [details]
[fast-cq] Patch for landing (v2)
Comment 21 EWS 2021-04-15 19:20:09 PDT
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].
Comment 22 Fujii Hironori 2021-04-15 23:19:03 PDT
This breaks non-Cocoa ports.
Filed: Bug 224649 – WebCoreArgumentCoders.h: error: 'WebCore/MediaPlaybackTargetContext.h' file not found since r276107
Comment 23 WebKit Commit Bot 2021-04-15 23:47:03 PDT
Re-opened since this is blocked by bug 224653
Comment 24 Peng Liu 2021-04-16 01:17:08 PDT
Created attachment 426194 [details]
Patch for landing (v3): fix clean build failures on some ports
Comment 25 Fujii Hironori 2021-04-16 02:00:26 PDT
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
Comment 26 Peng Liu 2021-04-16 09:38:40 PDT
(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
Comment 27 Peng Liu 2021-04-16 09:47:10 PDT
Created attachment 426242 [details]
[fast-cq] Patch for landing (v4): fix clean build failures on the WinCairo port
Comment 28 Peng Liu 2021-04-16 14:09:22 PDT
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.
Comment 29 EWS 2021-04-16 14:16:18 PDT
Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 30 EWS 2021-04-16 14:18:22 PDT
Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 31 EWS 2021-04-16 14:22:13 PDT
Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 32 Peng Liu 2021-04-16 14:28:22 PDT
Created attachment 426272 [details]
[fast-cq] Rebase the patch again for landing
Comment 33 EWS 2021-04-16 15:54:24 PDT
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].