Bug 21336 – ImageDiff tool improvements

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Pierre-Olivier Latour]

There are a couple of improvements that we would like to make to the ImageDiff tool:
- render images to RGBA8 bitmaps independently of platform endianness
- create image difference bitmap in reference image colorspace instead of device colorspace (which depends on the main display profile), so that no color matching happens

[Pierre-Olivier Latour]

Created attachment 24056 [details]
Patch

[Pierre-Olivier Latour]

Created attachment 24057 [details]
Patch v2

[Darin Adler]

Comment on attachment 24056 [details]
Patch

Clearing the review flag on this, assuming that the second version obsoletes the first one.

[Darin Adler]

Comment on attachment 24057 [details]
Patch v2

The ChangeLog isn't good. It only lists a bug number and doesn't mention what the change is. Also, it has "set EMAIL_ADDRESS environment variable" in it, rather than your email address.

+ /**
+  * Generates an RGBA8 bitmap in the reference image colorspace containing the differences between the 2 images
+  */

We would use "//" comments for this, not /* ones, and we don't do the "/**" thing ever.

-    RetainPtr<CGContextRef> context(AdoptCF, CGBitmapContextCreate(CFDataGetMutableBytePtr(data), CGImageGetWidth(testBitmap), CGImageGetHeight(testBitmap),
-        CGImageGetBitsPerComponent(testBitmap), CGImageGetBytesPerRow(testBitmap), colorSpace.get(), kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst));

+    CGContextRef context = CGBitmapContextCreate(CFDataGetMutableBytePtr(data), CGImageGetWidth(referenceImage), CGImageGetHeight(referenceImage), 8, rowBytes, CGImageGetColorSpace(referenceImage), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big);

It's not a good idea to change the use of RetainPtr here. It makes the patch bigger than it needs to be, for no real benefit.

But also, WebKit project coding style guide is to put the result of a "create" function directly into a RetainPtr rather than waiting -- this minimizes the chance of programming mistakes leading to memory leaks.

+    if (context == 0)
+        return 0;

WebKit project coding style is to say "if (!context)" in a case like this.

-
-    // NOTE: This may not be safe when switching between ENDIAN types
+    

Removing the comment is fine. But you should just change the whitespace back.

-    float totalPixels = static_cast<float>(pixelsHigh * pixelsWide);
-    return (differences * 100.f) / totalPixels;
+    return static_cast<float>(differences) / static_cast<float>(pixelsHigh * pixelsWide) * 100.f;

Why this change?

I don't understand when the diffBitmap can be 0. Would that be due to a colorspace problem perhaps? Is the output from the tool clear enough?

Overall, these changes look fine, assuming that the existing test results still work as-is. If they don't, then the patch should include both the changes to ImageDiff *and* the other changes needed to keep tests passing, not just the ImageDiff part.

review- because of the ChangeLog issues at least. Please consider my other comments as well.

[Darin Adler]

I forgot to say: Seems like a great thing to do! I hope our pixel tests can become a more reliable tool for regression testing.

[Pierre-Olivier Latour]

(In reply to comment #4)
> (From update of attachment 24057 [details] [edit])
> The ChangeLog isn't good. It only lists a bug number and doesn't mention what
> the change is. Also, it has "set EMAIL_ADDRESS environment variable" in it,
> rather than your email address.

Which env var am I supposed to set? The instructions on http://webkit.org/coding/contributing.html do not mention anything.

> + /**
> +  * Generates an RGBA8 bitmap in the reference image colorspace containing the
> differences between the 2 images
> +  */
> 
> We would use "//" comments for this, not /* ones, and we don't do the "/**"
> thing ever.

I was just following the comment pattern of the already existing function below. I'll fix both.

> -    RetainPtr<CGContextRef> context(AdoptCF,
> CGBitmapContextCreate(CFDataGetMutableBytePtr(data),
> CGImageGetWidth(testBitmap), CGImageGetHeight(testBitmap),
> -        CGImageGetBitsPerComponent(testBitmap),
> CGImageGetBytesPerRow(testBitmap), colorSpace.get(), kCGBitmapByteOrder32Little
> | kCGImageAlphaPremultipliedFirst));
> 
> +    CGContextRef context =
> CGBitmapContextCreate(CFDataGetMutableBytePtr(data),
> CGImageGetWidth(referenceImage), CGImageGetHeight(referenceImage), 8, rowBytes,
> CGImageGetColorSpace(referenceImage), kCGImageAlphaPremultipliedLast |
> kCGBitmapByteOrder32Big);
> 
> It's not a good idea to change the use of RetainPtr here. It makes the patch
> bigger than it needs to be, for no real benefit.
> 
> But also, WebKit project coding style guide is to put the result of a "create"
> function directly into a RetainPtr rather than waiting -- this minimizes the
> chance of programming mistakes leading to memory leaks.

Fixed

> +    if (context == 0)
> +        return 0;
> 
> WebKit project coding style is to say "if (!context)" in a case like this.

Fixed

> -
> -    // NOTE: This may not be safe when switching between ENDIAN types
> +    
> 
> Removing the comment is fine. But you should just change the whitespace back.

Fixed

> -    float totalPixels = static_cast<float>(pixelsHigh * pixelsWide);
> -    return (differences * 100.f) / totalPixels;
> +    return static_cast<float>(differences) / static_cast<float>(pixelsHigh *
> pixelsWide) * 100.f;
> 
> Why this change?

Since "(pixelsWide*pixelsHigh)" which is size_t is explicitly typecasted to float, I also added the explicit typecast for "difference" which is unsigned. It doesn't really matter since the compiler will see "100.f" and should promote everything to float anyway. That was a minor cosmetic thing with no real reason, so I reverted the change.

> I don't understand when the diffBitmap can be 0. Would that be due to a
> colorspace problem perhaps? Is the output from the tool clear enough?

I think it's better to be very conservative when it comes to pixel comparison i.e. make sure you compare 2 images in the same "referential" i.e. colorspace, dimensions, etc... and reject completely otherwise (in this case, return 0). You don't want the test to pass because CG has done some black magic behind you to fix things. For instance, say images are later generated with the wrong colorspace; in the previous code, CG would have colormatch to correct anyway and there was a chance the test would have passed (especially if -threshold wasn't 0).

In theory it's never supposed to happen since everything is supposed to remain consistent (all pixel tests done in Generic RGB, etc...). Because of this, I didn't add a bunch of printf() to stderr.

> Overall, these changes look fine, assuming that the existing test results still
> work as-is. If they don't, then the patch should include both the changes to
> ImageDiff *and* the other changes needed to keep tests passing, not just the
> ImageDiff part.

This change is only affecting the ImageDiff tool and not the test results per-se (DRT is not affected), only their interpretation (i.e. the diff image you can see in Safari), which should be more accurate after this change. I'm not sure what you mean? Are you talking about the other bug we opened (#21322)?

[Pierre-Olivier Latour]

Created attachment 24070 [details]
patch v3

[Darin Adler]

(In reply to comment #6)
> > Also, it has "set EMAIL_ADDRESS environment variable" in it,
> > rather than your email address.
> 
> Which env var am I supposed to set?

EMAIL_ADDRESS

Or you can edit it by hand. Either way is fine.

> The instructions on
> http://webkit.org/coding/contributing.html do not mention anything.

We should fix that. A patch would be welcome :-)

> In theory it's never supposed to happen since everything is supposed to remain
> consistent (all pixel tests done in Generic RGB, etc...). Because of this, I
> didn't add a bunch of printf() to stderr.

I don't understand the logic here. If it happens, we need to know, right? So don't we need at least some printf?

> This change is only affecting the ImageDiff tool and not the test results
> per-se (DRT is not affected), only their interpretation (i.e. the diff image
> you can see in Safari), which should be more accurate after this change.

OK, good.

[Darin Adler]

Comment on attachment 24070 [details]
patch v3

r=me

[Pierre-Olivier Latour]

(In reply to comment #8)
> > In theory it's never supposed to happen since everything is supposed to remain
> > consistent (all pixel tests done in Generic RGB, etc...). Because of this, I
> > didn't add a bunch of printf() to stderr.
> 
> I don't understand the logic here. If it happens, we need to know, right? So
> don't we need at least some printf?

Let me rephrase: if it happens, you will know as you will have a 100% failure. You will need to look at the images in Preview to realize dimensions or colorspace don't match. But I added some printf anyway :)

[Pierre-Olivier Latour]

Created attachment 24071 [details]
Patch v4

[Darin Adler]

Comment on attachment 24071 [details]
Patch v4

I'm not certain the stderr output is a good idea -- it depends on what happens when run-webkit-tests sees this -- but it probably is an improvement.

r=me again

[Pierre-Olivier Latour]

(In reply to comment #12)
> I'm not certain the stderr output is a good idea -- it depends on what happens
> when run-webkit-tests sees this -- but it probably is an improvement.

If I understand things right (but I'm no perl expert), run-webkit-tests monitors stdout from the tool, not stderr, so this wouldn't break the script.

Note that DRT uses the same approach: results through stdout and error messages through stderr.

[Simon Fraser (smfr)]

	M	WebKitTools/DumpRenderTree/cg/ImageDiffCG.cpp
	M	WebKitTools/ChangeLog
r37270 = 6bab0914f54d8a30e0282dfad15414184b0f081a (trunk)