Bug 236428 – Support AVIF images natively

• Component: Images
• Status: RESOLVED CONFIGURATION CHANGED

[Said Abou-Hallawa]

This will import libavif, dav1d and nasm projects to WebKit.

[Said Abou-Hallawa]

Created attachment 451491 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452268 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/89065897>

[Said Abou-Hallawa]

Created attachment 452317 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452328 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452437 [details]
Patch

[Aakash Jain]

Cancelled https://ews-build.webkit.org/#/builders/70/builds/624 to speed up mac-wk2 queue, it already run layout-tests and the failure seemed pre-existing.

[Said Abou-Hallawa]

Created attachment 452722 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452762 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452823 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452826 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452847 [details]
Patch

[Tim Horton]

I would elide the massive and useless list of changed symbols.

`WK_LIBAVIF_LDFLAGS` seems sorted wrong.

It seems important to note in the changelog that you've disabled this on Apple Silicon. That will confuse people for sure.

`NASM_HEADER_SEARCH_PATHS` and `HEADER_SEARCH_PATHS` should probably be defined in an xcconfig, not the xcodeproj (also, all of the other properties set in the xcodeproj, there are a bunch). Search for `XCBuildConfiguration` in the patch. Should be nearly empty, getting everything from xcconfigs.

There's a bunch of seemingly-generated code. Is it right that we're checking that in, or should  we be generating it? (I have no idea).

[Said Abou-Hallawa]

Created attachment 452932 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452934 [details]
Patch

Excluding frameworks

[Tim Horton]

Comment on attachment 452934 [details]
Patch

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

> Source/ThirdParty/ChangeLog:25
> +        -- nasm is used to compile the assembly sources in dava1d.

spelling: "dava1d"

> Source/ThirdParty/libavif/Configurations/dav1d.xcconfig:59
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=iphoneos*] = $(EXCLUDED_SOURCE_FILE_NAMES_x86);
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=iphonesimulator*] = $(EXCLUDED_SOURCE_FILE_NAMES_arm);
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=iphonesimulator*][arch=arm64*] = $(EXCLUDED_SOURCE_FILE_NAMES_x86);
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*] = $(EXCLUDED_SOURCE_FILE_NAMES_arm);
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*][arch=arm64*] = $(EXCLUDED_SOURCE_FILE_NAMES_x86);

Does this all have to be so complicated? Is it not just an architecture conditional in the end?

> Source/ThirdParty/libavif/Configurations/libavif.xcconfig:55
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=iphonesimulator*] = codec_aom.c codec_libgav1.c codec_rav1e.c codec_svt.c;
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=iphoneos*] = codec_aom.c codec_libgav1.c codec_rav1e.c codec_svt.c;
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macos*] = codec_aom.c codec_libgav1.c codec_rav1e.c codec_svt.c;

Are these intentionally included only in the watchOS and tvOS builds? If you meant to exclude them everywhere, why not list them only once? Or better, remove them from the project?

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:110
> +  humanReadableName: "Avif image decoding"
> +  humanReadableDescription: "Enable Avif image decoding and rendering"

As far as I can tell both of these should be all-capital "AVIF"

> Source/WebCore/loader/cache/CachedImage.cpp:512
> +        else
> +#endif
> +            // Have the image update its data from its internal buffer. Decoding the image data
> +            // will be delayed until info (like size or specific image frames) are queried which
> +            // usually happens when the observers are repainted.
> +            encodedDataStatus = updateImageData(false);

Multi-line statement after an else with no braces separated by a preprocessor directive is a recipe for a future mistake; can you reorganize this to avoid that? (I'd go with adding some braces, but there are many options)

[Said Abou-Hallawa]

Created attachment 452940 [details]
Patch

[Said Abou-Hallawa]

Created attachment 452941 [details]
Patch (Frameworks source excluded)

[Said Abou-Hallawa]

Comment on attachment 452934 [details]
Patch

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

>> Source/ThirdParty/ChangeLog:25
>> +        -- nasm is used to compile the assembly sources in dava1d.
> 
> spelling: "dava1d"

Fixed

>> Source/ThirdParty/libavif/Configurations/dav1d.xcconfig:59
>> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*][arch=arm64*] = $(EXCLUDED_SOURCE_FILE_NAMES_x86);
> 
> Does this all have to be so complicated? Is it not just an architecture conditional in the end?

This setting was simplified.

>> Source/ThirdParty/libavif/Configurations/libavif.xcconfig:55
>> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macos*] = codec_aom.c codec_libgav1.c codec_rav1e.c codec_svt.c;
> 
> Are these intentionally included only in the watchOS and tvOS builds? If you meant to exclude them everywhere, why not list them only once? Or better, remove them from the project?

This setting was removed and the files were removed.

>> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:110
>> +  humanReadableDescription: "Enable Avif image decoding and rendering"
> 
> As far as I can tell both of these should be all-capital "AVIF"

Fixed.

>> Source/WebCore/loader/cache/CachedImage.cpp:512
>> +            encodedDataStatus = updateImageData(false);
> 
> Multi-line statement after an else with no braces separated by a preprocessor directive is a recipe for a future mistake; can you reorganize this to avoid that? (I'd go with adding some braces, but there are many options)

Fixed.

[Adam Silverstein]

Hi Said,

Excited to see this ticket opened to add AVIF support to WebKit! 

I am a WordPress media component maintainer and I've been working with the core Performance team to help add modern image format support. 

AVIF looks like a promising improvement for WordPress and the web at scale and having support in WebKit would potentially unblock adding support for AVIF directly in WordPress core.

I noticed you haven't updated this ticket recently, is this something you are still planning to work on?

[Myles C. Maxfield]

AVIF images are supported in the betas of macOS Ventura and iOS 16.

This Bugzilla bug is about supporting AVIF images in previous versions of macOS (before Ventura).