Finishing AVIF support in getimagesize()

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

On Wed, Dec 8, 2021 at 12:41 PM Christoph M. Becker cmbecker69@gmx.de
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

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 Wed, Dec 8, 2021 at 12:41 PM Christoph M. Becker cmbecker69@gmx.de
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