ext/gd adding imagecompare. #14877

devnexen · 2024-07-08T17:46:49Z

can be useful to detect subtle differences between two images, size, transparent, background ... wise.

Girgias

Makes sense to me, just one question about the return value.

Girgias · 2024-07-08T20:59:11Z

ext/gd/gd.c

+	im1 = php_gd_libgdimageptr_from_zval_p(IM1);
+	im2 = php_gd_libgdimageptr_from_zval_p(IM2);
+
+	RETURN_LONG((zend_long)gdImageCompare(im1, im2));


Should this be normalized to -1/0/1 similarly to what ZEND_THREEWAY_COMPARE does?

It returns a mask.

A mask of what?

Those values added on the stub :

#define GD_CMP_IMAGE 1 #define GD_CMP_NUM_COLORS 2 #define GD_CMP_COLOR 4 #define GD_CMP_SIZE_X 8 #define GD_CMP_SIZE_Y 16 #define GD_CMP_TRANSPARENT 32 #define GD_CMP_BACKGROUND 64 #define GD_CMP_INTERLACE 128 #define GD_CMP_TRUECOLOR 256

So 0 => no difference or one or more of those values for each difference found.

thg2k · 2024-07-09T06:15:48Z

I thought about this many times, and I wondered why it was never exposed before being it a very useful function. Can we hold off on this merge until I do a bit of research? Just a couple of days. @devnexen @Girgias

devnexen · 2024-07-09T06:19:52Z

sure

thg2k · 2024-07-09T13:02:37Z

@devnexen
I searched the mailing list, bugs.php.net, github issues, and the git log, there is no mention of why imagecompare was never implemented. It seems like an overlook.

Nevertheless, what do you think about having it as bool retuning gdImageCompare(im1, im2) & GD_CMP_IMAGE? I have the feeling that comparing metadata is cumbersome and it clutters the global namespace with 9 new nearly useless globals. The metadata can be compared easily in userspace, the real deal is the pixel-by-pixel comparison that is super-slow in userspace.

Also it would be easier to document and much more clear for the users.

Should we ask on internals for this?

devnexen · 2024-07-09T13:33:43Z

I still think how and where the differences come from have value but yes we can always discuss it.

thg2k · 2024-07-10T10:26:00Z

I saw the discussion on internals, it's definitely taking an unexpected turn. I'll come back to it soon.

There is another problem.

The gdImageCompare function is buggy, as it completely disregards the alpha channel for truecolor images, which is a completely unexpected behavior, as demonstrated here:

$im1 = imagecreatetruecolor(1, 1);
imagealphablending($im1, false);
imagesetpixel($im1, 0, 0, imagecolorallocatealpha($im1, 255, 0, 0, 0));
// 00ff0000
printf("im1=%08x\n", imagecolorat($im1, 0, 0));

$im2 = imagecreatetruecolor(1, 1);
imagealphablending($im2, false);
imagesetpixel($im2, 0, 0, imagecolorallocatealpha($im2, 255, 0, 0, 1));
// 01ff0000
printf("im2=%08x\n", imagecolorat($im2, 0, 0));

$im3 = imagecreatetruecolor(1, 1);
imagealphablending($im3, false);
imagesetpixel($im3, 0, 0, imagecolorallocatealpha($im3, 254, 0, 0, 0));
// 00fe0000
printf("im3=%08x\n", imagecolorat($im3, 0, 0));

// 0 in my case, but code is identical in bundled
var_dump(GD_BUNDLED);

// broken case (0, should be 5)
var_dump(imagecompare($im1, $im2));

// control case (5, expected)
var_dump(imagecompare($im1, $im3));

Because of this major issue, my opinion is to not go forward with this. As an alternative, I propose a custom function fully implemented in php code (i.e. completely inside ext/gd/gd.c), of a new function imagematch($im1, $im2): bool that only does the pixel by pixel matching. If you like the idea, I'm happy to implement it.

This would provide the missing functionality (avoiding the slow userland pixel by pixel comparison) and avoid any discussion about adding new constants and signatures.

What do you think? Should I propose this on internals?

devnexen · 2024-07-10T11:43:00Z

You can always propose your idea on internals.

pierrejoye · 2024-07-10T13:55:09Z

