| Summary: | AVIF decoding support | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||
| Component: | Images | Assignee: | ChangSeok Oh <changseok> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | 709922234, agomez, andrew, annulen, aperez, bjr.roberts, calvaris, changseok, chi187, clopez, contact+bugs.webkit.org, cyb.ai.815, dev, E.M.terHoeven, ews-watchlist, gsnedders, gyuyoung.kim, jarilittlenen, jbauman+webkit, joehoyle, kai.hollberg, kyle.bavender, ltilve, mathias, mehmetgelisin, ncooper, nich, nidoyle, ryuan.choi, sergio, the.bull, thomas, tomac, vprelovac, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=224663 | ||||||||||||||
| Bug Depends on: | 212964 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Philippe Normand
2020-02-14 01:36:28 PST
In Firefox they already use dav1d, so it was decided to also use it for AVIF. Here is a WIP branch. https://github.com/shivamidow/webkit/tree/avif-decoding Only still images are supported for now. Created attachment 421855 [details]
Patch
(In reply to ChangSeok Oh from comment #3) > Created attachment 421855 [details] > Patch No review please. This aims to see EWS responses. Created attachment 421857 [details]
Patch
Created attachment 421987 [details]
Patch
Created attachment 422003 [details]
Patch
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 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 It's very easy to build nowadays. But anyway, I can make a first review round on this patch. 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? Created attachment 422288 [details]
Patch
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 on attachment 422288 [details]
Patch
Thanks! LGTM.
Comment on attachment 422288 [details]
Patch
Thanks!
Committed r273970: <https://commits.webkit.org/r273970> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422288 [details]. 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 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. Updates handled in https://bugs.webkit.org/show_bug.cgi?id=224177 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? (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. 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? dav1d 0.9.0 was released today with significant improvements for 10-bit (HDR) decoding on x64 and a new API to signal events: https://code.videolan.org/videolan/dav1d/-/tags/0.9.0 (In reply to Ewout ter Hoeven from comment #23) > Awesome! I think it now comes down to these two questions: > > 2) Who from Apple could give the go-ahead to flip the flag for Apple ports? The Apple ports use the system-provided image decoding capabilities; we're unlikely to deviate from that here, so any support for AVIF depends on underlying OS support. I can't comment as to whether any future OS releases will support AVIF. (In reply to Sam Sneddon [:gsnedders] from comment #25) > (In reply to Ewout ter Hoeven from comment #23) > > Awesome! I think it now comes down to these two questions: > > > > 2) Who from Apple could give the go-ahead to flip the flag for Apple ports? > > The Apple ports use the system-provided image decoding capabilities; we're > unlikely to deviate from that here, so any support for AVIF depends on > underlying OS support. I can't comment as to whether any future OS releases > will support AVIF. AVIF is great and it would be cool to be able to use in a custom-built WebKit on Mac. Would you be able to point to what would need to be changed for a custom WebKit build on Mac to support AVIF? |