Everything

php

Propagating AVIF support from libgd into PHP's bundled gd

php.internals

Unnamed Person

5 years ago

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

Ayesh - PHP.Watch

5 years ago

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:

Kalle Sommer Nielsen

5 years ago

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

Ayesh - PHP.Watch

5 years ago

>
> 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.

Unnamed Person

5 years ago

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:

Mark Randall

5 years ago

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.

Nikita Popov

5 years ago

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

Remi Collet

5 years ago

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

Nikita Popov

5 years ago

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

Pierre Joye

5 years ago

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  ")