Bug 226869 – [Cocoa] Disable hardware decoding in the WebProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

IOKit calls are sandboxed in the WebProcess. So we need to disable hardware decoding otherwise the images will not be displayed correctly.

[Said Abou-Hallawa]

Created attachment 431056 [details]
Patch

[Said Abou-Hallawa]

<rdar://77548905>

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 431057 [details]
Patch

[Said Abou-Hallawa]

Created attachment 431169 [details]
Patch

[Per Arne Vollan]

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?

[Per Arne Vollan]

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.

[Per Arne Vollan]

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.

[Said Abou-Hallawa]

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?

[Per Arne Vollan]

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.

[Said Abou-Hallawa]

Created attachment 431519 [details]
Patch

[Per Arne Vollan]

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?

[David Kilzer (:ddkilzer)]

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.

[Per Arne Vollan]

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.

[Said Abou-Hallawa]

Created attachment 431634 [details]
Patch

[Said Abou-Hallawa]

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.

[Per Arne Vollan]

Comment on attachment 431634 [details]
Patch

Great! R=me.

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

Created attachment 431726 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 431738 [details]
Patch

[EWS]

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].

[youenn fablet]

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

[youenn fablet]

See rdar://81166342 for more info