PixelBufferConversion.h functions should be tested so they can be modified

• Component: WebGL
• Status: ASSIGNED ---

[Kimmo Kinnunen]

PixelBufferConversion.h functions should be tested so they can be modified more easily

[Kimmo Kinnunen]

Created attachment 432611 [details]
Patch

[Kenneth Russell]

Comment on attachment 432611 [details]
Patch

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

Looks good overall; couple of tiny questions. Setting r+ to allow this to be landed asynchronously.

One question: on which EWS bot did the new test run? Looked through a few, but didn't find it.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5526
> +		FD3160BF12B0272A00C1A359 /* (null) in Headers */ = {isa = PBXBuildFile; };

This change looks wrong.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:31778
> +				FD3160BF12B0272A00C1A359 /* (null) in Headers */,

Similarly here.

> Tools/TestWebKitAPI/Tests/WebCore/PixelBufferConversionTest.cpp:111
> +    v = v * 255 / alpha;

Here and in the premultiply routine: I never remember what intermediate types C++ promotes these mathematical expressions to. Is there a need to upcast to floating-point before dividing by alpha or 255, and then downcast back to uint8_t?

> Tools/TestWebKitAPI/Tests/WebCore/PixelBufferConversionTest.cpp:228
> +    EXPECT_EQ(expectedPixels, resultPixels);

No tolerance necessary?

[Kimmo Kinnunen]

Created attachment 432669 [details]
Patch

[Kimmo Kinnunen]

(In reply to Kenneth Russell from comment #2)
> Comment on attachment 432611 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432611&action=review
> 
> Looks good overall; couple of tiny questions. Setting r+ to allow this to be
> landed asynchronously.

Thanks for looking and commenting!
I'll revise the patch a bit later, intending to add few more test-cases.

> /Tests/WebCore/PixelBufferConversionTest.cpp:111
> > +    v = v * 255 / alpha;
> 
> Here and in the premultiply routine: I never remember what intermediate
> types C++ promotes these mathematical expressions to. Is there a need to
> upcast to floating-point before dividing by alpha or 255, and then downcast
> back to uint8_t?

Not in the sense that if the definition of the premultiplication equation is in ints, these should be in ints.
The equations in the test-case are intended to replicate the "this is the equation webkit uses for premultiplication" exactly.

> 
> > Tools/TestWebKitAPI/Tests/WebCore/PixelBufferConversionTest.cpp:228
> > +    EXPECT_EQ(expectedPixels, resultPixels);
> 
> No tolerance necessary?

The idea is that the premultiplication should be deterministic.

[Kimmo Kinnunen]

> v = v * 255 / alpha;
> 
> Here and in the premultiply routine: I never remember what intermediate
> types C++ promotes these mathematical expressions to. 

And the types are (as far as I understand the C++ type promotion):
uint8_t v
int 255 
uint8_t alpha

Promotes v to int to do the multiplication.
Promotes alpha to int to do the division.
Casts the result to uint8_t for the assignment.

All of the promotions in the equations just use the "int promotion rule", where types smaller than int are promoted to int to do any arithmetic.

[Radar WebKit Bug Importer]

<rdar://problem/80279213>