Bug 216377 - Disable hardware JPEG decoding on x86 Mac
Summary: Disable hardware JPEG decoding on x86 Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Nham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-10 13:13 PDT by Ben Nham
Modified: 2020-09-10 16:52 PDT (History)
17 users (show)

See Also:


Attachments
Patch (14.72 KB, patch)
2020-09-10 13:27 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (14.66 KB, patch)
2020-09-10 15:11 PDT, Ben Nham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Nham 2020-09-10 13:13:43 PDT
Disable hardware JPEG decoding on x86 Mac
Comment 1 Ben Nham 2020-09-10 13:27:00 PDT
Created attachment 408472 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-09-10 13:27:33 PDT
<rdar://problem/68661412>
Comment 3 Geoffrey Garen 2020-09-10 13:42:31 PDT
Comment on attachment 408472 [details]
Patch

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

r=me

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:341
> +    if (PAL::isMediaToolboxFrameworkAvailable() && PAL::canLoad_MediaToolbox_FigPhotoDecompressionSetHardwareCutoff())

If we just need to know whether the framework is available, isn't weak linking the more efficient option?
Comment 4 Said Abou-Hallawa 2020-09-10 14:21:08 PDT
Comment on attachment 408472 [details]
Patch

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

> Source/WTF/wtf/PlatformEnableCocoa.h:237
> +#if !defined(ENABLE_HARDWARE_JPEG)
> +#if PLATFORM(MAC) && CPU(X86_64)
> +#define ENABLE_HARDWARE_JPEG 0
> +#else
> +#define ENABLE_HARDWARE_JPEG 1
> +#endif
> +#endif

Why do we need to define ENABLE_HARDWARE_JPEG when its value is 0? Can't the definition be like the rest in this file?

#if !defined(ENABLE_HARDWARE_JPEG) && !(PLATFORM(MAC) && CPU(X86_64))
#define ENABLE_HARDWARE_JPEG 1
#endif
Comment 5 Ben Nham 2020-09-10 14:24:30 PDT
Weak linking is definitely preferable to soft linking for cases like this. (From asking around it sounds like we are soft linking MediaToolbox not because of a circular dependency but because it doesn't exist in BaseSystem.) But that would be a larger and riskier patch so I'll probably do the work of changing this from a soft link to a weak link in a separate patch.

I'll address Said's comment in the patch for landing.
Comment 6 Geoffrey Garen 2020-09-10 14:31:18 PDT
Sounds good.
Comment 7 Ben Nham 2020-09-10 15:11:42 PDT
Created attachment 408483 [details]
Patch
Comment 8 EWS 2020-09-10 16:52:15 PDT
Committed r266898: <https://trac.webkit.org/changeset/266898>

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