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

[bugzilla-daemon at webkit.org]

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





------- Comment #27 from mitz at webkit.org  2008-10-23 11:50 PDT -------
(From update of attachment 24588)
>  #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.


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