[Webkit-unassigned] [Bug 21322] DumpRenderTree pixel test improvements
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 23 12:32:35 PDT 2008
https://bugs.webkit.org/show_bug.cgi?id=21322
aroben at apple.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #24588|review? |review-
Flag| |
------- Comment #28 from aroben at apple.com 2008-10-23 12:32 PDT -------
(From update of attachment 24588)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list