AVIF support #671
AVIF support #671
Conversation
cryptomilk
left a comment
cryptomilk
left a comment
There was a problem hiding this comment.
I've added some comments here. For now I just looked at decoding. I think we should get that right before we move on to encoding which is more tricky ...
|
Looks like we're close here! I'd like to resolve outstanding issues and see if this is ready for merge. Here's what I see: CMake build processThe current process works. We'd like to get rid of Color profilesWhen the user requests that we decode an AVIF image that's not sRGB, should we throw an error, or simply let them do it? I'd be inclined to simply emit a warning, but I don't know if developers using a language wrapper would see this. Examples@vapier , I created the human-usable encoding and decoding programs in
Testsim2im tests are currently failing because R, G, and B values are often off by 1. I'm not sure why this is happening - if the lossless encoding isn't truly lossless, whether a bit of precision is being lost somewhere, or I've done something I shouldn't have. Should I add some more basic unit tests in any case? EtcIs there anything else we need here? Thanks, everyone! |
|
Update: lossless conversion is now 100% lossless, and all current tests pass. |
|
You need to rebase your branch on the master branch of libgd. Here is a patch which applies to your current tree: |
|
Update 2 - cryptomilk has fixed the cmake build process. Here's what remains: Color profilesWhen the user requests that we decode an AVIF image that's not sRGB, should we throw an error, or simply let them do it? I'd be inclined to simply emit a warning, but I don't know if developers using a language wrapper would see this. Examples@vapier , I created the human-usable encoding and decoding programs in
Tests
# Note: We ignore config.h.cmake changes since it indirectly
# depends on the format of the output of autoheader :/. |
vapier
left a comment
vapier
left a comment
There was a problem hiding this comment.
i think it's fine to emit a warning for now via gd_error(...). we already expose a framework to users with gdSetErrorMethod so they can capture/display all such messages.
move your little test examples to the examples/ dir and call it a day. we don't build any of those atm, and we should fix that, but we don't have to hold this PR up for it.
wrt the tests, i wouldn't worry about 100% coverage ... just do a basic set that you think is reasonable. but the test files seem to be larger than necessary. smaller is better here as the git history is forever. so shrink them down -- ideally <50k per file.
i'll take a look at the generated cmake header file.
|
travis failures should be fixed now. i broke it earlier due to not fully reading/understanding the logs. sorry about that. you'll need to rebase onto the latest master. |
Thanks! Sadly when I tried to rebase, git kept getting stuck resurrecting old commits and asking me to resolve conflicts there. Finally I just did a merge. Sorry about the redundant commit logs. |
Codecov Report
@@ Coverage Diff @@
## master #671 +/- ##
==========================================
- Coverage 76.01% 75.93% -0.09%
==========================================
Files 252 254 +2
Lines 13680 13707 +27
==========================================
+ Hits 10399 10408 +9
- Misses 3281 3299 +18
Continue to review full report at Codecov.
|
vapier
left a comment
vapier
left a comment
There was a problem hiding this comment.
mostly nits
@cryptomilk want to take one last pass before we merge ?
| find_library(WEBP_LIBRARY | ||
| NAMES ${WEBP_NAMES} | ||
| PATHS "${PROJECT_SOURCE_DIR}/../deps/lib" /usr/lib64 /usr/lib /usr/local/lib | ||
| PATHS /usr/lib64 /usr/lib /usr/local/lib |
There was a problem hiding this comment.
i've just pulled this out as a sep commit and pushed it to master now
src/gd_avif.c
Outdated
|
|
||
| encoder = avifEncoderCreate(); | ||
| int quantizerQuality = quality == QUALITY_DEFAULT ? | ||
| QUANTIZER_DEFAULT : quality2Quantizer(quality); |
There was a problem hiding this comment.
is this better? I just indented the second line by a single tab, instead of trying to line up the second line horizontally with the word "quality".
There was a problem hiding this comment.
And I just made the same change a few lines earlier:
avifPixelFormat subsampling = quality >= HIGH_QUALITY_SUBSAMPLING_THRESHOLD ?
CHROMA_SUBAMPLING_HIGH_QUALITY : CHROMA_SUBSAMPLING_DEFAULT;
|
Ok, I've rebased this on the new master after #670 was merged. Merging, I realize AVIF support was missing from I combined the new I also see that the Tested the merged code... and it built with CMake, and the AVIF tests pass! |
7 participants
Demand for AVIF support on the web is growing, as the word gets out about this new file format which allows higher-quality encoding at smaller sizes. Core contributors to major open-source CMSs are interested in auto-generating AVIF images! They've been simply waiting for support to appear in libgd.
This PR aims to meet the growing demand, and to help bring smaller, more beautiful images to more of the web - to sites created by experienced developers and CMS users alike.
This PR adds support by incorporating libavif. Another option would be to use libheif. However, it's generally felt that libavif has more complete support for the AVIF format. libavif is also used by the Chromium project and squoosh.app.
In this PR, I've endeavored to incorporate the latest research into best practices for AVIF encoding - not just for default quantizer values, but also an algorithm for determining the number of horizontal tiles, vertical tiles, and threads.
Since this is my first contribution to this project, I've made this a draft PR. A couple of TODOs remain. And during the next days I will be adding more tests.
Thanks for considering this! This fixes #557 .