@giovanni thank you for the PR :) Alpha is disabled as far as I remember. It was a function from long ago. In the tests suite, I added gdTestImageDiff, which is surely better, but it is not exposed yet as I was more looking for a visual diff implementation as well and never came back to it :) https://github.com/libgd/libgd/blob/83498bbe1781bda6d122e9e3a1705116728224fb/tests/gdtest/gdtest.c#L487 I would not recommend adding more inside php's gd as it makes the maintenance a pain (Remi and co will be happier).

…

On Wed, Jul 10, 2024, 6:43 PM David CARLIER ***@***.***> wrote: You can always propose your idea on internals. — Reply to this email directly, view it on GitHub <#14877 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACE6KGSLKQSVPEIANAZH2LZLUM6HAVCNFSM6AAAAABKRJPN7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRQGI4TSNJRHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

thg2k · 2024-07-11T08:53:08Z

@pierrejoye Nevertheless, I still don't like broken functionality, so opened an alternative PR with an hypothetical imagematch() function.

It's still a draft, I would like to add the parameters: x1,y1,x2,y2,width,height to allow for pixel-by-pixel checking two images portions, but I'll invest time into this only if there is a chance to get it included.

It would be really cool to also have an imagediff() and generates a sort of map of the differences, but that a whole different problem. Also, they are not mutually exclusive as imagediff() would take memory, while imagematch() does only the comparison without instantiating any extra memory, so using imagediff() to simply do an equality check would be a waste of resources.

pierrejoye · 2024-07-11T09:05:42Z

imagediff is exactly what the function I linked does. Also there are radically different needs, exact match, perceptual differences (same or different size from the same image), etc. The perceptual diff is a bit trickier but the others are implementdd there already. To fit an exact match should not be too hard to add :)

…

On Thu, Jul 11, 2024, 3:53 PM Giovanni Giacobbi ***@***.***> wrote: @pierrejoye <https://github.com/pierrejoye> Nevertheless, I still don't like broken functionality, so opened an alternative PR with an hypothetical imagematch() function. It's still a draft, I would like to add the parameters: x1,y1,x2,y2,width,height to allow for pixel-by-pixel checking two images portions, but I'll invest time into this only if there is a chance to get it included. It would be really cool to also have an imagediff() and generates a sort of map of the differences, but that a whole different problem. Also, they are not mutually exclusive as imagediff() would take memory, while imagematch() does only the comparison without instantiating any extra memory, so using imagediff() to simply do an equality check would be a waste of resources. — Reply to this email directly, view it on GitHub <#14877 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACE6KDD5J5R4WR3PAG3TYLZLZBYXAVCNFSM6AAAAABKRJPN7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSGM4DKMBSGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

devnexen · 2024-07-11T09:10:56Z

Definitively a topic to discuss over @Internals

pierrejoye · 2024-07-11T09:20:23Z

I will take some time to expose what is there and then see. If I understand correctly, the main goals are, for now: - exact match (dimension and content) - image difference (same as match but returns an image with the diff) is it correct?

…

On Thu, Jul 11, 2024, 4:11 PM David CARLIER ***@***.***> wrote: Definitively a topic to discuss over @Internals <https://github.com/Internals> — Reply to this email directly, view it on GitHub <#14877 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACE6KED3LVH44F2KC6M3JDZLZD3PAVCNFSM6AAAAABKRJPN7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSGQZDCMBUGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pierrejoye · 2024-07-11T09:50:45Z

I replied in your new thread on internals as it lacks the information and details discussed and shown here.

…

On Thu, Jul 11, 2024, 4:20 PM Pierre Joye ***@***.***> wrote: I will take some time to expose what is there and then see. If I understand correctly, the main goals are, for now: - exact match (dimension and content) - image difference (same as match but returns an image with the diff) is it correct? On Thu, Jul 11, 2024, 4:11 PM David CARLIER ***@***.***> wrote: > Definitively a topic to discuss over @Internals > <https://github.com/Internals> > > — > Reply to this email directly, view it on GitHub > <#14877 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AACE6KED3LVH44F2KC6M3JDZLZD3PAVCNFSM6AAAAABKRJPN7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSGQZDCMBUGI> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

devnexen · 2024-07-11T10:33:08Z

Thanks both you two to launch the discussion on the ML. I have made up my mind already about this but let's see what other people think.

cmb69 · 2024-12-31T17:40:11Z

I doubt that exposing gdImageCompare() is a good idea, for the already mentioned reasons.

In the tests suite, I added gdTestImageDiff, which is surely better, but it is not exposed yet as I was more looking for a visual diff implementation as well and never came back to it :)

