Skip to content

Disable strict pixi requirement for libavif >= 0.9.1 #7253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Disable strict pixi requirement for libavif >= 0.9.1 #7253

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5889a4a
Disable strict pixi requirement for libavif >= 0.9.1
morsssss Jul 16, 2021
36698d2
Remove xfail from test on FreeBSD
morsssss Jul 20, 2021
89b277e
Use more compact AVIF_VERSION
morsssss Jul 26, 2021
d6226da
concatenate comment
morsssss Aug 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ext/gd/libgd/gd_avif.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ah, then the version check is fine for me.

// 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.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to link with libheif instead of libavif?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@andypost andypost Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andypost , please see the thread below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

decoder->strictFlags &= ~AVIF_STRICT_PIXI_REQUIRED;
#endif

io = createAvifIOFromCtx(ctx);
if (!io) {
gd_error("avif error - Could not allocate memory");
Expand Down