Let GDToolkit support AVIF image format

Thanks for creating this issue... this is exciting!

The relevant libgd release is on its way.

I imagine Drupal uses PHP's getimagesize() function, which will need support here as you mention... is that right? Is anything else needed?

Updated IS with existing issue for PHP https://bugs.php.net/bug.php?id=78832

There's already a PR to add AVIF support to getimagesize()? That's spectacular!

Talking to PHP core maintainers this weekend, I learned that PHP's GD diverged from libgd long ago. So my libgd PR needs to be adapted for the PHP codebase.

Fortunately that's mostly a matter of allowing it to build with libavif, customizing the main code, and a few other things.

I will coordinate with @mixologic on the process to make a testbot available.

Super exciting, thanks so much @benmorss! 😊

Meantime I used to try use avif via brand new ffi PHP extension https://github.com/andypost/php-ffi-libavif

Looks it's enough just 3 library structures and functions to read size, so promising to get support in PHP 8.1 but then a question of conversion appears...

Right now only imagemagick (not graphicsmagick yet) can be used via dgo.to/imagemagick

PS: PHP pecl extension imagick for PHP 8 just got alpha release

I've started doing the work to propagate the libgd support into PHP's bundled gd fork. The PHP folks tell me there shouldn't be trouble getting this into PHP 8.1.

I just won't get to finish this up until April. (Unless, of course, someone gets inspired to do it sooner 😃)

But, yep, imagemagick already supports AVIF via libheif. I haven't tried out their implementation, so I don't know what they've chosen for encoding defaults and the like.

Support for bundled libgd commited via https://github.com/php/php-src/pull/7026

The implementation AVIF support for getimagesize() and imagecreatefromstring() looks ready, so chances are high to get support in 8.1

Indeed, it's looking good! https://github.com/php/php-src/pull/7091 will add that support, and it's in review right now.

The algorithm I used to determine whether an image's AVIF comes right from Joe Drago, a core maintainer of libavif.

Note that there's no reliable way to determine an AVIF image's size without lots of context, so PHP will simply return 0's for height and width. But it will tell you definitely whether the image's indeed an AVIF.

Last brick landed into PHP 8.1! Thank you @benmorss a lot to make it happen!

Beta1 ready to be build https://downloads.php.net/~ramsey/

Using --with-avif (requires libavif-dev) and build-in libgd it could be tested (asked @mixologic)

Currently only polishing PR left for coming beta-RC times https://github.com/php/php-src/pull/7253

Disable strict pixi requirement for libavif >= 0.9.1

Sorry I missed the recent comments here! @andypost, thanks for trying this out and making comments in the PHP repo as you find things.

To @mondrake:

That's weird... and inconsistent with other image types. But, OK. For the purpose of this issue, this means that AVIF files will not be able to be 'lazy-loaded' like other files.

I agree. Joe Drago, a lead on libavif, has simply told me there's no simple way to reliably get an AVIF image's height and width. Of course, if you link to libavif, you can do it that way. The problem here is that getimagesize() does not use gd.

On the other hand, if you create a GD image with imagecreatefromavif(), you can get its width and height using imagesx() and imagesy(). Does that help at all?

@mondrake the imagecreatefromavif() using memory stream implementation IIRC so it will not load whole file when only metadata required

#26 @mondrake yeah, I know it's sadly quite inefficient to go through the overhead of creating the gd image just to get its size. I saw how simple it is for PHP to get dimensions for other types of images. I wish AVIF was the same!

Joe points out that libavif itself doesn't take much work to read an image and get to the point where it knows things like its size. I guess we could use libavif in this way, but I can only imagine the complexities that would come with providing and running a binary along with Drupal 😭

Your test hit a segfault?

Have you reported that in the PHP project? It sounds concerning.

@mondrake you could use Alpinelinux testing repo to build your image

docker run --rm alpine:edge sh -c 'apk add -X https://dl-cdn.alpinelinux.org/alpine/edge/testing php81 php81-gd && php81 --ri gd'
...
(21/30) Installing aom-libs (3.1.2-r0)
(22/30) Installing libdav1d (0.9.2-r0)
(23/30) Installing libavif (0.9.2-r2)
...
gd

GD Support => enabled
GD Version => bundled (2.1.0 compatible)
FreeType Support => enabled
FreeType Linkage => with freetype
FreeType Version => 2.11.0
GIF Read Support => enabled
GIF Create Support => enabled
JPEG Support => enabled
libJPEG Version => 8
PNG Support => enabled
libPNG Version => 1.6.37
WBMP Support => enabled
XPM Support => enabled
libXpm Version => 30411
XBM Support => enabled
WebP Support => enabled
BMP Support => enabled
AVIF Support => enabled
TGA Read Support => enabled

Directive => Local Value => Master Value
gd.jpeg_ignore_warning => 1 => 1

@mondrake, as far as getimagesize() goes, tentative good news: some folks at Google who specialize in AVIF are working on writing something compact that could be used to get the size of an AVIF image. If they succeed, I'll submit a patch for PHP 8.2.

I think we could add extra logic around getimagesize() because AVIF could have a set of image sizes so making "thumbnails" logic needs changes to select the most optimized base to apply imagestyle's plugins

@mondrake, is this then caused by the lack of an AVIF codec?

If so, it was fixed in the GD project . I've asked Pierre if he could replicate this work in PHP....

@mondrake @benmorss I did fix it only in alpine:edge with https://git.alpinelinux.org/aports/commit/?id=e4fba93fd9d83f520645469cae...

Moreover version of "libavif" in alpine 3.14 is 0.9.1 so I asked maintainer to upgrade it https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/25866

I'm packaging skilldlabs/php:81 image and it works for me

$ docker run --rm skilldlabs/php:81 php -r 'var_dump(imageavif(imagecreatetruecolor(8, 8), "/tmp/test"));'
bool(true)

#3202016-40: Let GDToolkit support AVIF image format @mondrake: yes!

libgd: https://github.com/libgd/libgd/issues/770
php: https://bugs.php.net/bug.php?id=81217

@benmorss any idea why libavif is not displaying version of library via phpinfo() I think it could be handy looking like other image-types doing it

$ php --ri gd

gd

GD Support => enabled
GD Version => bundled (2.1.0 compatible)
FreeType Support => enabled
FreeType Linkage => with freetype
FreeType Version => 2.11.0
GIF Read Support => enabled
GIF Create Support => enabled
JPEG Support => enabled
libJPEG Version => 8
PNG Support => enabled
libPNG Version => 1.6.37
WBMP Support => enabled
XPM Support => enabled
libXpm Version => 30411
XBM Support => enabled
WebP Support => enabled
BMP Support => enabled
AVIF Support => enabled
TGA Read Support => enabled

Directive => Local Value => Master Value
gd.jpeg_ignore_warning => 1 => 1

@mondrake re #41... thanks for adding that to alpine!

This AVIF support has been designed to work with libavif with version >=0.8.2. Though of course later is better.

I realize that we didn't require a particular version of libavif in PHP. Should change that.

@andypost #44: it's because we never added this to phpinfo() 🙄.

Looks like that would be a simple change to gd.c. When I get a minute, I can submit a PR.

Filed PR https://github.com/php/php-src/pull/7526 to simplify debug

FYI displaying of version string for libraries only possible when PHP is built with bundled GD

Support for getimagesize() just yet commited

Is this now [PP-2]?

I bet it depends on only one child issue

Not sure it should be blocked on tests refactoring

Avif is bundled in 8.1 or disabled as other formats are

Discussed with @catch and @andypost. This issue requires PHP 8.1, so cannot be backported to 9.5. That means we would not have branch parity between 10.0 and 9.5 for this functionality. However, if no deprecations are required to add support for it, it could theoretically be backported to 10.0.x if it is ready in time for the beta deadline. It can also go into 10.1.x.

