Some tests would be meaningful to have.Could you please add some?

Ok, tests added and the declarations are added too.

Is there anything else that needs amending?

Yep, the declaration is fine now and the function is available to users. But I've meant PHPT tests ;) GD is integrated into php, so it's own tests might be good but will not run for PHP. The compilation works so far on TS and NTS.

Are PHPT test's required? It will fallback to the existing ImageCopyMerge function if the opacity is not required, and it's still going to require external user verification to check the opacity levels are as expected.

Well i just see the userspace function using gdImageCopyMergeAlpha you've added, so there are of course some code paths for the new functionality you've added and for the older functionatily you're falling back to. A visual check is of course required, but once you see it's correct you can compare files MD5/SHA or whatever. Please look into the ext/gd/tests what how the other tests do that.

Is this ok?

I'd rather check image md5 to be sure it's the same you expect, just look what the other tests do )

Is this ok?

Not exactly, the md5 string should be in the EXPECT section, like

--FILE--
.... do stuff .....
echo $md5;
--EXPECT--
my_md5

The md5 sum is of course pre calculated earlier and hardcoded into the test. This way you ensure the output file is always the same. That's also what many tests in ext/gd/tests do.

Ok, I have changed to provide the 2 expected md5 sum

Whats the best way to get this patch included in the core?

Write a mail to Internals and if there are no further objections I don‘t see why this can’t be merged into at least the 5.5 branch.

Thanks. Done. How long is the wait for people to confirm if they have objections?

A week or so is a good timeframe.

Quick comment about checking result images, md5 does not work, and never will.

Use image comparisons for that and save the diff images so it can be analyzed.

About what is done in this patch, have you tried imagelayereffect? That's what it does unlinke imagecopy or imagecopymerge.

@pierrejoye, @mattclegg what's going on with this one, is it ok, not ok?

Not ok, imagecopymerge is supposed to emulate alpha, not to use alpha.

imagelayereffect and more generally imagecopy uses the alpha channel.

Further functions to easier use an external alpha channel are being worked on, but will 1st done upstream in libgd. Help welcome there too if you like to :)

So a new imagecopymerge function that does use alpha needs to be added to
libgd?

There is already one for that, see my previous comment about layer.
On Aug 5, 2013 9:09 AM, "mattclegg" notifications@github.com wrote:

So a new imagecopymerge function that does use alpha needs to be added to
libgd?

On Mon, Aug 5, 2013 at 7:49 AM, pierrejoye notifications@github.com
wrote:

imagelayereffect and more generally imagecopy uses the alpha channel.

Further functions to easier use an external alpha channel are being
worked
on, but will 1st done upstream in libgd. Help welcome there too if you
like
to :)

—
Reply to this email directly or view it on GitHub<
https://github.com/php/php-src/pull/211#issuecomment-22089570>
.

Matt Clegg

--Does anyone ever read these things anyway?
http://dancer.dev.mattclegg.com/examples/single_song_demo/

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/211#issuecomment-22090156
.

Comment on behalf of krakjoe at php.net:

Since the maintainer of this PR called it "not okay", and since it still uses techniques the maintainer of GD says won't work, or are not god enough, and since this PR is very old, I'm going to close the PR.

Anyone watching is free to open a clean PR, start a new discussion and move forward with the original idea.

maintainer of GD, I meant ...

Anyone watching is free to open a clean PR, start a new discussion and move forward with the original idea.

Whenever a feature would require changes to ext/gd/libgd/, it should not be filed against PHP, but rather against libgd. Otherwise we'll never be able to sync both libs.

@cmb69 can you tell me what the reasons are for bundling (and maintaining) our own copy of libgd ?

It is somewhat legacy. I wrote the original gd extension 20+ years ago. The code was rather different then, but the main technical reason which is actually still true today is that doing in-memory image manipulation in PHP should use the PHP memory manager and GD didn't have a way to hook in an alternative memory manager.

I see.

void *
gdMalloc (size_t size)
{
	return malloc (size);
}

void *
gdRealloc (void *ptr, size_t size)
{
	return realloc (ptr, size);
}

void *
gdReallocEx (void *ptr, size_t size)
{
	void *newPtr = gdRealloc (ptr, size);
	if (!newPtr && ptr)
		gdFree(ptr);
	return newPtr;
}

/*
  Function: gdFree

    Frees memory that has been allocated by libgd functions.

	Unless more specialized functions exists (for instance, <gdImageDestroy>),
	all memory that has been allocated by public libgd functions has to be
	freed by calling <gdFree>, and not by free(3), because libgd internally
	doesn't use alloc(3) and friends but rather its own allocation functions,
	which are, however, not publicly available.

  Parameters:

	ptr - Pointer to the memory space to free. If it is NULL, no operation is
		  performed.

  Returns:

	Nothing.
*/
BGD_DECLARE(void) gdFree (void *ptr)
{
	free (ptr);
}

It would seem rather trivial to patch libgd to use a custom allocator: Could we open an upstream PR to introduce that functionality and escape the burden of maintenance and bundling in some future release of PHP ?

@pierrejoye reading through docs, it would seem that gd used to use specialist allocators, not standard ones, so at one point it was probably easier to bundle libgd and patch it to work with our own allocator. That doesn't seem to be the case any longer, the easiest thing to do today would seem to be patching upstream. Also, libgd is widely available, maybe it wasn't so widely available 20 years ago, maybe "widely available" wasn't even a thing back then.

I'm not saying it's a huge burden, but a burden it is: When new stuff is pulled into libgd, we only want to worry about our layer, not merging in initial changes, and the unavoidable fixes that follow, and tracking all those fixes and changes. We just want to work on our layer, like we do for the vast majority of other extensions.

Could we open an upstream PR to introduce that functionality […]

Already did this: libgd/libgd#335.

can you tell me what the reasons are for bundling (and maintaining) our own copy of libgd ?

Besides the memory allocation issues, because both libs have run out of sync, and we should carefully merge them – there are improvements and fixes in either. Doing this sync is work in progress.