Disable strict pixi requirement for libavif >= 0.9.1 #723
Conversation
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.
src/gd_avif.c
Outdated
| // 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.) | ||
|
|
||
| #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) |
There was a problem hiding this comment.
these type of compares are so hard to read, and so easy to get wrong
isn't AVIF_VERSION provided specifically to make checks easier ?
#if AVIF_VERSION >= 90100
...
#endif
There was a problem hiding this comment.
No; see php/php-src#7253 (comment) for alternatives (none of which appear really better to me).
There was a problem hiding this comment.
assuming you mean this part:
This assumes the way AVIF_VERSION is defined in <avif/avif.h> will never change:
i don't see the problem here. it wouldn't be a publicly exported symbol if people weren't expected to rely on it, and the header file explicitly says:
This should allow for downstream projects to do greater-than preprocessor checks on AVIF_VERSION.
so assuming the format of the define is fine.
if upstream disagrees, then they should fix/clarify the header, and probably provide a macro like AVIF_MAKE_VERSION so people could pass in a tuple like AVIF_VERSION >= AVIF_MAKE_VERSION(0, 9, 1).
There was a problem hiding this comment.
A tuple of this sort would certainly be convenient!
Paging @wantehchang to get his opinion.
There was a problem hiding this comment.
@vapier, ah, I misread the comment. Doing #if AVIF_VERSION >= 90100 is fine with me.
There was a problem hiding this comment.
I think the comment text quoted above actually is an explication of the start of the comment:
AVIF_VERSION_DEVEL should always be 0 for official releases / version tags, and non-zero during development of the next release.
Nonetheless, since AVIF_VERSION_DEVEL does give this far more compact and legible way to determine the precise version, and it seems reasonable to expect it will continue to exist in this form, I'm personally quite happy to rely on it. I've made the change.
| @@ -361,7 +361,7 @@ BGD_DECLARE(gdImagePtr) gdImageCreateFromAvifCtx (gdIOCtx *ctx) | |||
| // 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.) | |||
|
|
|||
| #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 AVIF_VERSION >= 90100 | |||
There was a problem hiding this comment.
The way major, minor, and patch versions are packed into AVIF_VERSION changed after libavif v0.8.3.
v0.8.3:
#define AVIF_VERSION (AVIF_VERSION_MAJOR * 10000) + (AVIF_VERSION_MINOR * 100) + AVIF_VERSION_PATCH
v0.8.4:
#define AVIF_VERSION
((AVIF_VERSION_MAJOR * 1000000) + (AVIF_VERSION_MINOR * 10000) + (AVIF_VERSION_PATCH * 100) + AVIF_VERSION_DEVEL)
This is unlikely to change again, and I believe the new test should still work:
v0.8.3: AVIF_VERSION = 803
v0.8.4: AVIF_VERSION = 80400
There was a problem hiding this comment.
I guess we're just lucky here. Which doesn't feel great. But I'm quite happy to be able to use the more legible form AVIF_VERSION affords us.
I'm going to push this into php/php-src#7253 as well.
This keeps us in sync with libgd. See libgd/libgd#723 .
|
I am fine with it Also as soon as 1.0 of the lib is released, let remove it. We should rely on what the library does without our own sauce, easier to maintain :) |
|
my computer died shortly after my last comment and i haven't gotten a chance to resurrect it, so i forgot about this PR :/ |
|
Thanks! And please accept my condolences on your machine's untimely passing. |
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.
See also php/php-src#7253 .