Summary: | [GPUP] WebContent process should not create AVOutputContext instances when media in GPU Process is enabled | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||||||||||||||||||||||||||
Component: | Media | Assignee: | Peng Liu <peng.liu6> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, commit-queue, eric.carlson, ews-watchlist, glenn, gyuyoung.kim, Hironori.Fujii, jer.noble, philipj, ramtinbeheshti, ryuan.choi, sergio, webkit-bug-importer | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | Safari Technology Preview | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=224328 | ||||||||||||||||||||||||||||||||
Bug Depends on: | 224653 | ||||||||||||||||||||||||||||||||
Bug Blocks: | 224649 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Peng Liu
2021-04-12 20:38:46 PDT
Created attachment 425902 [details]
WIP patch
Created attachment 425904 [details]
WIP patch
Created attachment 425943 [details]
Fix layout test failures
Created attachment 425950 [details]
Rebase the patch
Created attachment 425955 [details]
Fix issues found in manual tests
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? Created attachment 426035 [details]
Fix issues on iPad
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. Created attachment 426066 [details]
Revise the patch after a discussion with Eric
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 Created attachment 426117 [details]
Revise the patch based on Eric's comments
Created attachment 426123 [details]
Rebase the patch
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 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 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? Created attachment 426130 [details]
[fast-cq] Patch for landing
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 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. Created attachment 426149 [details]
[fast-cq] Patch for landing (v2)
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]. This breaks non-Cocoa ports. Filed: Bug 224649 – WebCoreArgumentCoders.h: error: 'WebCore/MediaPlaybackTargetContext.h' file not found since r276107 Re-opened since this is blocked by bug 224653 Created attachment 426194 [details]
Patch for landing (v3): fix clean build failures on some ports
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 (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 Created attachment 426242 [details]
[fast-cq] Patch for landing (v4): fix clean build failures on the WinCairo port
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.
Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Tools/Scripts/svn-apply failed to apply attachment 426242 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Created attachment 426272 [details]
[fast-cq] Rebase the patch again for landing
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]. |