Disable strict pixi requirement for libavif >= 0.9.1 #7253

morsssss · 2021-07-16T21:30:05Z

Some AVIF image generators didn't include the PixelInformationProperty (pixi), even though strictly speaking they should. In v0.9.2, libavif began requiring this. Let's disable it so we can read those images too.

morsssss · 2021-07-16T22:25:10Z

This fixes the test that began to fail in #7091 in FreeBSD, when FreeBSD updated to libavif 0.9.2.

The inestimable @wantehchang pointed out that libavif, in versions >= 0.9.2, strictly requires the PixelInformationProperty (pixi), as this is mandated by the MIAF standard. In particular, libheif v <= 1.11.0 doesn't add pixi.

One of my test images, which I acquired from a public repo of AVIF images, I chose because it didn't have AVIF as a major brand, but only among the compatible brands. This test image didn't have the pixi property.

In order to let PHP deal with as many sorts of AVIF images as possible, we're disabling this check. I'll plan to propose the same change in libgd. In other words, I don't see an advantage to rejecting images that may have been created by a relatively popular library, even they lack an attribute that MIAF requires.

morsssss · 2021-07-16T22:25:42Z

/cc @nikic , @cmb69

cmb69

Thank you! This looks basically good to me, but I think this should go into libgd first (at least we should have the same implementation).

cmb69 · 2021-07-16T22:39:02Z

ext/gd/libgd/gd_avif.c

@@ -342,6 +342,14 @@ gdImagePtr gdImageCreateFromAvifCtx (gdIOCtx *ctx)

 	decoder = avifDecoderCreate();

+	// Check if libavif version is >= 0.9.1
+#if AVIF_VERSION_MAJOR > 0 || (AVIF_VERSION_MAJOR == 0 && AVIF_VERSION_MINOR > 9) || (AVIF_VERSION_MAJOR == 0 && AVIF_VERSION_MINOR == 9 && AVIF_VERSION_PATCH >= 1)


Is this version based conditional compilation necessary? Can't we just always exclude this flag? If AVIF_STRICT_PIXI_REQUIRED is only available as of 0.9.2, we could check whether it is defined instead of checking the version.

Hi Christoph,

AVIF_STRICT_PIXI_REQUIRED is defined as an enumerator, not as a macro. So we can't say

#if defined(AVIF_STRICT_PIXI_REQUIRED)

The version check could be simplified if necessary by packing major, minor, and patch into an integer and compare the integers. For example,

#define AVIF_VERSION_0_9_1 ((0 * 1000000) + (9 * 10000) + (1 * 100) + 0) #if AVIF_VERSION >= AVIF_VERSION_0_9_1

This assumes the way AVIF_VERSION is defined in <avif/avif.h> will never change:

#define AVIF_VERSION \ ((AVIF_VERSION_MAJOR * 1000000) + (AVIF_VERSION_MINOR * 10000) + (AVIF_VERSION_PATCH * 100) + AVIF_VERSION_DEVEL)

AVIF_STRICT_PIXI_REQUIRED is defined as an enumerator, not as a macro.

Ah, then the version check is fine for me.

wantehchang · 2021-07-16T23:01:00Z

Ben wrote:

@wantehchang pointed out that libavif, in versions >= 0.9.2, strictly requires the PixelInformationProperty (pixi), ...

Correction: That should be versions >= 0.9.1, not 0.9.2.

morsssss · 2021-07-17T16:43:49Z

@cmb69 , yes, certainly, I'll submit the same PR for libgd! I just wanted to get it in here since I know we're approaching deadlines for PHP 8.1.

I'll start that libgd PR on Monday.

cmb69 · 2021-07-18T10:57:48Z

This kind of change is not subject to feature freeze, so no need to worry. :)

nikic · 2021-07-19T12:29:02Z

Could you please also drop the xfail from ext/gd/tests/imagecreatefromstring_avif.phpt to check that the test now passes?

@cmb69 Why is it important that this is changed in libgd first?

cmb69 · 2021-07-19T12:34:25Z

Why is it important that this is changed in libgd first?