Per @catch:

The worst that could happen is a module or profile including it in default config for an image style and breaking 9.4/9.5 compatibility.

Realistically it'll be hard for sites to use it until all the browsers support it. It's bad enough with webp on old Mac OS versions. But it's good to have it available.

Tagging for RM review so we can evaluate it one final time before committing to 10.0.x without 9.5.x.

In related issue I've explained how to add this support #3283449-17: Create a DrupalCI Environment for PHP 8.2

Moreover you can use alpinelinux php8/php81 packages which has support, also there's pre-build docker images we are using https://hub.docker.com/r/skilldlabs/php/tags/?page=1&name=81

Note that AVIF images are terribly slow to create, and are usually suitable to be generated offline. As Drupal generates image styles when the page loads, that could actually harm the visitor experience the first time.

Ideally, AVIF images could be generated when images are uploaded (I think in WordPress they are trying to do that).

JPEG XL format would be more suitable for online conversion, but unfortunately is still unsupported in all browsers.

@Pere Orga true, the creation is not that fast but there's enough solutions in contrib to pre-generate/warm images.
Moreover there's #2986669: Split ImageStyle into the config entity and a separate event-based image processing service

I've updated both the 8.1 and 8.2 containers to use debian bullseye now, so we should have a more modern version of avif compiled in.

Re-queued as phantomjs failed

A new testing environment called: "9.3.x-dev test with PHP 8.1 & MySQL 5.7 w/Bullseye Upgrades" has been added, which may be useful here.

@hestenet I did rebase but there's no way run new pipeline as issue using Merge Request

sorry https://www.drupal.org/pift-ci-job/2446376 after sometime it said MR is mergeable and allowed to start custom

Sadly tests fail even more

Drupal\KernelTests\Core\Image\ToolkitGdTest::testCreateImageFromScratch
with data set #4 (19)
    imageavif(): avif error - Could not encode image: No codec available

@andypost - ah bummer. Any guesses what else we might need to configure to get the codec in there properly?

Just wandering back in here for a minute, without having seen the details of your work - so if this comment is irrelevant, please ignore.

But did you build libavif with a codec? When I worked with it, I remember using aom.

It needs both aom-dev and dav1d-dev to build libavif to support encode and decode

In Alpinelinux we use https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/libavif/...

@mondrake I filed https://github.com/libgd/libgd/issues/782 and related PR to PHP but both needs work

Using the patch from https://github.com/php/php-src/pull/7526 I see following output

$ php --ri gd

gd

GD Support => enabled
GD Version => bundled (2.1.0 compatible)
....
WebP Support => enabled
BMP Support => enabled
AVIF Support => enabled
AVIF Version => 0.10.1
AVIF Codecs => dav1d [dec]:1.0.0, aom [enc/dec]:v3.4.0
TGA Read Support => enabled

EDIT I bet dav1d is optional as aom already provides decoder

8.2 CI image is ready now so requeued https://www.drupal.org/pift-ci-job/2488891

DrupalCI's PHP 8.1 and 8.2 containers have been recompiled, they now have both libavf and libaom:

• https://hub.docker.com/repository/docker/drupalci/php-8.1-apache
• https://hub.docker.com/repository/docker/drupalci/php-8.2-apache

Hopefully that does it, but let us know if it looks like anything else should be added to the container.

PHP should be built with bundled GD but it needs configuration --with-avif + see #41
https://git.alpinelinux.org/aports/tree/community/php81/APKBUILD#n266

The 'with-avif' flag is already set:

PHP 8.1:

• with-avif flag: https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...
• libavif: https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...
• libaom: https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...

PHP 8.2

• with-avif flag: https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...
• libavif: https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...
• libaom: https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...

Note: we generate dockerfiles using an M4 template: https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...

And finally here's the build log for the last run.

EDIT: I think this part of @andypost's comment may be what's missing: https://git.alpinelinux.org/aports/commit/?id=e4fba93fd9d83f520645469cae...

