Bug 26457 – Build DumpRenderTree under Cairo

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

This bug allows the Windows Cairo port of WebKit to build the DumpRenderTree tool.

[Brent Fulgham]

Created attachment 31372 [details]
Rough initial patch

Initial cut.  Not ready for general users, but available for people who wish to try it out.

[Brent Fulgham]

Created attachment 31577 [details]
Proposed patch for adding this feature

[Eric Seidel (no email)]

Comment on attachment 31577 [details]
Proposed patch for adding this feature

I would think this would be abstracted into a shared call:
68     printf("Content-Type: %s\n", "image/png");
 69     printf("Content-Length: %lu\n", static_cast<unsigned long>(dataLength));
 70 
 71     const size_t bytesToWriteInOneChunk = 1 << 15;
 72     size_t dataRemainingToWrite = dataLength;
 73     while (dataRemainingToWrite) {
 74         size_t bytesToWriteInThisChunk = min(dataRemainingToWrite, bytesToWriteInOneChunk);
 75         size_t bytesWritten = fwrite(data, 1, bytesToWriteInThisChunk, stdout);
 76         if (bytesWritten != bytesToWriteInThisChunk)
 77             break;
 78         dataRemainingToWrite -= bytesWritten;
 79         data += bytesWritten;
 80     }

Style:
 65     const size_t dataLength = pixeldata.size ();

This part should all be shared:
93     // We need to swap the bytes to ensure consistent hashes independently of endianness
 94     MD5_CTX md5Context;
 95     MD5_Init(&md5Context);
 96     unsigned char* bitmapData = static_cast<unsigned char*>(cairo_image_surface_get_data(surface));
 97     for (unsigned row = 0; row < pixelsHigh; row++) {
 98         MD5_Update(&md5Context, bitmapData, 4 * pixelsWide);
 99         bitmapData += bytesPerRow;
 100     }
 101     unsigned char hash[16];
 102     MD5_Final(hash, &md5Context);
 103 
 104     hashString[0] = '\0';
 105     for (int i = 0; i < 16; i++)
 106         snprintf(hashString, 33, "%s%02x", hashString, hash[i]);
 107 }

There seems to be almost nothing cairo-specific about:
 109 void dumpWebViewAsPixelsAndCompareWithExpected(const std::string& expectedHash)


Can we share more code here?  At least can we abstract things into methods which we could share later?

[Brent Fulgham]

Created attachment 32087 [details]
Revised per Eric's comments.

Moved shared Cairo/CG logic to parent file.

[Brent Fulgham]

Created attachment 32088 [details]
Xcode change for Mac build.

The new PixelDumpSupport.cpp file is needed by the Mac build as well.

[Brent Fulgham]