It is not really important, but if libgd maintainers prefer another approach (or even reject for whatever reason), we had more differences in external/bundled (unless we'd sync again).

morsssss · 2021-07-20T01:31:21Z

Could you please also drop the xfail from ext/gd/tests/imagecreatefromstring_avif.phpt to check that the test now passes?

Ah, that's how you skipped the test on FreeBSD. Removed, running tests again.

morsssss · 2021-07-20T02:28:01Z

FreeBSD is passing. I'm not sure why Travis is failing, especially as it's skipping the AVIF tests.

nikic · 2021-07-20T09:52:47Z

@morsssss The intermittent Travis failure should be fixed by 977c3d1.

morsssss · 2021-07-20T17:11:41Z

@morsssss The intermittent Travis failure should be fixed by 977c3d1.

Ok! lmk if you want me to rebase on master, or if we can accept this PR as is.

andypost · 2021-07-21T01:11:04Z

ext/gd/libgd/gd_avif.c

+#if AVIF_VERSION_MAJOR > 0 || (AVIF_VERSION_MAJOR == 0 && AVIF_VERSION_MINOR > 9) || (AVIF_VERSION_MAJOR == 0 && AVIF_VERSION_MINOR == 9 && AVIF_VERSION_PATCH >= 1)
+	// If so, allow the PixelInformationProperty ('pixi') to be missing in AV1 image
+	// items. libheif v1.11.0 or older does not add the 'pixi' item property to
+	// AV1 image items. (This issue has been corrected in libheif v1.12.0.)


Is it possible to link with libheif instead of libavif?

got a test failure when libavif is build without libaom https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/23479

Is it due to just some minor variation of pixels? In that case we probably should use similarity.inc for the comparison.

Did you use a different encoder than aom? If so, which did you use?

For libgd, we decided to use libaom.

About libheif - this library can certainly create AVIF images as well! For various reasons, we decided to use libavif in gd. So I ultimately optimized my logic for libavif. To use libheif instead, you'd need to rewrite gd_avif.c.

@morsssss thank you! I used to build libavif with aom and applied your patch - tests now passed on 0.9.2

Both the PR and bb9ef2b still shows failures on arm (guess it caused by 32-bits)

This test passes on 32bit Windows with libavif 0.9.0; so either it's only an issue with newer libavif, or not particularly 32bit related at all.

Still fails on armv7 and armhf using beta2 (libavif 0.9.2)

TEST 6120/15019 [ext/gd/tests/imagecreatefromstring_avif.phpt] ========DIFF======== 002+ Frame size limit reduced from 268435456 to 67108864. int(250) int(375) 006+ Frame size limit reduced from 268435456 to 67108864. int(480) int(270) ========DONE======== FAIL imagecreatefromstring() - AVIF format [ext/gd/tests/imagecreatefromstring_avif.phpt] TEST 5938/15019 [ext/gd/tests/avif_decode_encode.phpt] ========DIFF======== 001+ Decoding AVIF image: Frame size limit reduced from 268435456 to 67108864. 002+ Frame size limit reduced from 268435456 to 67108864. 003+ ok 001- Decoding AVIF image: ok Default AVIF encoding: ok Encoding AVIF at quality 70: ok Encoding AVIF at quality 70 with speed 5: ok -- Encoding AVIF with illegal quality: ok Encoding AVIF with illegal speed: ok Encoding AVIF losslessly... ok 012+ Frame size limit reduced from 268435456 to 67108864. How many pixels are different in the two images? 0 ========DONE======== FAIL avif decoding/encoding tests [ext/gd/tests/avif_decode_encode.phpt]

@andypost , please see the thread below.

@morsssss FYI using patch to build libavif AOMediaCodec/libavif@1556f21 allowed to pass tests)

morsssss · 2021-07-22T16:55:22Z

@andypost , the tests on your setup are producing output that I don't usually see. It looks like the differences are there.

For imagecreatefromstring_avif.phpt, we usually see:

Reading image whose major brand is 'avif':
int(250)
int(375)
Reading image with a compatible brand that's 'avif':
int(480)
int(270)

I think you see:

�[1;32m002+ Frame size limit reduced from 268435456 to 67108864.�[0m
     int(250)
     int(375)
�[1;32m006+ Frame size limit reduced from 268435456 to 67108864.�[0m
     int(480)
     int(270)

So, something is outputting that extra information about the frame size limit. This doesn't look like a problem with the code. It's just some additional output.

For avif_decode_encode.phpt:

It's similar. The only difference is that there are additional lines starting with that �[1;32m escape code.

Since these tests will fail if the output isn't exactly what's expected, you'd need to determine what was outputting those codes and those warnings. Regrettably I can't!

cmb69 · 2021-07-22T18:24:45Z

Is dav1d used on ARM? Then the output is likely from https://github.com/videolan/dav1d/blob/895cda326200ee95f8db13c90e739a47252681d3/src/lib.c#L162

morsssss · 2021-07-22T19:13:46Z

Bingo! Nice find ;)

I don't suppose there's a way for our tests to send those logging messages somewhere else?

If not, I don't know how they'll pass with dav1d.

andypost · 2021-07-23T00:47:42Z

Maybe it just needs smaller image for tests, I mean smaller frame size

cmb69 · 2021-07-23T09:22:06Z

This is not particularly an issue regarding tests, but a more general issue, similar to libgd/libgd#295. If libavif allows us to catch the messages, great! Otherwise it will get tricky, and we may require dav1d to be build without CONFIG_LOG (what might be bad, though).

And no, this is unlikely to be related to the image dimensions.

wantehchang · 2021-07-23T15:34:33Z

Thank you for the investigation. We can avoid this dav1d log message with the following change to libavif:

diff --git a/src/codec_dav1d.c b/src/codec_dav1d.c
index b063fb0..4af10ec 100644
--- a/src/codec_dav1d.c
+++ b/src/codec_dav1d.c
@@ -216,7 +216,7 @@ avifCodec * avifCodecCreateDav1d(void)
     dav1d_default_settings(&codec->internal->dav1dSettings);
 
     // Set a maximum frame size limit to avoid OOM'ing fuzzers.
-    codec->internal->dav1dSettings.frame_size_limit = AVIF_MAX_IMAGE_SIZE;
+    codec->internal->dav1dSettings.frame_size_limit = (sizeof(size_t) < 8) ? (8192 * 8192) : AVIF_MAX_IMAGE_SIZE;
 
     // Ensure that we only get the "highest spatial layer" as a single frame
     // for each input sample, instead of getting each spatial layer as its own

Could you please test this libavif patch? Thanks.

morsssss · 2021-07-23T21:39:00Z

Meanwhile, here's a parallel PR for libgd:

libgd/libgd#723

morsssss · 2021-07-26T23:08:07Z

That libavif patch looks very promising! Ironically it will only apply to newer versions of libavif, which means, I imagine, that tests on certain systems will still output this undesired log info.

This keeps us in sync with libgd. See libgd/libgd#723 .

morsssss · 2021-07-26T23:11:59Z

Updated to stay in sync with libgd. See libgd/libgd#723 .

wantehchang · 2021-07-27T01:33:54Z

That libavif patch looks very promising! Ironically it will only apply to newer versions of libavif, which means, I imagine, that tests on certain systems will still output this undesired log info.

Yes, that solution requires a newer version of libavif. The only other solution is to compile dav1d with -DCONFIG_LOG=0.

morsssss · 2021-07-27T13:39:21Z

That libavif patch looks very promising! Ironically it will only apply to newer versions of libavif, which means, I imagine, that tests on certain systems will still output this undesired log info.

Yes, that solution requires a newer version of libavif. The only other solution is to compile dav1d with -DCONFIG_LOG=0.

@andypost , I fear that's the only way you're going to have these tests pass with your current setup.

cmb69 · 2021-08-04T10:01:46Z

@morsssss, can you please align the comment with libgd.

morsssss · 2021-08-06T17:41:21Z

@morsssss, can you please align the comment with libgd.

You have an eagle eye!

I've aligned the comment placements. Is it ok to keep using this PR for this purpose?

cmb69 · 2021-08-06T17:58:34Z

Yes, sure. :) I'm waiting for CI, and will then merge if all is fine. (I'm not going to merge the .vscode IDE files, though; see #7326 on how to avoid committing these.)

cmb69 · 2021-08-06T18:25:52Z

Thank you!

Disable strict pixi requirement for libavif >= 0.9.1

5889a4a

Some AVIF image generators didn't include the PixelInformationProperty (pixi), even though strictly speaking they should. In v0.9.2, libavif began requiring this. Let's disable it so we can read those images too.

morsssss mentioned this pull request Jul 16, 2021

AVIF support for getimagesize() and imagecreatefromstring() #7091

Closed

cmb69 reviewed Jul 16, 2021

View reviewed changes

Remove xfail from test on FreeBSD

36698d2

andypost reviewed Jul 21, 2021

View reviewed changes

andypost mentioned this pull request Jul 21, 2021

Added suport for AVIF to getimagesize(). #5127

Closed

morsssss mentioned this pull request Jul 23, 2021

Disable strict pixi requirement for libavif >= 0.9.1 libgd/libgd#723

Merged

Use more compact AVIF_VERSION

89b277e

This keeps us in sync with libgd. See libgd/libgd#723 .

concatenate comment

d6226da

cmb69 closed this in e0e2e9a Aug 6, 2021

Disable strict pixi requirement for libavif >= 0.9.1 #7253

Disable strict pixi requirement for libavif >= 0.9.1 #7253

Uh oh!

Conversation

morsssss commented Jul 16, 2021

Uh oh!

morsssss commented Jul 16, 2021

Uh oh!

morsssss commented Jul 16, 2021

Uh oh!

cmb69 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wantehchang commented Jul 16, 2021

Uh oh!

morsssss commented Jul 17, 2021

Uh oh!

cmb69 commented Jul 18, 2021

Uh oh!

nikic commented Jul 19, 2021

Uh oh!

cmb69 commented Jul 19, 2021

Uh oh!

morsssss commented Jul 20, 2021

Uh oh!

morsssss commented Jul 20, 2021

Uh oh!

nikic commented Jul 20, 2021

Uh oh!

morsssss commented Jul 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andypost Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morsssss commented Jul 22, 2021

Uh oh!

cmb69 commented Jul 22, 2021

Uh oh!

morsssss commented Jul 22, 2021

Uh oh!

andypost commented Jul 23, 2021

Uh oh!

cmb69 commented Jul 23, 2021

Uh oh!

wantehchang commented Jul 23, 2021

Uh oh!

morsssss commented Jul 23, 2021

Uh oh!

morsssss commented Jul 26, 2021

Uh oh!

morsssss commented Jul 26, 2021

Uh oh!

wantehchang commented Jul 27, 2021

Uh oh!

morsssss commented Jul 27, 2021

andypost Aug 4, 2021 •

edited

Loading