Propagating AVIF support from libgd into PHP's bundled gd

2 years ago by Ben Morss via internals — view source

unread

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. Recently, @Christoph M. Becker cmbecker69@gmx.de and Nikita
have kindly been guiding me as I created a PR to propagate libgd's AVIF
support into PHP's bundled gd fork. This would create two new PHP functions
for systems with gd enabled: imagecreatefromavif() and imageavif().
Additionally it would add AVIF awareness to imagetypes().

This PR https://github.com/php/php-src/pull/7026 now looks ready to go.
Christoph advised me to write to this list to ask folks to take a look, and
to see if there are any objections to merging it!

If the group approves the first PR, my plan is to finish another PR
https://github.com/php/php-src/pull/7091 to add AVIF support to the
non-gd image function getimagesize() . In the same PR, I plan to add
support to PHP's internal _php_image_type() function, which is used in
imagecreatefromstring().

Thanks for your help with all of this!

All best,

Ben Morss

2 years ago by Ayesh Karunaratne — view source

unread

Hi Ben,
Thank you for opening this PR and the discussion. With the wide
availability of AVIF/AV1 support in browsers, I think this will fit
nicely.

We have the Namespaces in Bundled Extensions RFC
(https://wiki.php.net/rfc/namespaces_in_bundled_extensions) passed, so
perhaps, the new functions are probably better in the Gd namespace?
This would mean the new functions would be \Gd\imagecreatefromavif
and \Gd\imageavif. They are inconsistent with the existing functions
of course, but I thought to mention it because it's a recent proposal
and I don't think we added new functions after that RFC. Some examples
are recently renamed PHP classes in IMAP, Pgsql, LDAP, and FTP
extensions to follow this new proposal.

Thank you.

Ayesh.

On Fri, Jun 4, 2021 at 8:06 PM Ben Morss via internals
internals@lists.php.net wrote:

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. Recently, @Christoph M. Becker cmbecker69@gmx.de and Nikita
have kindly been guiding me as I created a PR to propagate libgd's AVIF
support into PHP's bundled gd fork. This would create two new PHP functions
for systems with gd enabled: imagecreatefromavif() and imageavif().
Additionally it would add AVIF awareness to imagetypes().

This PR https://github.com/php/php-src/pull/7026 now looks ready to go.
Christoph advised me to write to this list to ask folks to take a look, and
to see if there are any objections to merging it!

If the group approves the first PR, my plan is to finish another PR
https://github.com/php/php-src/pull/7091 to add AVIF support to the
non-gd image function getimagesize() . In the same PR, I plan to add
support to PHP's internal _php_image_type() function, which is used in
imagecreatefromstring().

Thanks for your help with all of this!

All best,

Ben Morss

2 years ago by Kalle Sommer Nielsen — view source

unread

Den søn. 6. jun. 2021 kl. 00.09 skrev Ayesh Karunaratne ayesh@php.watch:

Hi Ben,
Thank you for opening this PR and the discussion. With the wide
availability of AVIF/AV1 support in browsers, I think this will fit
nicely.

We have the Namespaces in Bundled Extensions RFC
(https://wiki.php.net/rfc/namespaces_in_bundled_extensions) passed, so
perhaps, the new functions are probably better in the Gd namespace?
This would mean the new functions would be \Gd\imagecreatefromavif
and \Gd\imageavif. They are inconsistent with the existing functions
of course, but I thought to mention it because it's a recent proposal
and I don't think we added new functions after that RFC. Some examples
are recently renamed PHP classes in IMAP, Pgsql, LDAP, and FTP
extensions to follow this new proposal.

I don't think it makes much sense to do that for a single function,
because it makes the API cluttered, as why would I call
\Gd\imagecreatefromavif when everything else is in the global
namespace? That is a poor design, while I understand the intentions
behind it is good.

I think this needs to be well thoughtout in a topic on its own rather
than off topicing it here, so I will leave it at that.

Overall, +1 for adding this

regards,

Kalle Sommer Nielsen
kalle@php.net

2 years ago by Ayesh Karunaratne — view source

unread

Den søn. 6. jun. 2021 kl. 00.09 skrev Ayesh Karunaratne ayesh@php.watch:

Hi Ben,
Thank you for opening this PR and the discussion. With the wide
availability of AVIF/AV1 support in browsers, I think this will fit
nicely.

We have the Namespaces in Bundled Extensions RFC
(https://wiki.php.net/rfc/namespaces_in_bundled_extensions) passed, so
perhaps, the new functions are probably better in the Gd namespace?
This would mean the new functions would be \Gd\imagecreatefromavif
and \Gd\imageavif. They are inconsistent with the existing functions
of course, but I thought to mention it because it's a recent proposal
and I don't think we added new functions after that RFC. Some examples
are recently renamed PHP classes in IMAP, Pgsql, LDAP, and FTP
extensions to follow this new proposal.

I don't think it makes much sense to do that for a single function,
because it makes the API cluttered, as why would I call
\Gd\imagecreatefromavif when everything else is in the global
namespace? That is a poor design, while I understand the intentions
behind it is good.

I think this needs to be well thoughtout in a topic on its own rather
than off topicing it here, so I will leave it at that.

Overall, +1 for adding this

regards,

Kalle Sommer Nielsen
kalle@php.net

I also think going with imagecreatefromavif/imageavif is good for
consistency. I brought that RFC up because it was recent and this is
the first new function we are adding.

Thank you.

2 years ago by Ben Morss via internals — view source

unread

Thank you for these comments!

Perhaps it would make sense to, at some point, create a followup proposal
to namespace the entire gd extension - so that all gd functions would be in
the namespace? I'd leave it to others to determine whether this would be
desirable, or whether it wouldn't be worth making developers update
existing code.

Den søn. 6. jun. 2021 kl. 00.09 skrev Ayesh Karunaratne <ayesh@php.watch
:

Hi Ben,
Thank you for opening this PR and the discussion. With the wide
availability of AVIF/AV1 support in browsers, I think this will fit
nicely.

We have the Namespaces in Bundled Extensions RFC
(https://wiki.php.net/rfc/namespaces_in_bundled_extensions) passed, so
perhaps, the new functions are probably better in the Gd namespace?
This would mean the new functions would be \Gd\imagecreatefromavif
and \Gd\imageavif. They are inconsistent with the existing functions
of course, but I thought to mention it because it's a recent proposal
and I don't think we added new functions after that RFC. Some examples
are recently renamed PHP classes in IMAP, Pgsql, LDAP, and FTP
extensions to follow this new proposal.

I don't think it makes much sense to do that for a single function,
because it makes the API cluttered, as why would I call
\Gd\imagecreatefromavif when everything else is in the global
namespace? That is a poor design, while I understand the intentions
behind it is good.

I think this needs to be well thoughtout in a topic on its own rather
than off topicing it here, so I will leave it at that.

Overall, +1 for adding this

regards,

Kalle Sommer Nielsen
kalle@php.net

I also think going with imagecreatefromavif/imageavif is good for
consistency. I brought that RFC up because it was recent and this is
the first new function we are adding.

Thank you.

2 years ago by Mark Randall — view source

unread

Thank you for these comments!

Perhaps it would make sense to, at some point, create a followup proposal
to namespace the entire gd extension - so that all gd functions would be in
the namespace? I'd leave it to others to determine whether this would be
desirable, or whether it wouldn't be worth making developers update
existing code.

When we de-resourcified Gd we considered making the Gd class be useful
as something more than an storage type.

If doing a full remap it may be worth putting the functions into the
class itself.

2 years ago by Nikita Popov — view source

unread

Hi Ben,
Thank you for opening this PR and the discussion. With the wide
availability of AVIF/AV1 support in browsers, I think this will fit
nicely.

Yes, this looks like a reasonable addition. Effectively this is just a sync
with upstream libgd, and the exposed API follows existing conventions.

We have the Namespaces in Bundled Extensions RFC
(https://wiki.php.net/rfc/namespaces_in_bundled_extensions) passed, so
perhaps, the new functions are probably better in the Gd namespace?
This would mean the new functions would be \Gd\imagecreatefromavif
and \Gd\imageavif. They are inconsistent with the existing functions
of course, but I thought to mention it because it's a recent proposal
and I don't think we added new functions after that RFC. Some examples
are recently renamed PHP classes in IMAP, Pgsql, LDAP, and FTP
extensions to follow this new proposal.>

See
https://wiki.php.net/rfc/namespaces_in_bundled_extensions#existing_non-namespaces_symbols_and_consistency
:

When adding new symbols to existing extensions, it is more important to
be consistent with existing symbols than to follow the namespacing
guidelines.

Unless we introduce namespaced aliases for the gd extension first, these
functions should not be namespaced either. As Mark pointed out, in the case
of GD it might make more sense to move to an OO API rather than a new set
of namespaced functions. In any case, I don't think this is the place to
discuss this.

Regards,
Nikita

2 years ago by Remi Collet — view source

unread

Le 04/06/2021 à 16:36, Ben Morss via internals a écrit :

... I created a PR to propagate libgd's AVIF
support into PHP's bundled gd fork...

Does it really make sense to keep maintaining this fork ?

Having to maintain 2 (quite) different code and picking
piece from one to other seems a waste of energy.

I really think that we'll need to re-sync bundled gd
with upstream and only keep it as an alternative for
people without system gd (and perhaps even drop it later)

My 0.02cts

Remi

2 years ago by Nikita Popov — view source

unread

Le 04/06/2021 à 16:36, Ben Morss via internals a écrit :

... I created a PR to propagate libgd's AVIF
support into PHP's bundled gd fork...

Does it really make sense to keep maintaining this fork ?

Having to maintain 2 (quite) different code and picking
piece from one to other seems a waste of energy.

I really think that we'll need to re-sync bundled gd
with upstream and only keep it as an alternative for
people without system gd (and perhaps even drop it later)

Once https://github.com/libgd/libgd/pull/692 lands, the goal would indeed
be to switch away from using a gd fork to bundling an unpatched upstream gd
(and maybe unbundling in the future). This is not entirely easy as the
codebases have diverged, but custom allocators are the current technical
blocker.

Regards,
Nikita

2 years ago by Pierre Joye — view source

unread

Hj Remi! :)

Le 04/06/2021 à 16:36, Ben Morss via internals a écrit :

... I created a PR to propagate libgd's AVIF
support into PHP's bundled gd fork...

Does it really make sense to keep maintaining this fork ?

Having to maintain 2 (quite) different code and picking
piece from one to other seems a waste of energy.

I really think that we'll need to re-sync bundled gd
with upstream and only keep it as an alternative for
people without system gd (and perhaps even drop it later)

it is more or less the same.

If we want to drop the use of php malloc, could be easy then.

but this is not the topic of this discussion though ")

My 0.02cts

Remi

--

To unsubscribe, visit: https://www.php.net/unsub.php