Bug 21322 – DumpRenderTree pixel test improvements

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

There are a number of improvements that we would like to make in DRT, related to the pixel tests:
1. Clean up the view snapshot code
2. Avoid dumping the PNG image when running pixel tests if we can detect that the checksum is equal to the expected checksum (for performance)
3. Improve the code that sets/restores the screen color profile
4. Make the code more cross-platformy with std::string goodness

[Simon Fraser (smfr)]

Created attachment 24037 [details]
Patch

With this patch, clean pixel tests on fast/css when from 18.77 to 7.59s, so it's a big perf win.

[Simon Fraser (smfr)]

This patch includes the change in bug 21323.

[Pierre-Olivier Latour]

A few other minor things in the patch:
- rendering is always in premultiplied RGBA8 to avoid endian issues and guarantee consistent hash values
- ensure font smoothing is disabled (this is also called LCD anti-aliasing and is different from regular font CG anti-aliasing) as font-smoothing settings depends on the display and can also be changed by the user
- use a new cleared buffer for each test instead of the reusing same one to avoid potential result corruption across tests
- DRT can now receives the expected pixel hash as a suffix to the test path or url as "path|hash"

This patch includes several significant changes and I will not have time to review it tonight, but I was wondering about this:

+// We also need to ensure font smoothing (i.e. LCD Antialiasing) has been disabled, but
+// NSGraphicsContext seems to be overriding CGContextSetShouldSmoothFonts()...
+// 
+// Since there's no public API to force disable font smoothing, we work around it by using a
+// 'bitmapInfo' incompatible with font smoothing i.e. not matching pixels on screen as
+// (kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host).

In setDefaultsToConsistentValuesForTesting() in DumpRenderTree.mm, we call

    [defaults setInteger:MediumFontSmoothing forKey:@"AppleFontSmoothing"];

I believe that setting it to 0 instead of MediumFontSmoothing (2) is a far better way to accomplish this, especially since since there is code in WebCore that looks directly at that user default.

For Windows, there should be a similar way to do it, which is how Safari's appearance preferences work.

[Pierre-Olivier Latour]

We were indeed wondering if there was a NS default for font smoothing. But actually, it doesn't really matter:

It seems simpler to generate the same pixels on PPC or X86, so that the MD5 hashes match. That's why it now renders to RGBA8 independently of the endianness. A side effect of this is that because CG font smoothing is only supported for (kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host), you can't have a common pixel format and font smoothing.

On a side note, RGBA8 is also matching the PNG 32bits pixel format, except for alpha being premultiplied in CG and not in PNG.

(In reply to comment #5)
> It seems simpler to generate the same pixels on PPC or X86, so that the MD5
> hashes match. That's why it now renders to RGBA8 independently of the
> endianness. A side effect of this is that because CG font smoothing is only
> supported for (kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host), you
> can't have a common pixel format and font smoothing.

I see. Still, I would remove the discussion of font smoothing from the comment and change the font smoothing setting in setDefaultsToConsistentValuesForTesting(). It also looks like you removed the code path that reads pixels back from the window backing store, and instead you always have the view paint itself directly into your bitmap context. Does that not break the invalidation tests that use layoutTestController.display() (many of the tests in fast/repaint)?

[Pierre-Olivier Latour]

(In reply to comment #6)
> It also looks like you removed the
> code path that reads pixels back from the window backing store, and instead you
> always have the view paint itself directly into your bitmap context. Does that
> not break the invalidation tests that use layoutTestController.display() (many
> of the tests in fast/repaint)?

Indeed, this created a problem in this code path: I'll revert this part of the patch.

I guess there's nothing to change regarding the "AppleFontSmoothing" default since rendering happens to the window backbuffer no matter what and font smoothing will work there. The advantage is that base images don't need to be regenerated.

However, there's still an issue with the --repaint option for the DRT tool: the previous code path that was rendering directly to the CGContext hasn't changed. So either 1) you have font smoothing and different hashes for PPC / X86, 2) no font smoothing, but consistent hashes or 3) change this code path to also go through the window backbuffer.

Is this option used? If yes, in which tests?

