Finishing AVIF support in getimagesize()

  116543
November 30, 2021 23:52 internals@lists.php.net ("Ben Morss via internals")
Hello everyone!

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> 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/+/148321> (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, and all best,

Ben
  116587
December 8, 2021 11:40 cmbecker69@gmx.de ("Christoph M. Becker")
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> 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/+/148321> (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> -- Christoph M. Becker
  116590
December 8, 2021 16:02 pierre.php@gmail.com (Pierre Joye)
Good evening,

On Wed, Dec 8, 2021, 6:40 PM Christoph M. Becker <cmbecker69@gmx.de> wrote:

> On 01.12.2021 at 00:52, Ben Morss via internals wrote: > > l > > 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> >
On a general principle, I understood that bundling libs should be avoided. I am not totally sure aviinfo is stable enough to be bundle either way. The format will be widely used in a near future so i would rather allow it if libavinfo js available rather than bundling it. best, Pierre
> > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > >
  116591
December 8, 2021 18:22 internals@lists.php.net ("Yannis Guyon via internals")
> > On a general principle, I understood that bundling libs should be avoided. >
libavifinfo was designed more as a copy-pastable snippet than a library. Unfortunately AVIF is complex and avifinfo.c is lengthier than expected, although I believe 700 lines are still reasonable and way below a library's scale. I am not totally sure aviinfo is stable enough to be bundle either way.
>
What would you call "stable enough"? There is a test <https://aomedia.googlesource.com/libavifinfo/+/refs/heads/main/tests/avifinfo_test.cc>, it is currently internally fuzzed <https://aomedia.googlesource.com/libavifinfo/+/refs/heads/main/tests/avifinfo_fuzz.cc> and was successfully run on more than 60k crawled AVIF images. I do not plan on modifying or testing it further until the AVIF format changes. The format will be widely used in a near future so i would rather allow it
> if libavinfo js available rather than bundling it.
What would be the pros and cons of using a javascript version of libavifinfo rather than the C one? Thanks, Yannis
  116592
December 8, 2021 18:40 cmbecker69@gmx.de ("Christoph M. Becker")
On 08.12.2021 at 19:22, Yannis Guyon via internals wrote:

>> On a general principle, I understood that bundling libs should be avoided. > > libavifinfo was designed more as a copy-pastable snippet than a library.
Thanks for clarifying.
> Unfortunately AVIF is complex and avifinfo.c is lengthier than expected, > although > I believe 700 lines are still reasonable and way below a library's scale.. > >> I am not totally sure aviinfo is stable enough to be bundle either way. > > What would you call "stable enough"? > There is a test > <https://aomedia.googlesource.com/libavifinfo/+/refs/heads/main/tests/avifinfo_test.cc>, > it is currently internally fuzzed > <https://aomedia.googlesource.com/libavifinfo/+/refs/heads/main/tests/avifinfo_fuzz.cc> > and > was successfully run on more > than 60k crawled AVIF images. I do not plan on modifying or testing it > further > until the AVIF format changes. > >> The format will be widely used in a near future so i would rather allow it >> if libavinfo js available rather than bundling it. > > What would be the pros and cons of using a javascript version of libavifinfo > rather than the C one?
I think that "js" is a typo, and should have actually been "is". :) Anyhow, that is the point. If there will be no (widely available) libavifinfo, it makes sense to either bundle it, and have the full getimagesize() functionality for AVIF, or to not bundle it, and have only the most basic getimagesize() support for AVIF (as it is now). Christoph
  116608
December 9, 2021 19:45 nikita.ppv@gmail.com (Nikita Popov)
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> 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/+/148321> > (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
  116609
December 9, 2021 19:59 flaviohbatista@gmail.com (=?UTF-8?Q?Fl=C3=A1vio_Heleno?=)
On Thu, Dec 9, 2021 at 4:46 PM Nikita Popov 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> 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/+/148321> > > (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
  116616
December 9, 2021 21:56 cmbecker69@gmx.de ("Christoph M. Becker")
On 09.12.2021 at 20:59, Flávio Heleno wrote:

> On Thu, Dec 9, 2021 at 4:46 PM Nikita Popov 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> 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/+/148321> >>> (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. >> 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.
Whether AVIF support should be included for ext/gd is already configurable. This is solely about the getimagesize(fromstring) and image_type_to_* functions which are part of ext/std. We do not have configuration options for these for the other image formats either. -- Christoph M. Becker
  116619
December 10, 2021 00:39 flaviohbatista@gmail.com (=?UTF-8?Q?Fl=C3=A1vio_Heleno?=)
On Thu, Dec 9, 2021, 18:56 Christoph M. Becker <cmbecker69@gmx.de> wrote:

> On 09.12.2021 at 20:59, Flávio Heleno wrote: > > > On Thu, Dec 9, 2021 at 4:46 PM Nikita Popov 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> 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/+/148321> > >>> (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. > >> 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. > > Whether AVIF support should be included for ext/gd is already > configurable. This is solely about the getimagesize(fromstring) and > image_type_to_* functions which are part of ext/std. We do not have > configuration options for these for the other image formats either. > > -- > Christoph M. Becker >
Got it! I was refering to the general avif support, glad to see that it's already there =)
>
  116615
December 9, 2021 21:30 internals@lists.php.net ("Ben Morss via internals")
Thanks for the discussion, everyone!

Fortunately there are no licensing concerns, as the author is making this
code available to be copied and pasted freely. And we've already got the
build flag for avif.

I'll be excited to see getimagesize() actually return the size for AVIF
images ☺️


On Thu, Dec 9, 2021 at 2:45 PM Nikita Popov 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> 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/+/148321> >> (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 >