Filed MR to finally fix libavif #3283449-37: Create a DrupalCI Environment for PHP 8.2

TL'DR library needs suffix to be installed from backports

Requeued tests on the MR to see if changes from #3283449-44: Create a DrupalCI Environment for PHP 8.2 work..

Trying to save file when no encoder enabled will throw but it looks like weird way to test codec absence

The biggest question now is: do we want to support AVIF right now, when getting PHP setup right is non-trivial, and it's not possible to detect if all bits and pieces are correctly in place? gd_info() can tell us if AVIF is enabled, but that's not enough: if the codec is not installed, there is no way to understand it until an error occurs e.g. trying to save an AVIF image.

I think coming up with some form of automatic detection of support (+ really good documentation) is going to be crucial, given how tough it was for us to get all the pieces together even in our test environments.

Created follow-up #3325219: Remove workaround for PHP < 8.2 in GDToolkit when core require 8.2+

added the todo-link and rebased for current 10.1.x

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

it's RTBC IMO but needs summary update and CR, probably release note as well

Added a simple CR and RN snippet https://www.drupal.org/node/3348348

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased and applied suggestion for strict comparison

FYI, it seem there are problems with the diff.

$ git ac https://git.drupalcode.org/project/drupal/-/merge_requests/936.diff
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7846    0  7846    0     0  20701      0 --:--:-- --:--:-- --:--:-- 20701
error: missing binary patch data for 'core/tests/fixtures/files/img-test.avif'
error: binary patch does not apply to 'core/tests/fixtures/files/img-test.avif'
error: core/tests/fixtures/files/img-test.avif: patch does not apply

@quietone that seems like a gitlab issue. The .patch URL (instead of .diff) shows the binary content of the avif file.

@mstrelan, thanks. I forgot about the .patch version

I think we can add support independently, but both this and the existing webp support make #3213491: Add fallback format support to responsive images more important so that sites can actually use it. Adding as a related issue.

Does the GD toolkit already handle whether the webserver has support for AVIF - i.e. does it only show in the list of options if it's actually going to work?

The check was added in #3116611: Add a requirements check for GD support of allowed image types

but not every place in core using the filtered list of formats, probably there was an issue about add general requirements check

There's a set, hope it addresses #116 points

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

There's binary file in patch, so to apply patch it needs to download it and apply using git apply 936.patch or use patch -p1 -i 936.diff (it should be downloaded https://git.drupalcode.org/project/drupal/-/merge_requests/936.diff using .diff instead of .patch)

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thanks @mondrake I had read that earlier in the issue but had forgotten about it by now - I've added both of those to the issue summary. Also thanks to @andypost for opening them and working on them!

I wonder a bit if we should do some kind of kludgy support detection by catching the error in the meantime until those issues are fixed (or at least until AVIF is more widely available?), not sure where to put that though and if it's expensive we might have to cache the results or something.

Meantime animated AVIS added to latest Firefox https://www.mozilla.org/en-US/firefox/113.0/releasenotes/

Given the bleeding edge support (php 8.2) for AVIF, and complexity in it being compiled into PHP, it occurred to me that there should be an option to work around those limitations in contrib space. Alas, there is no conversion image style effect feature that I could find, so I opened up a feature request in what seemed like the most logical place to put it.

#3360072: Add an AVIF conversion image styles effect

Sadly that module doesn't seem like it has been getting a lot of love, use, or maintenance for a few years now.

Can MR be updated for 11.x

Also open thread mentions needing a follow up if that could be created also before the new branch is made.

Reading wrong commit forget the follow-up part.

Rebased

Changes look good and all tests green.

Rebased and sqashed commits into 2 for 11.x branch

Filed upgrade of PHP 8.3 CI image to latest Debian stable, AVIF is shipped now with libaom dependency fixed

https://git.drupalcode.org/project/drupalci_environments/-/merge_request...

Updated is with https://jakearchibald.com/2020/avif-has-landed/ which perfectly explains benefits

