Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement #32803: Add animated GIF support #2024

Closed
wants to merge 1 commit into from

Conversation

7 participants
@cmb69
Copy link
Contributor

cmb69 commented Jul 23, 2016

We add PHP bindings for libgd's features to write animated GIFs, supporting
only GD context output via PHP streams (for now).

As PHP's bundled libgd doesn't yet include the respective features of the
external libgd, we add these. For external libgd builds we assume that at
least GD_2_0_29 (released 2006-04-05) is available without further checks.

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Jul 23, 2016

This is just a first shot – suggestions for improvement are welcome.

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Jul 23, 2016

Hi @cmb69 I love your recent attention to GD, it is great to see. It was not too long ago I looked into features missing from libgd in the PHP binding and this was one of them. I however find the API a little weird to directly mirror in PHP, initially I thought of an OO approach, but then again most of ext/gd is procedural, so it would create an inconsistency (side thing: I do think that in the long run that ext/gd could benefit from an OO approach to make the API more lucrative).

But then again I do not see any better approach myself currently, but again, great work!

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Jul 23, 2016

Hmm, one thing I do not like is that stream part of the API, maybe that could be solved by said OO approach (or by adding another resource type) and store the stream buffer there and return it on animation end? Don't get me wrong, I LOVE streams in PHP, but it just seems like its the only part in ext/gd that needs this on top of my vague memory. What are your thoughts @cmb69 ?

ext/gd/gd.c Outdated
@@ -821,6 +821,28 @@ ZEND_BEGIN_ARG_INFO(arginfo_imagesetinterpolation, 0)
ZEND_ARG_INFO(0, method)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_INFO(arginfo_imagegifanimbegin, 0)
ZEND_ARG_INFO(0, im)

This comment has been minimized.

Copy link
@staabm

staabm Jul 23, 2016

Contributor

Doesnt the 0 means optional arg but the comment at the function body declares this as reqired arg?

Same on other new args added

This comment has been minimized.

Copy link
@cmb69

cmb69 Jul 23, 2016

Author Contributor

The first parameter of ZEND_ARG_INFO() indicates whether the argument is passed by reference. Optional arguments would require to use ZEND_BEGIN_ARG_INFO_EX(), what I actually should have used. Thanks!

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Jul 23, 2016

@KalleZ Thanks!

I agree that the API takes some time getting used to, but at least currently ext/gd is just a thin wrapper over libgd, and the imagegifanim*() functions mirror closely gdImageGifAnim*(). While I would like to see some OO API for ext/gd generally, this is nothing I'd like to introduce only for a small subset of ext/gd.

I'm not too happy with the stream approach, either, but that was the best I could come up with for now, and it's not something new in GD, because the functions to store images (imagepng() etc.) also accept a stream as seccond parameter (still undocumented, AFAIK). These functions, however, do automatically close the stream, what appears to be a bit unexpected (and wouldn't work for imagegifanim*() anyway).

Adding a new resource type is something to consider. That would make it possible to pass a filename to imagegifanimbegin() (managing the stream completely behind the scenes), but it might not be optimal if a stream would be accepted (in which case I'd prefer the current API), but accepting a stream might not be necessary/sensible at all. Have to think about it.

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Jul 23, 2016

@cmb69 As for OO, I once wrote a userland library to convert most of ext/gd into some OO magic which made the code a lot nicer to work with, as you could make some things feel more natural. Although it was 10 years ago, it may be time to re-visit that idea.

Let me know what your thoughts are, including ideas for the stream part ^_^

(unrelated side note: Since you are adding support for some of the missing feature in the binding, maybe you could add support for the "Scatter" filter me and Takeshi authored, there is some bits and pieces left here, but I think the PHP binding patch was lost: http://grokbase.com/t/php/php-gd-bugs/097gmpq1g5/libgd-208-comment-added-scatter-filter)

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Jul 25, 2016

[…] maybe you could add support for the "Scatter" filter […]

Principally, yes. However, I don't know what this filter does exactly, and there's not much documentation available. Could you point me to some documentation, please?

Anyhow, it seems to me that it would be best to file an own FR for separation of concerns. :-)

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Jul 25, 2016

As for OO, I once wrote a userland library to convert most of ext/gd into some OO magic which made the code a lot nicer to work with, as you could make some things feel more natural. Although it was 10 years ago, it may be time to re-visit that idea.

Yes, please! A userland OO wrapper for GD appears to be a good POC to finally introduce a OO API in ext/gd – again, that would deserve its own FR. :-)

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Jul 25, 2016

@cmb69 I added you in my repo for KalleZ/gdobjects, we can take discussions to there ;-)

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 27, 2016

Unrelated question: Why do we have a bundled libgd? Is this necessary because we patch something? If not, can we unbundle it to avoid shipping outdated code?

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Jul 27, 2016

Why do we have a bundled libgd? Is this necessary because we patch something?

