Re: Re: Finishing AVIF support in getimagesize()

On Thu, Dec 9, 2021 at 4:46 PM Nikita Popov <nikita.ppv@gmail.com> wrote: > On Wed, Dec 8, 2021 at 12:41 PM Christoph M. Becker <cmbecker69@gmx.de> > wrote: > > > On 01.12.2021 at 00:52, Ben Morss via internals wrote: > > > > > Earlier this year, I added support for AVIF images > > > <https://github.com/libgd/libgd/pull/671> to > > > libgd > > > <https://github.com/libgd/libgd>. My ultimate > > > goal was to bring > support > > for > > > this new image format to PHP, so that the world's top CMSs could let > > sites > > > serve AVIFs. A few of you kindly guided me as I made my first > > contributions > > > to the PHP codebase, propagating libgd's new AVIF support > > > <https://github.com/php/php-src/pull/7026> into > > > PHP's bundled gd fork, > > > and adding > > > AVIF awareness <https://github.com/php/php-src/pull/7091ÎÈ > > > óÊ&s¹}Ÿ~§Ï·> to non-gd > > > functions like getimagesize() and imagecreatefromstring(). > > > > > > Unfortunately, when I worked on getimagesize(), AVIF experts advised > that > > > there was no compact, reliable way to determine the size of an AVIF > image > > > without relying on an external library like libavif. We decided > > > > > > <https://github.com/php/php-src/pull/5127#issuecomment-799825277> > > > that > > it > > > was useful to return the information that a given image was an AVIF, > but > > > simply to return 0 for the actual dimensions and other details. The > user > > > would simply need to decode the image to determine this information. > > > > > > Of course, users would really like > > > > > > <https://github.com/php/php-src/pull/5127#issuecomment-976241397> > > > to > use > > > getimagesize() to return the actual size. So a colleague has kindly > > > created standalone > > > code > > > <https://aomedia-review.googlesource.com/c/libavifinfo/+/1483214X’­˜óÙ > > > žH¦hÀÂ@> > > (that > > > doesn't depend on libavif) to do this, with the goal of adding it to > PHP. > > > > > > The code comprises several hundred lines. But - it works, and it works > > well. > > > > > > Would it be ok to submit a PR containing this code to add this > > > functionality? Or does someone have a superior approach? > > > > Thanks for your and your colleague's work! It's highly appreciated. > > > > Anyhow, a respective PR[1] has been submitted now, and I'm in favor of > > bundling libavifinfo. I'm not too concerned regarding the additional > > size of the PHP binaries which would result by linking it in, but if > > others are, we could still introduce a configuration option (e.g. > > --with-libavifinfo). > > > > Thoughts? Objections to bundling libavifinfo at all? > > > > [1] <https://github.com/php/php-src/pull/7711> > > > > Assuming no licensing concerns, bundling libavifinfo sounds reasonable. The > library is small, our avif functionality is incomplete without it, and we > can't expect this to be available as a system library at this time. > > Regards, > Nikita > Although I'm not a core contributor, my 2c is that even with libavifinfo bundled, a configuration option is very welcome, the same way we have for png, jpeg, xpm and webp. As *cmb* said, thanks for your and your colleague's work! -- Atenciosamente, Flávio Heleno