I think we can add to reports if no AVIF support available so people can read logs to get why imagecreatefromavif() or imageavif() are not found to solve #124

I think we can add to reports if no AVIF support available so people can read logs to get why imagecreatefromavif() or imageavif() are not found to solve #124

Unfortunately these functions still exist, even if avif is not supported. I did find the following code (found in https://bugs.php.net/bug.php?id=81217) would trigger a warning:

$ php -r 'var_dump(imageavif(imagecreatetruecolor(8, 8), "/tmp/test"));'
PHP Warning:  imageavif(): AVIF image support has been disabled
 in Command line code on line 1

I've tried to test for this in the status report using a pattern I found in \Drupal\Core\Render\ElementInfoManager::buildInfo. This works in my very limited test of two environments, one supporting AVIF, one not.

I needed that for AVIF to work with imagemagick, but probably that is needed for GD as well: #3362088: ExtensionMimeTypeGuesser doesn't support .avif

Not sure correct status, but #141 mentions change record updates. Which is technically apart of the review process.

Core's CI images are not yet using external GD https://git.drupalcode.org/project/drupalci_environments/-/blob/producti...

--with-external-gd Use external libgd

Not sure CR should elaborate how specific distros can install codec if it missing, it's unrelated to core

Brought up in #needs-review-queue-initative in slack. Was mentioned by @catch that

detection logic is still in progress

and that parts will need the release manager review.

Wonder if anyone has any update for the previous comment?

IMHO the CR should be expanded with some hints on how to fix the lack of the codec or of the binding of libavif.

I also don't know much about fixing this by compiling PHP, but I can add that the avif contrib module allows selection of different processors beyond just the GD option, including https://github.com/kornelski/cavif-rs (via the avif_lib add-on module). So maybe the CR could point people to the issue I linked above if they're not able to compile PHP with the codec support on their hosting environment. #3360072: Add an AVIF conversion image styles effect

The code here looks good to me.

The needs release manager review tag was added in 9.x era when we were discussing min PHP requirements.

I don't think we need it anymore - especially as we're into 11.x territory where 8.2+ will be available.

I think the remaining tasks here are to update the CR with instructions for folks on how to add avif support to their version of PHP

Tagging as NW for that.

Great work here folks

We are in Drupal 11.1-land now. Which will require PHP 8.3. I've updated the related CR to include libavif installation instructions. Back to NR.

Merged 11.x and removed PHP 8.2 related workaround

I simplified check to prevent checking avif writes if no readonly support

Moving to NW for open questions on MR.

All threads resolved and a green pipeline 🥳

LGTM. All concerns addressed.

Made a couple of suggestions on the issue.

Thank you, applied clean-up!

btw it could use follow-up to add UI to configure quality and speed for images in terms of #3372932: [Meta] High-performance images (nearly) out of the box

ref https://www.php.net/manual/function.imageavif.php

Added some more comments to the MR.

I see no reason to use cache except of static variable because this check should happen only once while PHP is running

Removed cache as this kind of checks should be cached statically, also removed useless logging of error

Thanks Alex! I did rebase on top of 11.x and reverted all unrelated changes

Believe feedback for this one has been addressed

Would be great to get this in 11.2

Added some info to the issue summary about browser support, which is now around 94%.

Committed 71c58d6 and pushed to 11.x. Thanks!

Fixed gitattributes on commit to make .avif files easier to manage - this resolves #113

diff --git a/.gitattributes b/.gitattributes
index e7b792f8407..f9e806c9a10 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -48,6 +48,7 @@ core/.phpstan-baseline.php text eol=lf whitespace=blank-at-eol,-blank-at-eof,-sp
 # Define binary file attributes.
 # - Do not treat them as text.
 # - Include binary diff in patches instead of "binary files differ."
+*.avif    -text diff
 *.eot     -text diff
 *.exe     -text diff
 *.gif     -text diff

This is more of a highlight than something that needs a release note.