(In reply to comment #3)
> (From update of attachment 31577 [details] [review])
> This part should all be shared:
> 93     // We need to swap the bytes to ensure consistent hashes independently
> of endianness
>  94     MD5_CTX md5Context;
>  95     MD5_Init(&md5Context);

I tried to share this, but this routine in shared between Cairo, CG (Windows), and CG (Mac).  On CG (Mac) there are some additional checks and endian handling that change the structure of this function a bit.

For now, I'd prefer to leave this alone (all CG share one version, Cairo uses another), at least until I can start getting some build and run feedback from GTK users to determine if similar endianness logic is warranted.

Otherwise, I have made the changes you suggested.

I've also tested the Mac version with these changes and confirmed it works properly on the LayoutTests.

[Adam Roben (:aroben)]

Comment on attachment 32088 [details]
Xcode change for Mac build.

r=me

[Adam Roben (:aroben)]

Comment on attachment 32087 [details]
Revised per Eric's comments.

> [phone]  U-bfulgham-PC\bfulgham  <[e-mail]>

Looks like you need to set REAL_NAME on this computer.

> +        * DumpRenderTree/PixelDumpSupport.cpp: Added.
> +        (dumpWebViewAsPixelsAndCompareWithExpected):
> +        (printPNG):
> +        * DumpRenderTree/PixelDumpSupport.h:
> +        * DumpRenderTree/cairo: Added.
> +        * DumpRenderTree/cairo/PixelDumpSupportCairo.cpp: Added.
> +        (writeFunction):
> +        (printPNG):
> +        (computeMD5HashStringForBitmapContext):
> +        (dumpBitmap):
> +        * DumpRenderTree/cairo/PixelDumpSupportCairo.h: Added.
> +        (BitmapContext::createByAdoptingBitmapAndContext):
> +        (BitmapContext::~BitmapContext):
> +        (BitmapContext::cairoContext):
> +        (BitmapContext::BitmapContext):
> +        * DumpRenderTree/cg/PixelDumpSupportCG.cpp:
> +        (printPNG):
> +        (computeMD5HashStringForBitmapContext):
> +        (dumpBitmap):
> +        * DumpRenderTree/cg/PixelDumpSupportCG.h:
> +        * DumpRenderTree/win/DumpRenderTree.cpp:
> +        (main):
> +        * DumpRenderTree/win/DumpRenderTree.vcproj:
> +        * DumpRenderTree/win/PixelDumpSupportWin.cpp:
> +        (createBitmapContextFromWebView):

It would be good to add more detailed comments about what you changed next to each function/file.

> +#include "config.h"
> +#include "PixelDumpSupport.h"
> +#if PLATFORM(CG)
> +#include <CoreGraphics/CGBitmapContext.h>
> +#include "PixelDumpSupportCG.h"
> +#else
> +#include <cairo-win32.h>
> +#include "PixelDumpSupportCairo.h"
> +#endif
> +
> +#include "DumpRenderTree.h"
> +#include "LayoutTestController.h"
> +#include <wtf/Assertions.h>
> +#include <wtf/RefPtr.h>
> +#include <wtf/RetainPtr.h>

I'm not sure I understand why we need to include any port-specific headers here. Can the declarations of the required functions move to PixelDumpSupport.h?

> +#if PLATFORM(WIN)
> +static const CFStringRef kUTTypePNG = CFSTR("public.png");
> +#endif

I don't think this is needed in PixelDumpSupportCairo.

> +    const size_t dataLength = pixeldata.size ();
> +    const unsigned char* data = &pixeldata[0];
> +
> +    printPNG (dataLength, data);

Please remove the spaces after the function names.

pixeldata.data() would be better than &pixeldata[0]. pixeldata should probably be renamed to pixelData.

> +void computeMD5HashStringForBitmapContext(RefPtr<BitmapContext> context, char hashString[33])

context's type should be BitmapContext*.

> +    // We need to swap the bytes to ensure consistent hashes independently of endianness

Your code doesn't seem to take endianness into account. Should this turn into a FIXME? Be removed entirely?

> +void dumpBitmap(RefPtr<BitmapContext> context)

context's type should be BitmapContext*.

> +{
> +    cairo_surface_t* surface = cairo_get_target(context->cairoContext());
> +    printPNG(surface);
> +}
> \ No newline at end of file

Please add a newline.

> +    RetainPtr<CairoContextRef> m_context;

It seems unlikely that RetainPtr is the right smart pointer for holding a CairoContextRef. Is it really right to use CFRetain/CFRelease with a CairoContextRef?

> +#endif // PixelDumpSupportCairo_h
> \ No newline at end of file

Please add a newline.

[Brent Fulgham]

(In reply to comment #8)
> (From update of attachment 32087 [details] [review])
> > [phone]  U-bfulgham-PC\bfulgham  <[e-mail]>
> 
> Looks like you need to set REAL_NAME on this computer.

Fixed (permanently!)

> > +        * DumpRenderTree/PixelDumpSupport.cpp: Added.
> > +        (dumpWebViewAsPixelsAndCompareWithExpected):
> > +        (printPNG):
[...]
> > +        * DumpRenderTree/win/PixelDumpSupportWin.cpp:
> > +        (createBitmapContextFromWebView):
> 
> It would be good to add more detailed comments about what you changed next to
> each function/file.

Done

> I'm not sure I understand why we need to include any port-specific headers
> here. Can the declarations of the required functions move to
> PixelDumpSupport.h?

Done.  I initially thought this header was included in other DumpRenderTree variants, but since
it was already required these headers it shouldn't be a problem.

> > +#if PLATFORM(WIN)
> > +static const CFStringRef kUTTypePNG = CFSTR("public.png");
> > +#endif
> 
> I don't think this is needed in PixelDumpSupportCairo.

You are right.  Removed.

> Please remove the spaces after the function names.
> 
> pixeldata.data() would be better than &pixeldata[0]. pixeldata should probably
> be renamed to pixelData.

Done.
 
> > +void computeMD5HashStringForBitmapContext(RefPtr<BitmapContext> context, char hashString[33])
> 
> context's type should be BitmapContext*.

Done.
 
> > +    // We need to swap the bytes to ensure consistent hashes independently of endianness
> 
> Your code doesn't seem to take endianness into account. Should this turn into a
> FIXME? Be removed entirely?

This was copied from the CG logic.  In theory, someone might run this on a PowerPC Linux box so this
might be an issue, but I'm not sure how much of an issue this is (no one else is using it yet, and Windows
is x86-only).

Removing the comment; this will only show up as a problem if the GTK port starts using this code on PPC or similar systems.  We can address it if that ever happens.

> > +void dumpBitmap(RefPtr<BitmapContext> context)
> 
> context's type should be BitmapContext*.

Done.

> > \ No newline at end of file

Done.
 
> > +    RetainPtr<CairoContextRef> m_context;
> 
> It seems unlikely that RetainPtr is the right smart pointer for holding a
> CairoContextRef. Is it really right to use CFRetain/CFRelease with a
> CairoContextRef?

Nope.  This is now fixed.
 
> > +#endif // PixelDumpSupportCairo_h
> > \ No newline at end of file

Done. 

[Brent Fulgham]

Created attachment 32189 [details]
Revised per Adam's comments.

[Brent Fulgham]

Created attachment 32190 [details]
Revised per Adam's comments (better).

[Eric Seidel (no email)]

Don't APIs normally have:
function(char* data, size_t length)
instead of:
function(length, data) like you used here?

[Brent Fulgham]

(In reply to comment #12)
> Don't APIs normally have:
> function(char* data, size_t length)
> instead of:
> function(length, data) like you used here?
> 

I guess; I can certainly change that if you want.

[Adam Roben (:aroben)]

Comment on attachment 32190 [details]
Revised per Adam's comments (better).

There's a newer version of the license header in WebCore/html/PreloadScanner.cpp. Please use that template for new files.

> +        * DumpRenderTree/PixelDumpSupport.h: Add declaration for new
> +          common pringPNG function.

Typo: pringPNG

> +    printPNG (dataLength, data);

Please remove the space after "printPNG".

> +typedef struct _cairo* CairoContextRef;

I thought there was a cairo_t typedef. Maybe there's some reason we can't use that name here.

> +    BitmapContext(PlatformBitmapBuffer buffer, CairoContextRef context)
> +        : m_buffer(buffer)
> +        , m_context(context)
> +    {
> +    }
> +
> +    PlatformBitmapBuffer m_buffer;
> +    CairoContextRef m_context;

Do we need to retain/release m_context (using the appropriate Cairo functions)?

r=me

[Brent Fulgham]

(In reply to comment #14)
> (From update of attachment 32190 [details] [review])
> There's a newer version of the license header in
> WebCore/html/PreloadScanner.cpp. Please use that template for new files.
> 
> > +        * DumpRenderTree/PixelDumpSupport.h: Add declaration for new
> > +          common pringPNG function.
> 
> Typo: pringPNG

Fixed while landing.

> > +    printPNG (dataLength, data);
> 
> Please remove the space after "printPNG".

Fixed while landing. 

> > +typedef struct _cairo* CairoContextRef;
> 
> I thought there was a cairo_t typedef. Maybe there's some reason we can't use
> that name here.

No; I thought it would be nice to look more like the CG code for comparison, but we use cairo_t* everywhere else, so I'm changing to that while landing.

> > +    BitmapContext(PlatformBitmapBuffer buffer, CairoContextRef context)
> > +        : m_buffer(buffer)
> > +        , m_context(context)
> > +    {
> > +    }
> > +
> > +    PlatformBitmapBuffer m_buffer;
> > +    CairoContextRef m_context;
> 
> Do we need to retain/release m_context (using the appropriate Cairo functions)?
> 
> r=me
Yes; I forgot about the possibility of the m_context not being null at the time we do the Adopt.  So, it needs to release the old context (if it exists), but it does *not* add a retain to the pointer being passed in.

Thanks -- I corrected that as well. 

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45505.

[Brent Fulgham]

Created attachment 32214 [details]
Workaround for build.