(In reply to comment #7)
> Is this option used? If yes, in which tests?

It is available through a run-webkit-tests command-line option for any test and programatically through layoutTestController, with a few fast/repaint tests using it.

I think it is fine to make changes that will require all pixel results to be regenerated, so if font smoothing contributes to inter-configuration variance, we should still consider turning it off.

[Pierre-Olivier Latour]

On side note, on X86 w/ 10.5.4 + WK TOT, it seems all pixel tests in fast/repaint do fail (I don't specify a threshold). I looked at a few failures and minor differences in font rendering seem to be the reason.

(In reply to comment #9)
> On side note, on X86 w/ 10.5.4 + WK TOT, it seems all pixel tests in
> fast/repaint do fail (I don't specify a threshold). I looked at a few failures
> and minor differences in font rendering seem to be the reason.

Yes, this is the state for most tests currently.

[Pierre-Olivier Latour]

(In reply to comment #10)
> Yes, this is the state for most tests currently.

Is this a temporary thing because of a regression somewhere, or all base images need to be re-generated no matter what?

[Pierre-Olivier Latour]

I found an interesting issue: this method of reading back the pixels from the backing store basically strips any alpha as the returned NSBitmapImageRep is in RGB8.

(In reply to comment #12)
> I found an interesting issue: this method of reading back the pixels from the
> backing store basically strips any alpha as the returned NSBitmapImageRep is in
> RGB8.

I wonder if that is true for transparent windows as well. WebKit supports transparent WebViews and you can use them in windows (Safari's Debug menu has an option to use a transparent window). Anyway, we currently don't test drawing in transparent WebViews, so I don't think this is an issue.

[Pierre-Olivier Latour]

(In reply to comment #13)
> I wonder if that is true for transparent windows as well.

Actually, it is not, if you set your NSWindow properly for transparency, reading back from the backing with produce an RGBA8 bitmap. So I'm trying to get this to work in DRT as well. Even if alpha is not tested today, it probably doesn't hurt to be able to.

(In reply to comment #14)
> Actually, it is not, if you set your NSWindow properly for transparency,
> reading back from the backing with produce an RGBA8 bitmap. So I'm trying to
> get this to work in DRT as well. Even if alpha is not tested today, it probably
> doesn't hurt to be able to.

Sounds like a good option to have, but I am not sure it is a good idea to use a transparent window by default.

[Pierre-Olivier Latour]

(In reply to comment #15)
> Sounds like a good option to have, but I am not sure it is a good idea to use a
> transparent window by default.

More investigation shows that the only way to have the backing of the window to be considered to have alpha information is 1) to have it visible on screen and 2) have -setOpaque:NO and -setBackgroundColor:[NSColor clearColor] on the window 3) have the webview properly draw with transparency.

So in a nutshell, it's a complex fix for an unclear reward.

(In reply to comment #11)
> (In reply to comment #10)
> > Yes, this is the state for most tests currently.
> 
> Is this a temporary thing because of a regression somewhere, or all base images
> need to be re-generated no matter what?
> 

Most base images were generated on Tiger and do not match results on Leopard. I think those will need to be re-generated on Leopard, but perhaps some Leopard-based results will need to be regenerated as well as a result of this patch.

[Pierre-Olivier Latour]

Created attachment 24166 [details]
Revised patch

[Pierre-Olivier Latour]

(In reply to comment #18)
> Created an attachment (id=24166) [edit]
> Revised patch

Please make the previous patch obsolete (for some reason, I could not).

Note that with this new patch, font smoothing is always off, so pretty much all pixel tests fail as the bases need to be updated.

[Simon Fraser (smfr)]

The patch needs testing on Windows.

Comment on attachment 24166 [details]
Revised patch

Thanks for working on making pixel tests better!

A general comment:
Propagating knowledge of the test's URL or path even deeper into DumpRenderTree is not a good thing in my opinion. I think it would actually be good to move in the opposite direction: since you are adding the ability to pass metadata (the expected hash) along with the test path/URL, you should consider passing custom WebView dimensions in a similar fashion and making it the responsibility of run-webkit-tests to decide the dimensions based on the path (maybe in an even nicer way than hard-coding "svg/W3C-SVG-1.1").

I am not sure it makes sense to add the expected hash as a LayoutTestController member. We mostly use DumpRenderTree globals for things like that.

 34 #include <ctype.h>
 35 #include <algorithm>

Those should be in ASCII order along with the rest of the #includes, so just before #include <wtf/Assertions.h>.

 91         snprintf(hashString, 33, "%s%02x", hashString, hash[i]);

That is not pretty! (But you did not write it.)

 38 typedef struct CGContext* CGContextRef;
 39 
 40 
 41 class BitmapContext : public RefCounted<BitmapContext>

Extra newline there.

 78         , m_context(AdoptCF, context)

That makes for a very unusual calling convention where BitmapContext::create() takes ownership of the passed-in CGContext. I would rather avoid it.

 92 PassRefPtr<BitmapContext> getBitmapContextFromWebView(bool incrementalRepaint, bool sweepHorizontally, bool drawSelectionRect);

This is an unfortunate name for a function with "create" semantics. Again, not your fault.

 195         if (URLString.compare(URLString.length() - curIgnore.length(), curIgnore.length(), curIgnore) == 0)

WebKit coding style is to avoid comparison with 0, NULL and nil, so the above should be written as

 195         if (!URLString.compare(URLString.length() - curIgnore.length(), curIgnore.length(), curIgnore))

 353 #if PLATFORM(MAC)
 354     restoreMainDisplayColorProfile(0);
 355 #endif

This is a Mac-only file, so you don't need the #if.

 530 #if PLATFORM(MAC)
528531     if (dumpPixels)
529          restoreColorSpace(0);
 532         restoreMainDisplayColorProfile(0);
 533 #endif

Ditto.

 967     // Look for '|' as a separator between the path or URL, and the pixel dump hash that follows.
 968     // FIXME: this is a valid URL characters. Use a different separator?  

Typo: "characters". I think this is a problem. While | is a valid URL character and almost any character is valid in a path in some platform, but the single quote (') is not valid in the scheme part of a URL, so a format like
        'expectedhash'path/or/URL
will be unambiguous for the URL case, and only problematic for paths that begin with a '.

 978     NSString* pathOrURLString = [NSString stringWithUTF8String:pathOrURL.c_str()];

WebKit coding style is for Objective-C objects to put the space before the *, so "NSString *pathOrURLString = ...".

 984     NSURL* url;

Ditto.

 990         fprintf(stderr, "Failed to parse \"%s\" as an url\n", pathOrURL.c_str());

Typo: "an url" instead of "a URL".

 84     NSURL* url = [NSURL fileURLWithPath:[NSString stringWithUTF8String:PROFILE_PATH]];

Again, the * should be next to "url".

 88         if(error == noErr)

Missing space after "if". I think you can write this as if (!error).

 130     size_t rowBytes = (4 * pixelsWide + 63) & ~63; // Use a multiple of 64 bytes to improve CG performances

Typo? "performances".

 146     NSGraphicsContext* nsContext = [NSGraphicsContext graphicsContextWithGraphicsPort:context flipped:NO];

"*" misplaced, through no fault of your own.

 157     }
 158     else {

This should be all on one line as "} else {"

 172         NSView* documentView = [[mainFrame frameView] documentView];

Another * placement issue.

644              } elsif (/BaselineHash: ([a-f0-9]{32})/) {
 659             } elsif (/ExpectedHash: ([a-f0-9]{32})/) {

Is this change correct? What is it fixing? Is that code even used?

r- for now, as I think you should address at least some of the comments.

[Pierre-Olivier Latour]

(In reply to comment #21)
> A general comment:
> Propagating knowledge of the test's URL or path even deeper into DumpRenderTree
> is not a good thing in my opinion. I think it would actually be good to move in
> the opposite direction: since you are adding the ability to pass metadata (the
> expected hash) along with the test path/URL, you should consider passing custom
> WebView dimensions in a similar fashion and making it the responsibility of
> run-webkit-tests to decide the dimensions based on the path (maybe in an even
> nicer way than hard-coding "svg/W3C-SVG-1.1").

I actually haven't touched this part of the DRT logic, it behaves the same as before. The difference is that the path to the test is now stored in the layout controller along with optional hash instead of being in a global.

The issue you raise make sense but I'd rather have it addressed in a separate bug: the patch is already pretty big as is :)

> I am not sure it makes sense to add the expected hash as a LayoutTestController
> member. We mostly use DumpRenderTree globals for things like that.

gLayoutTestController is created / destroyed for each test, which I like as this way you know you don't have "stalled" parameters or data.

>  34 #include <ctype.h>
>  35 #include <algorithm>
> 
> Those should be in ASCII order along with the rest of the #includes, so just
> before #include <wtf/Assertions.h>.

Fixed

>  91         snprintf(hashString, 33, "%s%02x", hashString, hash[i]);
> 
> That is not pretty! (But you did not write it.)

Yeah, I noticed too ;)

>  38 typedef struct CGContext* CGContextRef;
>  39 
>  40 
>  41 class BitmapContext : public RefCounted<BitmapContext>
> 
> Extra newline there.

Fixed

>  78         , m_context(AdoptCF, context)
> 
> That makes for a very unusual calling convention where BitmapContext::create()
> takes ownership of the passed-in CGContext. I would rather avoid it.

I'd rather not put the CGContext creation code in BitmapContext as it is closely coupled to getBitmapContextFromWebView(): having all this setup code in a single place makes it easier to maintain it (especially when you have to properly deal with colorspace, endianness, pixel format and whatnot).

>  92 PassRefPtr<BitmapContext> getBitmapContextFromWebView(bool
> incrementalRepaint, bool sweepHorizontally, bool drawSelectionRect);
> 
> This is an unfortunate name for a function with "create" semantics. Again, not
> your fault.

Agreed, fixed

>  195         if (URLString.compare(URLString.length() - curIgnore.length(),
> curIgnore.length(), curIgnore) == 0)
> 
> WebKit coding style is to avoid comparison with 0, NULL and nil, so the above
> should be written as
> 
>  195         if (!URLString.compare(URLString.length() - curIgnore.length(),
> curIgnore.length(), curIgnore))

I hadn't touched this part but fixed anyway

>  353 #if PLATFORM(MAC)
>  354     restoreMainDisplayColorProfile(0);
>  355 #endif
> 
> This is a Mac-only file, so you don't need the #if.

Fixed

>  967     // Look for '|' as a separator between the path or URL, and the pixel
> dump hash that follows.
>  968     // FIXME: this is a valid URL characters. Use a different separator?  
> 
> Typo: "characters". I think this is a problem. While | is a valid URL character
> and almost any character is valid in a path in some platform, but the single
> quote (') is not valid in the scheme part of a URL, so a format like
>         'expectedhash'path/or/URL
> will be unambiguous for the URL case, and only problematic for paths that begin
> with a '.

OK, changed

>  978     NSString* pathOrURLString = [NSString
> stringWithUTF8String:pathOrURL.c_str()];
> 
> WebKit coding style is for Objective-C objects to put the space before the *,
> so "NSString *pathOrURLString = ...".
> 
>  984     NSURL* url;

Fixed

>  990         fprintf(stderr, "Failed to parse \"%s\" as an url\n",
> pathOrURL.c_str());
> 
> Typo: "an url" instead of "a URL".

Fixed

>  84     NSURL* url = [NSURL fileURLWithPath:[NSString
> stringWithUTF8String:PROFILE_PATH]];
> 
> Again, the * should be next to "url".

Fixed

>  88         if(error == noErr)
> 
> Missing space after "if". I think you can write this as if (!error).

Fixed

>  130     size_t rowBytes = (4 * pixelsWide + 63) & ~63; // Use a multiple of 64
> bytes to improve CG performances
> 
> Typo? "performances".

Fixed

>  146     NSGraphicsContext* nsContext = [NSGraphicsContext
> graphicsContextWithGraphicsPort:context flipped:NO];
> 
> "*" misplaced, through no fault of your own.

Fixed

>  157     }
>  158     else {
> 
> This should be all on one line as "} else {"

Fixed

>  172         NSView* documentView = [[mainFrame frameView] documentView];
> 
> Another * placement issue.

Fixed

> 644              } elsif (/BaselineHash: ([a-f0-9]{32})/) {
>  659             } elsif (/ExpectedHash: ([a-f0-9]{32})/) {
> 
> Is this change correct? What is it fixing? Is that code even used?

Yes, this is a required change: the script expects DRT to outputs the expected MD5 (even if the same script passes it to DRT in the first place), so I fixed DRT and for consistency also fixed the script to use "expected" as everywhere else in the pixel test system.

[Pierre-Olivier Latour]

Created attachment 24196 [details]
Re-revised patch

[Pierre-Olivier Latour]

(In reply to comment #23)
> Created an attachment (id=24196) [edit]
> Re-revised patch

Note that as explained earlier, a number of pixel now fail because of font smoothing always being disabled. Although, many were already failing anyway because of small differences in font rendering.

So basically all bases need to be regenerated, but I'd rather not do it as part of this patch as we have at least one other change to get in: https://bugs.webkit.org/show_bug.cgi?id=21458

[Pierre-Olivier Latour]

Another improvement we would need:
https://bugs.webkit.org/show_bug.cgi?id=21495

After that, I think we're done :)

[Pierre-Olivier Latour]

Created attachment 24588 [details]
Complete patch v1

Primary changes in DumpRenderTree:
- Ensure font smoothing is disabled (this is also called LCD anti-aliasing and
is different from regular font CG anti-aliasing) as font-smoothing settings
depends on the display and can also be changed by the user
- Use a new cleared buffer for each test instead of the reusing same one to
avoid potential result corruption across tests
- Can now receive the expected pixel hash as a suffix to the test path or
url as "path'hash"
- Make sure hash is computed in a endian-independent way
- Improve the code that sets/restores the screen color profile
- Make the code more cross-platformy with std::string goodness
- Added an "on-screen" mode where the snapshot will take into account surfaces on the window (like OpenGL content): this uses the new CG APIs on 10.5 or reading from the display framebuffer on 10.4. This mode is not active by default for performance reason, but must be explicitly activated from the test file using the new "testOnscreen()" JS API.

Primary changes in ImageDiff:
- Provide a new comparison algorithm that is more tolerant to "acceptable"
failures (i.e. very small differences in font rendering, which --threshold is
not really good at handling)
- Generate normalized intensity-only diff images

Primary changes in run-webkit-tests:
- Take advantage of hashes for pixel tests which makes them much faster by minimizing image comparisons
- Removed repaint options as these should be set from within test files using JS API
- Replaced "threshold" option in  by "tolerance" expressed in percents
- Added more logging when in "verbose" mode

Notes:
- Tested on various PPC and X86 machines running 10.5.4 as well as PPC running 10.4.11 and X86 running SnowLeopard
- Made sure WebKit still builds on Windows
- This patch will require to redo all reference images in LayoutTests to really take advantage of the new features

Finally, the current version of this patch does not include the ChangeLog, since there will likely be changes to be made anyway.

Comment on attachment 24588 [details]
Complete patch v1

>  #include <wtf/RefCounted.h>
> +#include <string>

Those are in the wrong order.

>  #if PLATFORM(WIN)
> +#define strtof(x, y) strtod(x, y)
> +#define roundf(x) ((x-floorf(x)) > 0.5 ? ceilf(x) : floorf(x))
>  static const CFStringRef kUTTypePNG = CFSTR("public.png");
>  #endif

I'd like Adam to comment on that.

> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());

Can this be a static?

> +    void *baseBuffer = calloc(height, rowBytes);

The * should go next to "void".

> +    void *buffer = calloc(height, rowBytes);

Ditto.

> +    void *diffBuffer = malloc(width * height);

Ditto.

I think it might be better to hang on to these buffers and only reallocate them as size changes.

> +    unsigned char *basePixel = (unsigned char*)baseBuffer;
> +    unsigned char *pixel = (unsigned char*)buffer;
> +    unsigned char *diff = (unsigned char*)diffBuffer;

More misplaced *s.

> +                if (distance > top)
> +                    top = distance;

Perhaps "maxDistance" is a better name than "top".

> +    // Compute the difference as a percentage combining both the number of different pixels and their difference amount
> +    if (count > 0.0f)
> +        difference = 100.0f * (count / ((float)width * (float)height)) * (sum / count);

I am not sure the two "count" terms improve clarity. "100 * sum / (width * height)" is more understandable, and will do floating point division and multiplication because sum is a float. Given that the "count" terms cancel each other out, I think it is a little misleading to say that the formula is "combining both the number of different pixels and their difference amount". It is simply the average distance over the entire image.

> +        if (top < 1.0f) {
> +            diff = (unsigned char*)diffBuffer;
> +            for(size_t p = 0; p < height * width; ++p, ++diff)
> +            *diff = (float)*diff / top;
> +        }

The body of the for statement should be indented. The cast to float is redundant because top is a float. I think not incrementing diff and using diff[p] instead would read better.

> +    CGImageAlphaInfo            info = CGImageGetAlphaInfo(image);

Extra spaces there.

> +                    difference = roundf(difference * 100.0f) / 100.0f;
> +                    difference = max(difference, 0.01f); // round to 2 decimal places

Artificially changing the rounding rule near 0 is kind of dubious. If people specify a tolerance of less than .01, then they probably higher precision anyway.

> +            }
> +            else
> +                fprintf(stdout, "diff: %01.2f%% passed\n", difference);

The else should go on the same line with the brace.

> +    // On PPC the bitmap context will be in ARGB8 instead of BGRA8 as on X86, so we need to swap the bytes to ensure consistent hashes

It's better to get the bitmap format from the context than to decide at compile-time based on endianness.

> +        for (unsigned column = 0; column < pixelsWide; column++) {
> +            buffer[column] = OSReadLittleInt32(bitmapData, 4 * column);
> +        }

A one-line body should not have braces.

> +    ASSERT(context.get());

The get() is redundant.

> +    // Look for "'" as a separator between the path or URL, and the pixel dump hash that follows.

I thought you were going to make the hash come before the URL, to avoid the problem with URLs containing the separator character.

> +        fprintf(stderr, "Failed to parse \"%s\" as a url\n", pathOrURL.c_str());

"url" should be uppercase.

> +                            void *flipBuffer = calloc(pixelsHigh, rowBytes);

Misplaced "*".

> +        }
> +        else
> +            ASSERT_NOT_REACHED();

The else should go on the same line with the brace. But it would be better to make an explicit ASSERT([documentView conformsToProtocol:@protocol(WebDocumentSelection)]) before the if statement instead.

[Adam Roben (:aroben)]

Comment on attachment 24588 [details]
Complete patch v1

It's exciting to see someone new working on the pixel tests!

This patch would be a lot easier to review if it were split up into a number of smaller changes (e.g., the move to use std::string in more places could have been its own patch). It's hard to review large patches, and when there's a straightforward way to break the work up into smaller pieces it's almost always better to do so.

Normally in .cpp files we use using directives rather than continuing to use namespace scope operators. For example, in LayoutTestController.cpp, you could put "using namespace std;" just after all the #include directives, then all the places where you have "std::string" in that file could just become "string". (We don't use using directives in headers, however, so you'd still need the "std::" prefix there.)

Even though this is a patch-in-progress that will probably be further revised, having a detailed ChangeLog is still important. It makes the reviewer's job much easier because it explains *why* each change was made, not just *how* (which is all the code tells you). I also find it helpful as a patch writer to write a detailed ChangeLog before sending my patch out for review because it forces me to think about the approach I took as a whole. Sometimes I end up changing parts of my patch because of things I realized while writing my own ChangeLog, even before sending the patch out for review.

> +++ b/WebKitTools/DumpRenderTree/cg/ImageDiffCG.cpp
> @@ -54,6 +54,8 @@ typedef float CGFloat;
>  using namespace std;
>  
>  #if PLATFORM(WIN)
> +#define strtof(x, y) strtod(x, y)
> +#define roundf(x) ((x-floorf(x)) > 0.5 ? ceilf(x) : floorf(x))
>  static const CFStringRef kUTTypePNG = CFSTR("public.png");
>  #endif

You should use wtf/MathExtras.h to get a definition of roundf() for Windows. I think a static inline function would be better than a macro for strtof.

> +static RetainPtr<CGImageRef> compareImages(CGImageRef baseImage, CGImageRef testImage, float& difference)

I find the "compareImages" name a bit confusing. Maybe something like createDifferenceImage would be clearer? Or if the main purpose is to produce the difference value and the image is more of a side-effect, perhaps the image should be an out parameter and the difference value should be the return value.

> +void dumpWebViewAsPixelsAndCompareWithExpected(const std::string& expectedHash)
>  {
> -    RetainPtr<CGContextRef> context = getBitmapContextFromWebView();
> -
> +    RefPtr<BitmapContext> context;
> +    
>  #if PLATFORM(MAC)
> -    if (gLayoutTestController->testRepaint())
> -        repaintWebView(context.get(), gLayoutTestController->testRepaintSweepHorizontally());
> -    else 
> -        paintWebView(context.get());
> -
> -    if (gLayoutTestController->dumpSelectionRect())
> -        drawSelectionRect(context.get(), getSelectionRect());
> +    context = createBitmapContextFromWebView(gLayoutTestController->testOnscreen(), gLayoutTestController->testRepaint(), gLayoutTestController->testRepaintSweepHorizontally(), gLayoutTestController->dumpSelectionRect());

There's no need to pre-declare the context variable. You can just do:

RefPtr<BitmapContext> context = createBitmapContextFromWebView(...);

> +class BitmapContext : public RefCounted<BitmapContext>
> +{

This brace should go on the previous line.

> +#if PLATFORM(MAC)
> +    BitmapContext(void* pixelData, CGContextRef context)
> +        : m_pixelData(pixelData)
> +#elif PLATFORM(WIN)
> +    BitmapContext(HBITMAP bitmap, CGContextRef context)
> +        : m_bitmap(bitmap)
> +#endif
> +        , m_context(AdoptCF, context)
> +    {
> +    }
> +    
> +#if PLATFORM(MAC)
> +    void* m_pixelData;
> +#elif PLATFORM(WIN)
> +    HBITMAP m_bitmap;
> +#endif    
> +    RetainPtr<CGContextRef> m_context;
> +
> +};

It might be cleaner to make a typedef that is void* on Mac and HBITMAP on Windows.

> +void setupMainDisplayColorProfile()
>  {
> -    fprintf(stderr, "Failed to get current color profile.  I will not be able to restore your current profile, thus I'm not changing it.  Many pixel tests may fail as a result.  (Error: %i)\n", error);
> -}
> -
> -static void setDefaultColorProfileToRGB()
> -{
> -    CMProfileRef genericProfile = (CMProfileRef)[[NSColorSpace genericRGBColorSpace] colorSyncProfile];
> -    CMProfileLocation genericProfileLocation;
> -    UInt32 locationSize = sizeof(genericProfileLocation);
> -    int error = NCMGetProfileLocation(genericProfile, &genericProfileLocation, &locationSize);
> -    if (error) {
> -        failedGettingCurrentProfile(error);
> -        return;
> +    const CMDeviceScope scope = { kCFPreferencesCurrentUser, kCFPreferencesCurrentHost };
> +    int error;
> +    
> +    CMProfileRef profile = NULL;

We use 0 instead of NULL in C++ and Obj-C++ code.

> +    CGContextRef context = CGBitmapContextCreate(buffer, pixelsWide, pixelsHigh, 8, rowBytes, colorSpace, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host); // Use ARGB8 on PPC or BGRA8 on X86 to improve CG performance
> +    if (!context) {
> +        free(buffer);
> +        return 0;
>      }
>  
> -    [NSGraphicsContext setCurrentContext:savedContext.get()];
> -}
> +    // The BitmapContext keeps the CGContextRef and the pixel buffer alive
> +    RefPtr<BitmapContext> bitmapContext = BitmapContext::create(buffer, context);

I believe you're leaking the CGContext here. BitmapContext retains the context you pass in, so you need to call CGContextRelease to balance the CGBitmapContextCreate.

> @@ -63,6 +61,8 @@ RetainPtr<CGContextRef> getBitmapContextFromWebView()
>      ASSERT(info.bmBitsPixel == 32);
>  
>      RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());
> -    return RetainPtr<CGContextRef>(AdoptCF, CGBitmapContextCreate(info.bmBits, info.bmWidth, info.bmHeight, 8,
> -                                                info.bmWidthBytes, colorSpace.get(), kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst));
> +    CGContextRef context = CGBitmapContextCreate(info.bmBits, info.bmWidth, info.bmHeight, 8,
> +                                                info.bmWidthBytes, colorSpace.get(), kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst);
> +
> +    return BitmapContext::create(bitmap, context);

You're leaking context here, too.

> @@ -658,42 +658,58 @@ for my $test (@tests) {
>          }
>      }
>  
> -    # If we got some PNG data, diff with expected
> +    if($verbose && $pixelTests && !$resetResults && $actualPNGSize) {

You're missing a space after "if".

We'll at least need to fix the leaks before we land this. Fixing the other suggestions Dan and I gave you would be good, too.

[Adam Roben (:aroben)]

(In reply to comment #28)

> > +    CGContextRef context = CGBitmapContextCreate(buffer, pixelsWide, pixelsHigh, 8, rowBytes, colorSpace, kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host); // Use ARGB8 on PPC or BGRA8 on X86 to improve CG performance
> > +    if (!context) {
> > +        free(buffer);
> > +        return 0;
> >      }
> >  
> > -    [NSGraphicsContext setCurrentContext:savedContext.get()];
> > -}
> > +    // The BitmapContext keeps the CGContextRef and the pixel buffer alive
> > +    RefPtr<BitmapContext> bitmapContext = BitmapContext::create(buffer, context);
> 
> I believe you're leaking the CGContext here. BitmapContext retains the context
> you pass in, so you need to call CGContextRelease to balance the
> CGBitmapContextCreate.

Dan has pointed out to me that BitmapContext's constructor adopts the CGContext. I think that is quite confusing (as he said in comment 21). I think it would be clearer to get rid of the adoption in the constructor and just add a release at the callsites.

[Pierre-Olivier Latour]

(In reply to comment #27)
> (From update of attachment 24588 [details] [edit])
> >  #include <wtf/RefCounted.h>
> > +#include <string>
> 
> Those are in the wrong order.

Fixed

> > +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());
> 
> Can this be a static?

Fixed
 
> > +    void *baseBuffer = calloc(height, rowBytes);
> > +    void *buffer = calloc(height, rowBytes);
> > +    void *diffBuffer = malloc(width * height);

Fixed

> I think it might be better to hang on to these buffers and only reallocate them
> as size changes.

I'd rather re-allocate zero'ed new buffers each time, to ensure you don't get any corruption due to previous results . If you hit the DiffTool, then you're already on the comparison slow path anyway (hash comparison failed), so it's not like it's gonna make a difference.

> > +    unsigned char *basePixel = (unsigned char*)baseBuffer;
> > +    unsigned char *pixel = (unsigned char*)buffer;
> > +    unsigned char *diff = (unsigned char*)diffBuffer;

Fixed

> > +                if (distance > top)
> > +                    top = distance;
> 
> Perhaps "maxDistance" is a better name than "top".

Fixed

> > +    // Compute the difference as a percentage combining both the number of different pixels and their difference amount
> > +    if (count > 0.0f)
> > +        difference = 100.0f * (count / ((float)width * (float)height)) * (sum / count);
> 
> I am not sure the two "count" terms improve clarity. "100 * sum / (width *
> height)" is more understandable, and will do floating point division and
> multiplication because sum is a float. Given that the "count" terms cancel each
> other out, I think it is a little misleading to say that the formula is
> "combining both the number of different pixels and their difference amount". It
> is simply the average distance over the entire image.

Fixed

> > +        if (top < 1.0f) {
> > +            diff = (unsigned char*)diffBuffer;
> > +            for(size_t p = 0; p < height * width; ++p, ++diff)
> > +            *diff = (float)*diff / top;
> > +        }
> 
> The body of the for statement should be indented. The cast to float is
> redundant because top is a float. I think not incrementing diff and using
> diff[p] instead would read better.

Fixed

> > +    CGImageAlphaInfo            info = CGImageGetAlphaInfo(image);
> 
> Extra spaces there.

Fixed

> > +                    difference = roundf(difference * 100.0f) / 100.0f;
> > +                    difference = max(difference, 0.01f); // round to 2 decimal places
> 
> Artificially changing the rounding rule near 0 is kind of dubious. If people
> specify a tolerance of less than .01, then they probably higher precision
> anyway.

That's what I had initially, but realized after that you actually don't want this: fundamentally, if the image differ, the tool should not return 0.0 no matter what.

> > +            }
> > +            else
> > +                fprintf(stdout, "diff: %01.2f%% passed\n", difference);
> 
> The else should go on the same line with the brace.

Fixed

> > +    // On PPC the bitmap context will be in ARGB8 instead of BGRA8 as on X86, so we need to swap the bytes to ensure consistent hashes
> 
> It's better to get the bitmap format from the context than to decide at
> compile-time based on endianness.

Fixed

> > +        for (unsigned column = 0; column < pixelsWide; column++) {
> > +            buffer[column] = OSReadLittleInt32(bitmapData, 4 * column);
> > +        }
> 
> A one-line body should not have braces.

Fixed

> > +    ASSERT(context.get());
> 
> The get() is redundant.

Fixed

> > +    // Look for "'" as a separator between the path or URL, and the pixel dump hash that follows.
> 
> I thought you were going to make the hash come before the URL, to avoid the
> problem with URLs containing the separator character.

I got confused, I thought we just want to use a different separator.

> > +        fprintf(stderr, "Failed to parse \"%s\" as a url\n", pathOrURL.c_str());
> 
> "url" should be uppercase.

Fixed

> > +                            void *flipBuffer = calloc(pixelsHigh, rowBytes);
> 
> Misplaced "*".

Fixed

> > +        }
> > +        else
> > +            ASSERT_NOT_REACHED();
> 
> The else should go on the same line with the brace. But it would be better to
> make an explicit ASSERT([documentView
> conformsToProtocol:@protocol(WebDocumentSelection)]) before the if statement
> instead.

Fixed

[Pierre-Olivier Latour]

(In reply to comment #29)
> > I believe you're leaking the CGContext here. BitmapContext retains the context
> > you pass in, so you need to call CGContextRelease to balance the
> > CGBitmapContextCreate.
> 
> Dan has pointed out to me that BitmapContext's constructor adopts the
> CGContext. I think that is quite confusing (as he said in comment 21). I think
> it would be clearer to get rid of the adoption in the constructor and just add
> a release at the callsites.

You cannot do this because the CGBitmapContext does not retain its backing, which is also different on Mac vs Windows. That's why we had to create this wrapper class that owns the context and the backing.

[Adam Roben (:aroben)]

(In reply to comment #31)
> (In reply to comment #29)
> > > I believe you're leaking the CGContext here. BitmapContext retains the context
> > > you pass in, so you need to call CGContextRelease to balance the
> > > CGBitmapContextCreate.
> > 
> > Dan has pointed out to me that BitmapContext's constructor adopts the
> > CGContext. I think that is quite confusing (as he said in comment 21). I think
> > it would be clearer to get rid of the adoption in the constructor and just add
> > a release at the callsites.
> 
> You cannot do this because the CGBitmapContext does not retain its backing,
> which is also different on Mac vs Windows. That's why we had to create this
> wrapper class that owns the context and the backing.

I don't think I understand why retaining is not possible when adopting is possible.

All I'm suggesting is that the BitmapContext constructor retain the passed-in CGContext, where today it adopts it.

[Pierre-Olivier Latour]

(In reply to comment #28)

> This patch would be a lot easier to review if it were split up into a number of
> smaller changes (e.g., the move to use std::string in more places could have
> been its own patch). It's hard to review large patches, and when there's a
> straightforward way to break the work up into smaller pieces it's almost always
> better to do so.

That's how I had started, but unfortunately, everything became inter-dependent rapidly.
 
> Normally in .cpp files we use using directives rather than continuing to use
> namespace scope operators. For example, in LayoutTestController.cpp, you could
> put "using namespace std;" just after all the #include directives, then all the
> places where you have "std::string" in that file could just become "string".
> (We don't use using directives in headers, however, so you'd still need the
> "std::" prefix there.)

Fixed

> Even though this is a patch-in-progress that will probably be further revised,
> having a detailed ChangeLog is still important. It makes the reviewer's job
> much easier because it explains *why* each change was made, not just *how*
> (which is all the code tells you).

I have one this time :)

> > +++ b/WebKitTools/DumpRenderTree/cg/ImageDiffCG.cpp
> > @@ -54,6 +54,8 @@ typedef float CGFloat;
> >  using namespace std;
> >  
> >  #if PLATFORM(WIN)
> > +#define strtof(x, y) strtod(x, y)
> > +#define roundf(x) ((x-floorf(x)) > 0.5 ? ceilf(x) : floorf(x))
> >  static const CFStringRef kUTTypePNG = CFSTR("public.png");
> >  #endif
> 
> You should use wtf/MathExtras.h to get a definition of roundf() for Windows. I
> think a static inline function would be better than a macro for strtof.

Fixed

> > +static RetainPtr<CGImageRef> compareImages(CGImageRef baseImage, CGImageRef testImage, float& difference)
> 
> I find the "compareImages" name a bit confusing. Maybe something like
> createDifferenceImage would be clearer? Or if the main purpose is to produce
> the difference value and the image is more of a side-effect, perhaps the image
> should be an out parameter and the difference value should be the return value.

There are both as important, so I just renamed the function

> There's no need to pre-declare the context variable. You can just do:
> 
> RefPtr<BitmapContext> context = createBitmapContextFromWebView(...);

The exact code (which I assume wasn't visible in the diff) is:

    RefPtr<BitmapContext> context;
    
#if PLATFORM(MAC)
    context = createBitmapContextFromWebView(gLayoutTestController->testOnscreen(), gLayoutTestController->testRepaint(), gLayoutTestController->testRepaintSweepHorizontally(), gLayoutTestController->dumpSelectionRect());
#endif
    ASSERT(context);

> > +class BitmapContext : public RefCounted<BitmapContext>
> > +{
> 
> This brace should go on the previous line.

Fixed

> It might be cleaner to make a typedef that is void* on Mac and HBITMAP on
> Windows.

Fixed

> We use 0 instead of NULL in C++ and Obj-C++ code.

Fixed
 
> > -    # If we got some PNG data, diff with expected
> > +    if($verbose && $pixelTests && !$resetResults && $actualPNGSize) {
> 
> You're missing a space after "if".

Fixed

[Pierre-Olivier Latour]

> > > Dan has pointed out to me that BitmapContext's constructor adopts the
> > > CGContext. I think that is quite confusing (as he said in comment 21). I think
> > > it would be clearer to get rid of the adoption in the constructor and just add
> > > a release at the callsites.
> > 
> > You cannot do this because the CGBitmapContext does not retain its backing,
> > which is also different on Mac vs Windows. That's why we had to create this
> > wrapper class that owns the context and the backing.
> 
> I don't think I understand why retaining is not possible when adopting is
> possible.
> 
> All I'm suggesting is that the BitmapContext constructor retain the passed-in
> CGContext, where today it adopts it.

So you're ok with a constructor that adopts one argument but retains the other?

[Adam Roben (:aroben)]

(In reply to comment #34)
> > > > Dan has pointed out to me that BitmapContext's constructor adopts the
> > > > CGContext. I think that is quite confusing (as he said in comment 21). I think
> > > > it would be clearer to get rid of the adoption in the constructor and just add
> > > > a release at the callsites.
> > > 
> > > You cannot do this because the CGBitmapContext does not retain its backing,
> > > which is also different on Mac vs Windows. That's why we had to create this
> > > wrapper class that owns the context and the backing.
> > 
> > I don't think I understand why retaining is not possible when adopting is
> > possible.
> > 
> > All I'm suggesting is that the BitmapContext constructor retain the passed-in
> > CGContext, where today it adopts it.
> 
> So you're ok with a constructor that adopts one argument but retains the other?

Ah, I see what you mean now. OK, what if we changed BitmapContext::create to something like BitmapContext::createByAdoptingBitmapAndContext?

[Pierre-Olivier Latour]

> Ah, I see what you mean now. OK, what if we changed BitmapContext::create to
> something like BitmapContext::createByAdoptingBitmapAndContext?

Deal!

[Pierre-Olivier Latour]

Created attachment 24631 [details]
Updated patch

Comment on attachment 24631 [details]
Updated patch

> +        - Ensure font smoothing is disabled (this is also called LCD anti-aliasing and
> +        is different from regular font CG anti-aliasing) as font-smoothing settings
> +        depends on the display and can also be changed by the user

The user setting was overridden by the old code by setting the user default value to MediumFontSmoothing, so I don't think that was part of the problem.

> +        - Use a new cleared buffer for each test instead of the reusing same one to
> +        avoid potential result corruption across tests

Is there any evidence that such corruption occurred or that it could occur? It is OK to make this change, but it is not clear that it is solving a problem, not even a potential one.

> +                }
> +            }
> +            else
> +                fputs("error, test and reference image have different properties.\n", stderr);

The else should go on the same line with the brace.

> +    static PassRefPtr<BitmapContext> createByAdoptingBitmapAndContext(BitmapContextBacking backing, CGContextRef context)

I think "BitmapContextBacking" and "backing" are not ideal names. Variable names should almost always be nouns, and the noun meaning of "backing" (the gerund) does not make sense here. "Backing buffer" or "backing store" seem better for the variable names, while something "PlatformBitmapBuffer" might be better as the type name.

These are minor comments so I am going to say r=me.

[Pierre-Olivier Latour]

(In reply to comment #38)
> (From update of attachment 24631 [details] [edit])
> > +        - Ensure font smoothing is disabled (this is also called LCD anti-aliasing and
> > +        is different from regular font CG anti-aliasing) as font-smoothing settings
> > +        depends on the display and can also be changed by the user
> 
> The user setting was overridden by the old code by setting the user default
> value to MediumFontSmoothing, so I don't think that was part of the problem.

Actually, it's better not to have any font smoothing at all, as it's only available in some circumstances and can change across OS releases. There were also some bases that were checked in with no font smoothing, while the majority has it.

> > +        - Use a new cleared buffer for each test instead of the reusing same one to
> > +        avoid potential result corruption across tests
> 
> Is there any evidence that such corruption occurred or that it could occur? It
> is OK to make this change, but it is not clear that it is solving a problem,
> not even a potential one.

No evidence, but if for whatever reason a test page doesn't full paint opaquely the context, it will blend with the previous result.

> > +                }
> > +            }
> > +            else
> > +                fputs("error, test and reference image have different properties.\n", stderr);
> 
> The else should go on the same line with the brace.

Fixed

> > +    static PassRefPtr<BitmapContext> createByAdoptingBitmapAndContext(BitmapContextBacking backing, CGContextRef context)
> 
> I think "BitmapContextBacking" and "backing" are not ideal names. Variable
> names should almost always be nouns, and the noun meaning of "backing" (the
> gerund) does not make sense here. "Backing buffer" or "backing store" seem
> better for the variable names, while something "PlatformBitmapBuffer" might be
> better as the type name.

Fixed

[Pierre-Olivier Latour]

Created attachment 24689 [details]
Final patch

[Pierre-Olivier Latour]

Created attachment 24702 [details]
Final patch (for real)

Comment on attachment 24702 [details]
Final patch (for real)

r=me

[Simon Fraser (smfr)]

	M	WebKitTools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj
	M	WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp
	M	WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.h
	M	WebKitTools/DumpRenderTree/cg/ImageDiffCG.cpp
	M	WebKitTools/DumpRenderTree/PixelDumpSupport.h
	M	WebKitTools/DumpRenderTree/win/PixelDumpSupportWin.cpp
	M	WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp
	A	WebKitTools/DumpRenderTree/ForwardingHeaders/wtf/PassRefPtr.h
	A	WebKitTools/DumpRenderTree/ForwardingHeaders/wtf/RefPtr.h
	M	WebKitTools/DumpRenderTree/mac/PixelDumpSupportMac.mm
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
	M	WebKitTools/DumpRenderTree/LayoutTestController.cpp
	M	WebKitTools/DumpRenderTree/LayoutTestController.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/run-webkit-tests
r37928 = bdbd3f773d48b595c1cd282f048638dbf65b677e (trunk)