Finish AVIF support in getimagesize() #7711
Conversation
See #80828 and the internals@ mailing list discussion at https://externals.io/message/116543 Use libavifinfo's AvifInfoGetFeaturesStream() in php_handle_avif() to get the width, height, bit depth and channel count from an AVIF payload. Implement stream reading/skipping functions and data struct. Use libavifinfo's AvifInfoIdentifyStream() in php_is_image_avif(). Update the expected features read from "test1pix.avif" in getimagesize.phpt.
There was a problem hiding this comment.
Thank you for the PR! I'm generally in favor of bundling libavifinfo (at least as long as it is not widely available), and I'm not really concerned about the size. Others may object, though, and we may consider to make its inclusion optional (--with-libavifinfo). Let's see.
Anyhow, please fix the Windows build (comment below), and consider to restore the old format of config.m4 to avoid potential merge conflicts; having an overlong line there appears not to be a problem). I also wonder why the directory name is image_avif instead of libavifinfo; I'd prefer the latter.
Fix Windows build. Revert config.m4 formatting to the previous one.
|
Thanks for the quick review.
I do not plan to release libavifinfo binaries for now. It is meant to be built by users or copy-pasted.
I used the name |
|
Thanks for the update!
I wasn't referring to binaries, but rather a stand-alone library which may be distributed by third-parties (e.g. Linux distro managers). Having it as system library appears to be beneficial. But anyway, keeping it bundled shouldn't be a problem either (we do that with some other libraries as well). |
|
The AppVeyor failure looks unrelated; the Azure Pipeline failures definitely are. |
|
How can I relaunch the checks? Should I rebase? |
No need to. I re-triggered the AppVeyor build just now; the Azure builds are already failing for ~2 days, so no need to re-run them until the issue is resolved. |
|
++ to add it to standard codebase as distro-builders can vary it as GD |
|
If this gets merged, there needs to be an entry added in README.REDIST.BINS regarding the libavifinfo license. |
Move license text from README.md to original files LICENSE and PATENTS.
Done. |
|
Feel free to ask here if there is anything else I can do to help move this forward. |
There was a problem hiding this comment.
According to https://externals.io/message/116543#116608, I think we should merge this, but please have a look at my CS nits.
|
Thanks for this! I am excited to run (and finish) Serendipity Styx with it. |
|
@ophian, you can checkout/download the latest HEAD of php-src, and build yourself. |
|
well yes... ;-) But I cannot say this my users though! |
Okay, I see. They will have to wait for PHP 8.2.0, which is supposed to be released at the end of 2022. :| |
Currently getimagesize() identifies AVIF files but does not fill the image properties.
This patch fixes this issue by copy-pasting libavifinfo's 700-line C source and header, called from getimagesize().
libavifinfo parses a stream quickly and partially, without dynamic memory allocation.
See the bug #80828, the internals@ mailing list discussion #116543 and the GitHub discussion #5127.
The total bin folder size goes from 117950251 bytes to 118036716, which represents an increase of 0.0733%.
Tests pass. Compilation was only tried with default and enable-debug flags on Linux for now.