Propagating AVIF support from libgd into PHP's bundled gd

  114728
June 4, 2021 14:36 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. 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
  114745
June 5, 2021 21:08 ayesh@php.watch (Ayesh Karunaratne)
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
  114748
June 6, 2021 01:41 kalle@php.net (Kalle Sommer Nielsen)
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
  114749
June 6, 2021 10:07 ayesh@php.watch (Ayesh Karunaratne)
> > 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.
  114750
June 6, 2021 13:00 internals@lists.php.net ("Ben Morss via internals")
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.

On Sun, Jun 6, 2021 at 6:08 AM Ayesh Karunaratne <ayesh@php.watch> wrote:

> > > > 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. >
  114752
June 6, 2021 13:30 marandall@php.net (Mark Randall)
On 06/06/2021 14:00, Ben Morss via internals wrote:
> 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.
  114755
June 7, 2021 07:41 nikita.ppv@gmail.com (Nikita Popov)
On Sat, Jun 5, 2021 at 11:09 PM Ayesh Karunaratne <ayesh@php.watch> wrote:

> 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
  114756
June 7, 2021 08:00 remi@php.net (Remi Collet)
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
  114758
June 7, 2021 08:05 nikita.ppv@gmail.com (Nikita Popov)
On Mon, Jun 7, 2021 at 10:01 AM Remi Collet <remi@php.net> wrote:

> 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
  114761
June 7, 2021 09:10 pierre.php@gmail.com (Pierre Joye)
Hj Remi! :)

On Mon, Jun 7, 2021, 3:01 PM Remi Collet <remi@php.net> wrote:

> 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > >