Bug 226869

Created attachment 431056 [details] Patch

<rdar://77548905>

Unfortunately this change works on iOS but it does not work on macOS.

Created attachment 431057 [details] Patch

Created attachment 431169 [details] Patch

Comment on attachment 431169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431169&action=review > Source/WebKit/WebProcess/WebProcess.cpp:341 > + restrictImageAndVideoDecoders(); We should make sure this is called after entering the sandbox. Is this the case here?

Comment on attachment 431169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431169&action=review >> Source/WebKit/WebProcess/WebProcess.cpp:341 >> + restrictImageAndVideoDecoders(); > > We should make sure this is called after entering the sandbox. Is this the case here? If this is iOS only, that should not be an issue.

Comment on attachment 431169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431169&action=review >> Source/WebKit/WebProcess/WebProcess.cpp:341 >> + restrictImageAndVideoDecoders(); > > We should make sure this is called after entering the sandbox. Is this the case here? If this is iOS only, that should not be an issue.

Comment on attachment 431169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431169&action=review >>>> Source/WebKit/WebProcess/WebProcess.cpp:341 >>>> + restrictImageAndVideoDecoders(); >>> >>> We should make sure this is called after entering the sandbox. Is this the case here? >> >> If this is iOS only, that should not be an issue. > > If this is iOS only, that should not be an issue. It is working on iOS only. But I think it should work in both iOS and macOS. Where should I put this call to make sure we entered the sandbox?

Comment on attachment 431169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431169&action=review >>>>> Source/WebKit/WebProcess/WebProcess.cpp:341 >>>>> + restrictImageAndVideoDecoders(); >>>> >>>> We should make sure this is called after entering the sandbox. Is this the case here? >>> >>> If this is iOS only, that should not be an issue. >> >> If this is iOS only, that should not be an issue. > > It is working on iOS only. But I think it should work in both iOS and macOS. Where should I put this call to make sure we entered the sandbox? Yes, the current invocation of restrictImageAndVideoDecoders() is too early on macOS. It should be called after AuxiliaryProcess::initializeSandbox.

Created attachment 431519 [details] Patch

Comment on attachment 431519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431519&action=review > Source/WebCore/PAL/pal/spi/cocoa/VideoToolboxSPI.h:33 > +#include <wtf/SoftLinking.h> Should this include be in the SPI file?

Comment on attachment 431519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431519&action=review >> Source/WebCore/PAL/pal/spi/cocoa/VideoToolboxSPI.h:33 >> +#include <wtf/SoftLinking.h> > > Should this include be in the SPI file? No, it should not. Per Arne is correct that this should be included in the *.cpp or *.mm source file only.

Comment on attachment 431519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431519&action=review To make sure this works on macOS, you can add the following code to WebProcess::platformInitializeWebProcess: #if PLATFORM(MAC) if (parameters.trustedAgentExtensionHandle) { if (auto extension = SandboxExtension::create(WTFMove(*parameters.trustedAgentExtensionHandle))) { bool ok = extension->consume(); if (PAL::isMediaToolboxFrameworkAvailable() && PAL::canLoad_MediaToolbox_FigPhotoSupportsHEVCHWDecode()) PAL::softLinkMediaToolboxFigPhotoSupportsHEVCHWDecode(); ok = extension->revoke(); ASSERT_UNUSED(ok, ok); } } #endif > Source/WebKit/Shared/AuxiliaryProcess.cpp:86 > +#if PLATFORM(IOS_FAMILY) > + restrictImageAndVideoDecoders(); > +#endif To make this work on macOS, you can remove the #ifdef. I also think we should call this in the WebContent process, only.

Created attachment 431634 [details] Patch

Comment on attachment 431519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431519&action=review >>> Source/WebCore/PAL/pal/spi/cocoa/VideoToolboxSPI.h:33 >>> +#include <wtf/SoftLinking.h> >> >> Should this include be in the SPI file? > > No, it should not. Per Arne is correct that this should be included in the *.cpp or *.mm source file only. Removed. >> Source/WebKit/Shared/AuxiliaryProcess.cpp:86 >> +#endif > > To make this work on macOS, you can remove the #ifdef. I also think we should call this in the WebContent process, only. This call was moved to WebProcessCocoa.mm and this made macOS and iOS display the HEIF image correctly.

Comment on attachment 431634 [details] Patch Great! R=me.

Comment on attachment 431634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431634&action=review > Source/WTF/ChangeLog:3 > + [Cocoa] Disable hardware decoding in the WebProcess Cocoa > Source/WebCore/ChangeLog:3 > + [iOS] Disable hardware decoding in the WebProcess iOS > Source/WebCore/PAL/ChangeLog:3 > + [Cocoa] Disable hardware decoding in the WebProcess Cocoa > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:381 > + PAL::softLinkMediaToolboxFigPhotoSupportsHEVCHWDecode(); Does this function have side effects? We're ignoring the return value. > LayoutTests/platform/ios/TestExpectations:3038 > +fast/images/heic-as-background-image.html [ Pass ] Should we pass the test on macOS too?

Created attachment 431726 [details] Patch

Comment on attachment 431634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431634&action=review >> Source/WebCore/ChangeLog:3 >> + [iOS] Disable hardware decoding in the WebProcess > > iOS This one is fixed to match the title of the bug. >> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:381 >> + PAL::softLinkMediaToolboxFigPhotoSupportsHEVCHWDecode(); > > Does this function have side effects? We're ignoring the return value. We do not need the return value. We just need to call it once while holding the trustd extension. This function sets a static local variable when it is called for the first time. To set the static local variable, it uses the service com.apple.trustd.agent. I will add a comment in the code and describe it a little in the ChangeLog. >> LayoutTests/platform/ios/TestExpectations:3038 >> +fast/images/heic-as-background-image.html [ Pass ] > > Should we pass the test on macOS too? The test is now passed on macOS.

Created attachment 431738 [details] Patch

Committed r279030 (238950@main): <https://commits.webkit.org/238950@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431738 [details].

This breaks WebRTC video decoding in WebProcess since we need IOSurface-backed buffers there.

See rdar://81166342 for more info