I doubt that it is a good idea to expose gdTestImageDiff() either. That function is pretty opiniated regarding the diff (channel values are emphasized more or less); that might make sense for the libgd test suite, and as long as the function lives only there, changes can be made at will without breaking BC.

I'm not sure what to expose, though. libgd already support different effects and it might make sense to extend on that. However, even the effects that are already available can't be used by PHP because they are controlled by gdImageAlphaBlending(), but imagealphablending() only supports a bool (i.e. gdEffectReplace and gdEffectAlphaBlend).

Some "simple" image pixel comparison (what is not simple) could make much sense for userland testing purposes.

cmb69 · 2024-12-31T18:23:13Z

libgd already support different effects and it might make sense to extend on that.

POC

I've quickly hacked the following, using the relevant code of gdTestImageDiff() for gdLayerDiff:

 ext/gd/gd.c       |  2 +-
 ext/gd/libgd/gd.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ext/gd/libgd/gd.h |  2 ++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/ext/gd/gd.c b/ext/gd/gd.c
index 17bda3d65e..4d0dc954f7 100644
--- a/ext/gd/gd.c
+++ b/ext/gd/gd.c
@@ -896,7 +896,7 @@ PHP_FUNCTION(imagealphablending)
 
 	im = php_gd_libgdimageptr_from_zval_p(IM);
 
-	gdImageAlphaBlending(im, blend);
+	gdImageAlphaBlending(im, gdEffectDiff);
 
 	RETURN_TRUE;
 }
diff --git a/ext/gd/libgd/gd.c b/ext/gd/libgd/gd.c
index 44a90773ee..8277bc749e 100644
--- a/ext/gd/libgd/gd.c
+++ b/ext/gd/libgd/gd.c
@@ -786,6 +786,8 @@ void gdImageSetPixel (gdImagePtr im, int x, int y, int color)
 						case gdEffectMultiply :
 							im->tpixels[y][x] = gdLayerMultiply(im->tpixels[y][x], color);
 							break;
+						case gdEffectDiff :
+							im->tpixels[y][x] = gdLayerDiff(im->tpixels[y][x], color);
 					}
 				} else {
 					im->pixels[y][x] = color;
@@ -3097,6 +3099,60 @@ int gdLayerMultiply (int dst, int src)
 		);
 }
 