We have patched a lot of things in the past (long ago, while libgd wasn't really maintained anymore), and so both variants have diverged. We're working on synching them, though, and hopefully that might happen for PHP 7.1 or PHP 7.2. I guess we could unbundle libgd then. CC @pierrejoye

@pierrejoye

This comment has been minimized.

Copy link
Contributor

pierrejoye commented Jul 27, 2016

No we won't unbundle it.

There are numerous advantage for it. Memory management, always available,
etc.

Once the sync is fully done, maintain both will be less of a problem.

On Jul 27, 2016 9:34 PM, "Christoph M. Becker" notifications@github.com
wrote:

Why do we have a bundled libgd? Is this necessary because we patch
something?

We have patched a lot of things in the past (long ago, while libgd wasn't
really maintained anymore), and so both variants have diverged. We're
working on synching them, though, and hopefully that might happen for PHP
7.1 or PHP 7.2. I guess we could unbundle libgd then. CC @pierrejoye
https://github.com/pierrejoye


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2024 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARPKHufQXvjACXPdVcSikKTvPtcLnpnks5qZ2xegaJpZM4JTVFs
.

@pierrejoye

This comment has been minimized.

Copy link
Contributor

pierrejoye commented Jul 27, 2016

And the code is not outdated btw.

The real issue is not related to bundled libs or not but from a security pov. It is a pain lately in the core.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 27, 2016

I don't care about libgd in particular, I'm just concerned about the number of libraries we bundle without a strong reason. "Always available" seems like a bad argument for bundling a huge chunk of code with a continuous stream of security patches, given how distros do not use bundled libraries anyway and, consequently, 99% of PHP end-users do not either.

Doesn't the fact that we can end up with decade-large codebase divergences tell us that this bundling approach clearly does not work?

@pierrejoye

This comment has been minimized.

Copy link
Contributor

pierrejoye commented Jul 27, 2016

Constant stream of security issues?

We have some since the release of tools for fuzzy tests tools. Old code get
hardened. Most if not all recent issues are about invalid arguments and
local only (no rce).

The same applies to the dozen of flaws per release in php, except for 7,
which has more obviously.

On Jul 28, 2016 12:35 AM, "Nikita Popov" notifications@github.com wrote:

I don't care about libgd in particular, I'm just concerned about the
number of libraries we bundle without a strong reason. "Always available"
seems like a bad argument for bundling a huge chunk of code with a
continuous stream of security patches, given how distros do not use bundled
libraries anyway and, consequently, 99% of PHP end-users do not either.

Doesn't the fact that we can end up with decade-large codebase
divergences tell us that this bundling approach clearly does not work?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2024 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARPKJ3DSadUilx---lB5lTaEbXHtr5Iks5qZ5bTgaJpZM4JTVFs
.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 27, 2016

Err, sorry. I guess I'm not able to express myself in a way sufficiently diplomatic to make this discussion productive. So, nevermind, everybody carry on.

@pierrejoye

This comment has been minimized.

Copy link
Contributor

pierrejoye commented Jul 27, 2016

No worry :)

I am open to discuss but unbundle for the sake of unbundle something is not
very appealing to me.

On Jul 28, 2016 1:02 AM, "Nikita Popov" notifications@github.com wrote:

Err, sorry. I guess I'm not able to express myself in a way sufficiently
diplomatic to make this discussion productive. So, nevermind, everybody
carry on.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2024 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARPKHLJZAZ6NN_xRDiQxNSpankCuv2qks5qZ50lgaJpZM4JTVFs
.

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Jul 27, 2016

@nikic and @pierrejoye I sort of agree with both your statements, but I think that the main issue for libgd is that it seems to not be in sync, and despite a lot of work being put into it, it never seems to manage fully in sync.

@pierrejoye as for memory management, couldn't gd be upgraded with some finer hooks so that even the external could take the same advantages as the bundled does? Could even allow other programs that link to gd to do the same, maybe PHP can be considered a special user of libgd, but still there could be some abstraction to do so.

I for one would welcome a fully synced libgd at least, but always available thing is not really an issue, not on Windows anymore with all the libraries we have in our build tool chains nowadays, nor can I see it being an issue on Unix since you would have those libs installed anyway when you install the package for a certain extension. It is less a burden for us to having to upgrade libraries all the time (pcrelib, bcmath, gd, sqlite, ...) it mainly provides a better convenience for developers to quickly being able to build and test imo., that is unless we ofcourse bundle a library and modify it to work for PHP, like magic for ext/fileinfo.

All in all I hope that in 7.2, we could maybe look at extensions and their [bundled] libraries, which seems to have been out of focus since almost 5.4 when we finally bundled an opcode cache, get some light on extensions to move in and out of pecl, and whether or not to bundle libraries, and in those cases which would be better over all to include, like pcrelib since ext/pcre cannot be disabled. Again as for GD, I hope we can reach a fully synced version that is 100% compatible with the external libgd, which can take the same advantages as the bundled/semi patched one can do.

@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Jan 3, 2017

@cmb69 fix conflicts please :)

@pierrejoye I'm sure you came to review this and got sidetracked ... can I request that you okay this feature please ?

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Jan 3, 2017

as for memory management, couldn't gd be upgraded with some finer hooks so that even the external could take the same advantages as the bundled does?

FTR: I've filed libgd/libgd#335 a while ago.

... can I request that you okay this feature please?

Not so much the feature, but rather the API might deserve more general discussion, perhaps even an RFC (see the discussion with Kalle at the start of the thread).

@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Jan 3, 2017

@cmb69 an RFC sounds even better ... do that ;)

Implement #32803: Add animated GIF support
We add PHP bindings for libgd's features to write animated GIFs, supporting
only GD context output via PHP streams (for now).

As PHP's bundled libgd doesn't yet include the respective features of the
external libgd, we add these. For external libgd builds we assume that at
least GD_2_0_29 (released 2006-04-05) is available without further checks.

@cmb69 cmb69 force-pushed the cmb69:gif-anim branch from f9236f0 to 4fda961 Jan 3, 2017

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Jan 3, 2017

I've rebased onto current master, and I'll start the RFC process ASAP.

@cmb69

This comment has been minimized.

Copy link
Contributor Author

cmb69 commented Sep 13, 2017

I'll start the RFC process ASAP.

Sorry, missed that for 7.2, so I'm closing this PR for now.

@cmb69 cmb69 closed this Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.