Bug 207750 - AVIF decoding support
Summary: AVIF decoding support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: ChangSeok Oh
URL:
Keywords: InRadar
Depends on: 212964
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-14 01:36 PST by Philippe Normand
Modified: 2021-04-19 00:59 PDT (History)
31 users (show)

See Also:


Attachments
Patch (509.01 KB, patch)
2021-03-01 12:29 PST, ChangSeok Oh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (525.58 KB, patch)
2021-03-01 13:05 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (30.20 KB, patch)
2021-03-02 13:37 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (33.16 KB, patch)
2021-03-02 15:19 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (35.56 KB, patch)
2021-03-04 14:39 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2020-02-14 01:36:28 PST
Reference implementation: https://github.com/AOMediaCodec/libavif
Spec: https://aomediacodec.github.io/av1-avif/
Comment 1 Philippe Normand 2020-03-10 10:57:26 PDT
In Firefox they already use dav1d, so it was decided to also use it for AVIF.
Comment 2 ChangSeok Oh 2020-06-09 21:28:07 PDT
Here is a WIP branch. https://github.com/shivamidow/webkit/tree/avif-decoding
Only still images are supported for now.
Comment 3 ChangSeok Oh 2021-03-01 12:29:44 PST
Created attachment 421855 [details]
Patch
Comment 4 ChangSeok Oh 2021-03-01 12:30:38 PST
(In reply to ChangSeok Oh from comment #3)
> Created attachment 421855 [details]
> Patch

No review please. This aims to see EWS responses.
Comment 5 ChangSeok Oh 2021-03-01 13:05:59 PST
Created attachment 421857 [details]
Patch
Comment 6 ChangSeok Oh 2021-03-02 13:37:20 PST
Created attachment 421987 [details]
Patch
Comment 7 ChangSeok Oh 2021-03-02 15:19:27 PST
Created attachment 422003 [details]
Patch
Comment 8 Philippe Normand 2021-03-02 23:38:09 PST
Comment on attachment 422003 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch brings an initial support of AVIF image format to the gtk port.

I suspect supporting WPE wouldn't be much work, can you take a look? Otherwise it can be handled later on.
Comment 9 ChangSeok Oh 2021-03-03 11:18:51 PST
Comment on attachment 422003 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        This patch brings an initial support of AVIF image format to the gtk port.
> 
> I suspect supporting WPE wouldn't be much work, can you take a look? Otherwise it can be handled later on.

Let me look into wpe later. To be honest, I haven't even built it. :P
Comment 10 Philippe Normand 2021-03-03 11:42:37 PST
It's very easy to build nowadays. But anyway, I can make a first review round on this patch.
Comment 11 Philippe Normand 2021-03-04 08:46:24 PST
Comment on attachment 422003 [details]
Patch

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

> Source/WebCore/platform/image-decoders/avif/AVIFImageDecoder.cpp:52
> +

Empty line can be removed.

> Source/WebCore/platform/image-decoders/avif/AVIFImageReader.h:47
> +    AVIFImageDecoder* m_decoder;

Could this be a RefPtr?

> Source/WebCore/platform/image-decoders/avif/AVIFImageReader.h:48
> +    avifDecoder* m_avifDecoder;

Maybe unique_ptr with custom deleter?
Comment 12 ChangSeok Oh 2021-03-04 14:39:45 PST
Created attachment 422288 [details]
Patch
Comment 13 ChangSeok Oh 2021-03-04 14:42:00 PST
Comment on attachment 422003 [details]
Patch

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

Thanks for your comments!

>> Source/WebCore/platform/image-decoders/avif/AVIFImageDecoder.cpp:52
>> +
> 
> Empty line can be removed.

Removed.

>> Source/WebCore/platform/image-decoders/avif/AVIFImageReader.h:47
>> +    AVIFImageDecoder* m_decoder;
> 
> Could this be a RefPtr?

Replaed this line with RefPtr<AVIFImageDecoder>.

>> Source/WebCore/platform/image-decoders/avif/AVIFImageReader.h:48
>> +    avifDecoder* m_avifDecoder;
> 
> Maybe unique_ptr with custom deleter?

AVIFUniquePtr is newly added.
Comment 14 Philippe Normand 2021-03-05 00:29:51 PST
Comment on attachment 422288 [details]
Patch

Thanks! LGTM.
Comment 15 ChangSeok Oh 2021-03-05 08:04:28 PST
Comment on attachment 422288 [details]
Patch

Thanks!
Comment 16 EWS 2021-03-05 08:15:12 PST
Committed r273970: <https://commits.webkit.org/r273970>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422288 [details].
Comment 17 Radar WebKit Bug Importer 2021-03-05 08:16:17 PST
<rdar://problem/75095248>
Comment 18 Ewout ter Hoeven 2021-03-09 13:01:05 PST
Thanks a lot for this effort, it's great to see AVIF decoding support landing in WebKit!

I noticed that currently libavif 0.7.3 (from May 2020) and dav1d 0.7.0 (also from  May 2020) are specified in this patch. I don't know if these versions are used in production or only for CI/tooling, but in either case I would be nice having the latest versions. In a nutshell, dav1d added more assembly code for faster decoding and libavif has improved compatibility and features, since the currently used versions.

For updating dav1d from 0.7.0 to 0.8.2 in tools/gtk I submitted a patch: https://bugs.webkit.org/show_bug.cgi?id=222971

Updating libavif (to 0.9.0) could be a little less straight forward, because it has some breaking changes since the current version (which is 0.7.3), mainly around the CICP refactor (which handels color profiles and such) in 0.8.0.

Also libavif 0.8.2 added the amazing avifIO reader API, which allows parsing meta data and start decoding without having the full AVIF payload yet. 
Chromium has implemented this recently, see this issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1136922
Comment 19 Jon Bauman 2021-03-09 14:46:25 PST
Seconding the call for updating libdav1d. On Firefox we discovered at least one crashing issue in dav1d with AVIF that was fixed between 0.8.1 and 0.8.2: https://bugzilla.mozilla.org/show_bug.cgi?id=1691376.
Comment 20 Philippe Normand 2021-04-05 01:44:46 PDT
Updates handled in https://bugs.webkit.org/show_bug.cgi?id=224177
Comment 21 Ewout ter Hoeven 2021-04-14 01:03:52 PDT
With Animated AVIF support implemented, dav1d and libavif updated and the parsing fixed, would it be time to be considering flipping the flag and enabling AVIF support by default?
Comment 22 Philippe Normand 2021-04-14 01:10:01 PDT
(In reply to Ewout ter Hoeven from comment #21)
> With Animated AVIF support implemented, dav1d and libavif updated and the
> parsing fixed, would it be time to be considering flipping the flag and
> enabling AVIF support by default?

For the GTK and WPE ports I would be in favor.
For the Apple ports, I can't speak.
Comment 23 Ewout ter Hoeven 2021-04-15 08:29:23 PDT
Awesome! I think it now comes down to these two questions:

1) Can we enable AVIF decoding by default for the GTK and WPE ports only (for now)?
2) Who from Apple could give the go-ahead to flip the flag for Apple ports?