Bug 58242 – [GTK] Implement pixel dump support for WebKitTestRunner

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

After #57068 closes, WebKitTestRunner will run, but not be able to dump test pixels. This bug tracks adding that feature.

[Martin Robinson]

Created attachment 89035 [details]
Patch

[Brent Fulgham]

Comment on attachment 89035 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89035&action=review

This change looks great! I've added a few comments for future thought.  I don't think anything I mention is worth addressing in this change, except perhaps the "ref" call.

> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:42
> +

Note: I'm not at all sure that this ref() is needed here.  In CoreGraphics, the allocation of the image surface includes a callback routine that performs the cleanup.  We might need to either omit this ref() here, or make sure that the destructor for the PassOwnPtr<GraphicsContext> somehow decrements the reference count here.

> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:61
> +

Note that I cribbed most (all?) of this routine from ImageCairo.cpp.  It would be nice if this could be shared someplace common for both implementations.

[Adam Roben (:aroben)]

Comment on attachment 89035 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89035&action=review

> Source/WebKit2/Shared/ShareableBitmap.h:116
> +    // This creates a copied BitmapImage (most likely a copy-on-write) of the shareable bitmap.
> +    cairo_surface_t* makeImageCopy();
> +
> +    // This creates a BitmapImage that directly references the shared bitmap data.
> +    // This is only safe to use when we know that the contents of the shareable bitmap won't change.
> +    cairo_surface_t* makeImage();

Seems like these should return PassRefPtr, given that you use RefPtr<cairo_surface_t> in the implementation. (I didn't know that was possible!)

Is makeImageCopy() really doing a copy-on-write? CG does that behind-the-scenes on Mac.

>> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:42
>> +    ref(); // Balanced by deref in releaseBitmapContextData.
>> +
> 
> Note: I'm not at all sure that this ref() is needed here.  In CoreGraphics, the allocation of the image surface includes a callback routine that performs the cleanup.  We might need to either omit this ref() here, or make sure that the destructor for the PassOwnPtr<GraphicsContext> somehow decrements the reference count here.

I agree with Brent. This looks like a leak to me. But if you remove this ref(), what will guarantee that the ShareableBitmap stays alive as long as the cairo_surface_t might access its data?

> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:110
> +void TestInvocation::dumpPixelsAndCompareWithExpected(WKImageRef wkImage)
> +{
> +    cairo_surface_t* surface =  WKImageCreateCairoSurface(wkImage);
> +
> +    char actualHash[33];
> +    computeMD5HashStringForCairoContext(surface, actualHash);
> +    fprintf(stdout, "\nActualHash: %s\n", actualHash);
> +
> +    // Check the computed hash against the expected one and dump image on mismatch
> +    bool hashesMatch = false;
> +    if (m_expectedPixelHash.length() > 0) {
> +        ASSERT(m_expectedPixelHash.length() == 32);
> +
> +        fprintf(stdout, "\nExpectedHash: %s\n", m_expectedPixelHash.c_str());
> +
> +        // FIXME: Do case insensitive compare.
> +        if (m_expectedPixelHash == actualHash)
> +            hashesMatch = true;
> +    }
> +
> +    if (!hashesMatch)
> +        dumpBitmap(surface);
>  }

Seems like we should refactor the code so that all ports can share the logic in the latter part of this function.

[Martin Robinson]

Comment on attachment 89035 [details]
Patch

Removing r? in response to aroben's comments.

[Brent Fulgham]

Created attachment 91520 [details]
Proposed update to ShareableBitmap implementation

[Martin Robinson]

Created attachment 97162 [details]
Patch

[Martin Robinson]

I've updated the patch here with Adam's suggestions.

[Adam Roben (:aroben)]

Comment on attachment 97162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97162&action=review

> Source/WebKit2/ChangeLog:12
> +        * Shared/API/c/cairo/WKImageCairo.h: Copied from Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp.

This file seems to be missing from your patch.

> Tools/WebKitTestRunner/TestInvocation.h:50
> +    bool compareActualHashToExpectedAndDumpResults(char[33]);

Can the argument be a const char[33]?

> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:49
> +    for (unsigned row = 0; row < pixelsHigh; row++) {

Might as well use size_t and preincrement here.

> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:50
> +        md5Context.addBytes(bitmapData, 4 * pixelsWide);

Is it worth checking the bits-per-pixel of the surface instead of assuming 32?

> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:92
> +    cairo_surface_t* surface = WKImageCreateCairoSurface(wkImage);

Can you use a RefPtr here?

> Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:-145
> -        // FIXME: Do case insensitive compare.

You lost this FIXME when moving this code.

[Martin Robinson]

Created attachment 98079 [details]
Patch wth Adam's suggestions

[Martin Robinson]

(In reply to comment #8)

Thanks for the review. I've attached a new patch, which I hope addresses your comments.

> (From update of attachment 97162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97162&action=review
> 
> > Source/WebKit2/ChangeLog:12
> > +        * Shared/API/c/cairo/WKImageCairo.h: Copied from Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp.
> 
> This file seems to be missing from your patch.

Wow. For some reason webkit-patch isn't picking it up. :(

> > Tools/WebKitTestRunner/TestInvocation.h:50
> > +    bool compareActualHashToExpectedAndDumpResults(char[33]);
> 
> Can the argument be a const char[33]?

Fixed!

> > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:49
> > +    for (unsigned row = 0; row < pixelsHigh; row++) {
> 
> Might as well use size_t and preincrement here.

Fixed!

> > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:50
> > +        md5Context.addBytes(bitmapData, 4 * pixelsWide);
> 
> Is it worth checking the bits-per-pixel of the surface instead of assuming 32?

I assert above that the format matches what we expect here. Is that enough? I believe that getting any other image format at this point represents an error.

> > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:92
> > +    cairo_surface_t* surface = WKImageCreateCairoSurface(wkImage);
> 
> Can you use a RefPtr here?

I had hoped so originally, but the Cairo RefPtr specializations are not header files and are not exported publically from libwebkitetetc.so.

> > Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:-145
> > -        // FIXME: Do case insensitive compare.
> 
> You lost this FIXME when moving this code.

Fixed!

[Adam Roben (:aroben)]

Comment on attachment 97162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97162&action=review

>>> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:50
>>> +        md5Context.addBytes(bitmapData, 4 * pixelsWide);
>> 
>> Is it worth checking the bits-per-pixel of the surface instead of assuming 32?
> 
> I assert above that the format matches what we expect here. Is that enough? I believe that getting any other image format at this point represents an error.

Sounds fine to me.

[Adam Roben (:aroben)]

Comment on attachment 98079 [details]
Patch wth Adam's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=98079&action=review

> Source/WebKit2/Shared/API/c/cairo/WKImageCairo.h:33
> +typedef _cairo_surface cairo_surface_t;

Seems like you're missing a "struct" in here.

[WebKit Review Bot]

Comment on attachment 98079 [details]
Patch wth Adam's suggestions

Clearing flags on attachment: 98079

Committed r89426: <http://trac.webkit.org/changeset/89426>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.