[Webkit-unassigned] [Bug 21322] DumpRenderTree pixel test improvements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21322





------- Comment #30 from pol at apple.com  2008-10-23 13:19 PDT -------
(In reply to comment #27)
> (From update of attachment 24588 [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


-- 
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.