+int gdLayerDiff (int dst, int src)
+{
+	int r1,b1,g1,a1,r2,b2,g2,a2;
+	unsigned int diff_a,diff_r,diff_g,diff_b;
+
+	a1 = gdTrueColorGetAlpha(dst);
+	a2 = gdTrueColorGetAlpha(src);
+	diff_a = abs (a1 - a2);
+	diff_a *= 4;  /* emphasize */
+
+	if (diff_a) {
+		diff_a += 128; /* make sure it's visible */
+	}
+	if (diff_a > gdAlphaMax) {
+		diff_a = gdAlphaMax/2;
+	}
+
+	r1 = gdTrueColorGetRed(dst);
+	r2 = gdTrueColorGetRed(src);
+	diff_r = abs (r1 - r2);
+	// diff_r *= 4;  /* emphasize */
+	if (diff_r) {
+		diff_r += gdRedMax/2; /* make sure it's visible */
+	}
+	if (diff_r > 255) {
+		diff_r = 255;
+	}
+
+	g1 = gdTrueColorGetGreen(dst);
+	g2 = gdTrueColorGetGreen(src);
+	diff_g = abs (g1 - g2);
+
+	diff_g *= 4;  /* emphasize */
+	if (diff_g) {
+		diff_g += gdGreenMax/2; /* make sure it's visible */
+	}
+	if (diff_g > 255) {
+		diff_g = 255;
+	}
+
+	b1 = gdTrueColorGetBlue(dst);
+	b2 = gdTrueColorGetBlue(src);
+	diff_b = abs (b1 - b2);
+	diff_b *= 4;  /* emphasize */
+	if (diff_b) {
+		diff_b += gdBlueMax/2; /* make sure it's visible */
+	}
+	if (diff_b > 255) {
+		diff_b = 255;
+	}
+
+	return gdTrueColorAlpha(diff_r, diff_g, diff_b, diff_a);
+}
+
 void gdImageSetClip (gdImagePtr im, int x1, int y1, int x2, int y2)
 {
 	if (x1 < 0) {
diff --git a/ext/gd/libgd/gd.h b/ext/gd/libgd/gd.h
index 672ddf9483..478da55be4 100644
--- a/ext/gd/libgd/gd.h
+++ b/ext/gd/libgd/gd.h
@@ -91,6 +91,7 @@ extern "C" {
 #define gdEffectNormal 2
 #define gdEffectOverlay 3
 #define gdEffectMultiply 4
+#define gdEffectDiff 5
 
 #define GD_TRUE 1
 #define GD_FALSE 0
@@ -105,6 +106,7 @@ extern "C" {
 int gdAlphaBlend(int dest, int src);
 int gdLayerOverlay(int dst, int src);
 int gdLayerMultiply(int dest, int src);
+int gdLayerDiff(int dest, int src);
 
 /**
  * Group: Transform

Can be used like:

imagealphablending($im2, true);
imagecopy($im2, $im1, 0, 0, 0, 0, imagesx($im1), imagesy($im1));

PS: completely missed imagelayereffect(), so this can be used instead of a patched imagealphablending().

github-actions bot added the Extension: gd label Jul 8, 2024

ext/gd adding imagecompare.

aa4c9cb

can be useful to detect subtle differences between two images, size, transparent, background ... wise.

devnexen force-pushed the gd_imagecompare branch from 439c795 to aa4c9cb Compare July 8, 2024 18:29

devnexen marked this pull request as ready for review July 8, 2024 19:20

devnexen requested a review from kocsismate as a code owner July 8, 2024 19:20

Girgias approved these changes Jul 8, 2024

View reviewed changes

thg2k mentioned this pull request Jul 11, 2024

Add imagematch() function #14914

Open

cmb69 mentioned this pull request Dec 31, 2024

Implement slow GD test helpers in C #17305

Closed

2 tasks

cmb69 mentioned this pull request Jan 2, 2025

Introduce gdLayerDiff() and gdEffectDiff libgd/libgd#920

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext/gd adding imagecompare. #14877

ext/gd adding imagecompare. #14877

devnexen commented Jul 8, 2024

Girgias left a comment

Girgias Jul 8, 2024

devnexen Jul 8, 2024

Girgias Jul 8, 2024

devnexen Jul 8, 2024

devnexen Jul 8, 2024

thg2k commented Jul 9, 2024

devnexen commented Jul 9, 2024

thg2k commented Jul 9, 2024 •

edited

Loading

devnexen commented Jul 9, 2024

thg2k commented Jul 10, 2024

devnexen commented Jul 10, 2024

pierrejoye commented Jul 10, 2024 via email •

edited

Loading

thg2k commented Jul 11, 2024

pierrejoye commented Jul 11, 2024 via email

devnexen commented Jul 11, 2024

pierrejoye commented Jul 11, 2024 via email

pierrejoye commented Jul 11, 2024 via email

devnexen commented Jul 11, 2024

cmb69 commented Dec 31, 2024

cmb69 commented Dec 31, 2024 •

edited

Loading

ext/gd adding imagecompare. #14877

Are you sure you want to change the base?

ext/gd adding imagecompare. #14877

Conversation

devnexen commented Jul 8, 2024

Girgias left a comment

Choose a reason for hiding this comment

Girgias Jul 8, 2024

Choose a reason for hiding this comment

devnexen Jul 8, 2024

Choose a reason for hiding this comment

Girgias Jul 8, 2024

Choose a reason for hiding this comment

devnexen Jul 8, 2024

Choose a reason for hiding this comment

devnexen Jul 8, 2024

Choose a reason for hiding this comment

thg2k commented Jul 9, 2024

devnexen commented Jul 9, 2024

thg2k commented Jul 9, 2024 • edited Loading

devnexen commented Jul 9, 2024

thg2k commented Jul 10, 2024

devnexen commented Jul 10, 2024

pierrejoye commented Jul 10, 2024 via email • edited Loading

thg2k commented Jul 11, 2024

pierrejoye commented Jul 11, 2024 via email

devnexen commented Jul 11, 2024

pierrejoye commented Jul 11, 2024 via email

pierrejoye commented Jul 11, 2024 via email

devnexen commented Jul 11, 2024

cmb69 commented Dec 31, 2024

cmb69 commented Dec 31, 2024 • edited Loading

thg2k commented Jul 9, 2024 •

edited

Loading

pierrejoye commented Jul 10, 2024 via email •

edited

Loading

cmb69 commented Dec 31, 2024 •

edited

Loading