PixelBufferConversion.h functions should be tested so they can be modified more easily
Created attachment 432611 [details] Patch
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?
Created attachment 432669 [details] Patch
(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.
> 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.
<rdar://problem/80279213>