Issue 5494076: The SSE32 BlitRow function mistakenly degrades opaque pixels to be non-opaque when blending (Closed)
|
Created:
13 years ago by danakj Modified:
11 years, 5 months ago CC:
backer_chromium.org, piman, enne, skia-review_googlegroups.com, reed, reveman Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionThe function wants to divide by 256 instead of 255 for obvious reasons. It finds a scale multiplier to make the colors in the range [0,256], however it scales the alpha first when finding the multiplier which makes it incorrect.
What this means is that if you have a pixel P with alpha=255, and you blit a color C over it with alpha=127, the resulting alpha is:
255*(256-128)/256+127
= 255*128/256+127
= 254
Wrong!
If we scale the destination values up to be in the range 256 we get
256*(256-128)/256+127
= 256*128/256+127
= 255
Right!
Also, this gives correct values for combinations of P alpha=0, C alpha=0, and P alpha=255, C alpha=255 and C alpha=127 (ie. 0.5). Test is included to verify this.
P=0 C=0
1*(256-1)/256+0 = 0
P=255 C=0
256*(256-1)/256+0 = 255
P=0 C=255
1*(256-256)/256+255 = 255
P=255 C=255
256*(256-256)/256+255 = 255
BUG=420
TEST=BlitRectTest.cpp
Patch Set 1 #Patch Set 2 : Get correct 0 and 255 values without extra addition (don't scale alpha twice). #Patch Set 3 : 80 columns #
Total comments: 3
Patch Set 4 : Catch 16-bit 565 blits and 32-bit non-SSE blits. #Patch Set 5 : 80 columns #
Total comments: 17
Patch Set 6 : Cover a few more functions with antialiasing. #Patch Set 7 : more complete #Patch Set 8 : addressed feedback, strengthened test #
Total comments: 2
MessagesTotal messages: 24
Thanks for looking into this. We know that the blending is inaccurate (a price we pay for speed). However, it should at least be getting the 0 and 255 cases correct. "What this means is that if you have a pixel P with alpha=255, and you blit a color C over it with alpha=127, the resulting alpha is: 255*(256-128)/256+127 = 255*128/256+127 = 254" I'm not sure the (256-128) term is correct. It's 256 - alpha, so it should be 256 - 127 (subtracting from 256 instead of 255 already adds 1; no need to do it again on the alpha). 255*(256-127)/256 + 127. = 255*129/256 + 127 = 255 Is that not what you're seeing?
Sign in to reply to this message.
On 2011/12/20 00:51:00, Stephen White wrote: > Thanks for looking into this. We know that the blending is inaccurate (a price > we pay for speed). However, it should at least be getting the 0 and 255 cases > correct. > > "What this means is that if you have a pixel P with alpha=255, and you blit a > color C over it with alpha=127, the resulting alpha is: > > 255*(256-128)/256+127 > = 255*128/256+127 > = 254" > > I'm not sure the (256-128) term is correct. It's 256 - alpha, so it should be > 256 - 127 (subtracting from 256 instead of 255 already adds 1; no need to do it > again on the alpha). > 255*(256-127)/256 + 127. > = 255*129/256 + 127 > = 255 > > Is that not what you're seeing? No because it subtracts a scaled up alpha from 256. So you get 256-128, due to subtracting the return value of SkAlpha255To256. If we stop using the SkAlpha255To256 then the results are correct for 0 and 255 cases again, and we don't incur any perf hit (for both cases ie. the count is multiple of 4, and not a multiple). I will adjust the patch to reflect this change instead, thanks.
Sign in to reply to this message.
On 2011/12/20 15:03:13, danakj wrote: > On 2011/12/20 00:51:00, Stephen White wrote: > > Thanks for looking into this. We know that the blending is inaccurate (a > price > > we pay for speed). However, it should at least be getting the 0 and 255 cases > > correct. > > > > "What this means is that if you have a pixel P with alpha=255, and you blit a > > color C over it with alpha=127, the resulting alpha is: > > > > 255*(256-128)/256+127 > > = 255*128/256+127 > > = 254" > > > > I'm not sure the (256-128) term is correct. It's 256 - alpha, so it should be > > 256 - 127 (subtracting from 256 instead of 255 already adds 1; no need to do > it > > again on the alpha). > > 255*(256-127)/256 + 127. > > = 255*129/256 + 127 > > = 255 > > > > Is that not what you're seeing? > > No because it subtracts a scaled up alpha from 256. So you get 256-128, due to > subtracting the return value of SkAlpha255To256. If we stop using the > SkAlpha255To256 then the results are correct for 0 and 255 cases again, and we > don't incur any perf hit (for both cases ie. the count is multiple of 4, and not > a multiple). I will adjust the patch to reflect this change instead, thanks. Ahh, I think that's it. Looks like the original Color32 (in SkBlitRow_D32.cpp) has this problem too. Instead of unsigned scale = 256 - SkAlpha255To256(colorA); it should be unsigned scale = 256 - colorA; Same fix in Color32_SSE2 should take care of it.
Sign in to reply to this message.
Can/should you do the same fix for the non-sse2 code path? http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp File tests/BlitRectTest.cpp (right): http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:115: Can you simplify this logic, and just store expected results (no need for blend -vs- no_blend, since you can trivially specify whatever alpha you want in the src (or dst) SkColor directly. That would remove the need for half of your k-loops, and simplify the code logic a fair amount. http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:123: } gSrcRec[] = { Not sure if it makes sense to have a zero-tolerance check for the dithered results, since skia makes no promise on exactly how it performs dithering. I wonder if its enough to check the alpha, or to check that the expected color is within +/- 1 of the non-dithered result... http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:152: reporter)) { Do you need SkDebugf if we can use the reporter for recording output?
Sign in to reply to this message.
On 2011/12/20 16:54:05, reed1 wrote: > Can/should you do the same fix for the non-sse2 code path? Done. There were similar issues in the 16-bit 565 code also that the test showed. This is fixed in SkSrcOver32To16() in SkColorPriv.h. > http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp > File tests/BlitRectTest.cpp (right): > > http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:115: > Can you simplify this logic, and just store expected results (no need for blend > -vs- no_blend, since you can trivially specify whatever alpha you want in the > src (or dst) SkColor directly. That would remove the need for half of your > k-loops, and simplify the code logic a fair amount. Thanks, done. Instead testing with/without platform-specific (SSE) opts and a shader. Also added a SkBlitRow function to force off platform-specific opts for testing purposes in SkBlitRow.h/SkBlitRow_D32.cpp > http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:123: } gSrcRec[] = { > Not sure if it makes sense to have a zero-tolerance check for the dithered > results, since skia makes no promise on exactly how it performs dithering. I > wonder if its enough to check the alpha, or to check that the expected color is > within +/- 1 of the non-dithered result... Removed dither as discussed. > http://codereview.appspot.com/5494076/diff/2002/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:152: reporter)) { > Do you need SkDebugf if we can use the reporter for recording output? It was handy during testing/debugging to tell which iteration of the loop failed. It was basically copied from BlitRowTest.cpp, can do it differently if you'd like.
Sign in to reply to this message.
There were a few functions escaping coverage in the last patch. I was going to say I couldn't justify the change in S32_Blend_BlitRow32, but now the tests cover it (and caught another similar problem in the SEE version S32_Blend_Blitrow32_SSE2). I got this extra coverage by drawing a line with anti-aliasing, and then just testing the alpha value. It was losing opaque-ness in the same way without this patch.
Sign in to reply to this message.
This is an important idea, but its a big change. 1. Lets beef up the unittest as much as possible to confirm our desired invariants: - src-alpha == 0 never changes anything - src-alpha == 0xFF always results in dst-alpha == 0xFF - dst-alpha == 0xFF is always left unchanged regardless of src-alpha - (are there others) Perhaps we could add this test right now, since its useful to review/tweak independent of this CL, and it will help document where our bugs are now. We can also beef it up by testing "real" shaders (bitmap or gradients), as skia sometimes is sneaky and will recognize that a shader is a color_shader, and take short cuts (though we should include color_shader as well). 2. Lets definitely preflight this or any similar CL against webkit's DRT before committing it, as I anticipate it may affect some expected images. Summary: good idea, but big enough to require more thinking/testing before we can commit. http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h File include/core/SkBlitRow.h (right): http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... include/core/SkBlitRow.h:77: Factory32 and ColorProcFactory. ... "The default setting is true." ... "This is only present for testing, and may be ignored in a production environment". http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... include/core/SkBlitRow.h:78: */ Skia static methods are Capitalized. http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h File include/core/SkColorPriv.h (left): http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h#ol... include/core/SkColorPriv.h:356: unsigned db = SkGetPackedB16(dst); This is a big change for a function used in so many places. Have you preflighted this against chrome and dumprendertree (layouttests) to see the extent of the changes? http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h#ol... include/core/SkColorPriv.h:362: db = (sb + SkMul16ShiftRound(db, isa, SK_B16_BITS)) >> (8 - SK_B16_BITS); why use '/' instead of >> have you confirmed that compilers generate exactly the same output for both? http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp File src/core/SkBlitRow_D32.cpp (right): http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp#ne... src/core/SkBlitRow_D32.cpp:29: unsigned src_scale = SkAlpha255To256(alpha); This change seems to break the invariant that (src_scale + dst_scale) == 256. Do you think that invariant is not valid? http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp File tests/BlitRectTest.cpp (right): http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:112: SkBitmap::kRGB_565_Config, Are these other two configs not testable, or failing? http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:121: uint16_t fResult16; Other interesting combinations to test (imho): src=0, dst=0, black, white src=0x80..., dst=0, black, white (in addition to your 0x7F...) On this 2nd one (0x80 or 0x7F) I'm not sure we care exactly if the answers come back as exactly 0x7F or 0x80 per-se, other than we care that drawing any src-alpha onto an dst-0xFF should always result in dst-0xFF. Similar think should apply to src-0x00, which should never change the dst regardless of its alpha or color values... http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:170: if (!shader) { Why is this test excluding shaders? http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:171: dstBM.eraseColor(gSrcRec[k].fDst); canvas.drawColor(paint.getColor()); is simpler
Sign in to reply to this message.
Thanks for review, I'll address the comments. But I've just been working on running this on DRT on linux to start. Tracking results here: https://docs.google.com/a/google.com/spreadsheet/ccc?key=0Ak8ECULDPzz2dHdpUXh... There are 2035 tests that break for linux cpu. 18 for linux gpu.
Sign in to reply to this message.
http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h File include/core/SkBlitRow.h (right): http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... include/core/SkBlitRow.h:77: Factory32 and ColorProcFactory. On 2011/12/21 22:21:03, reed1 wrote: > ... "The default setting is true." > ... "This is only present for testing, and may be ignored in a production > environment". Done. http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... include/core/SkBlitRow.h:78: */ On 2011/12/21 22:21:03, reed1 wrote: > Skia static methods are Capitalized. Done. http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h File include/core/SkColorPriv.h (left): http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h#ol... include/core/SkColorPriv.h:362: db = (sb + SkMul16ShiftRound(db, isa, SK_B16_BITS)) >> (8 - SK_B16_BITS); oh, I was copying the comment from SkMull6ShiftRound. changed to shifts. http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp File src/core/SkBlitRow_D32.cpp (right): http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp#ne... src/core/SkBlitRow_D32.cpp:29: unsigned src_scale = SkAlpha255To256(alpha); On 2011/12/21 22:21:03, reed1 wrote: > This change seems to break the invariant that (src_scale + dst_scale) == 256. Do > you think that invariant is not valid? I do.. In the old code, src_scale falls in the range [1,256], but dst_scale has a value in the range [0,255]. So you get: src*alpha256/256 + dst*alpha255/256 The dst is multiplied by at most 255 and divided by 256, so it doesn't contribute to the result properly. http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp File tests/BlitRectTest.cpp (right): http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:112: SkBitmap::kRGB_565_Config, On 2011/12/21 22:21:03, reed1 wrote: > Are these other two configs not testable, or failing? Oh, tbh I didn't try them, and just copied this from the BlitRow test, and left the comment in. I'll give them a try. http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:121: uint16_t fResult16; On 2011/12/21 22:21:03, reed1 wrote: > Other interesting combinations to test (imho): > > src=0, dst=0, black, white > src=0x80..., dst=0, black, white (in addition to your 0x7F...) > > On this 2nd one (0x80 or 0x7F) I'm not sure we care exactly if the answers come > back as exactly 0x7F or 0x80 per-se, other than we care that drawing any > src-alpha onto an dst-0xFF should always result in dst-0xFF. Similar think > should apply to src-0x00, which should never change the dst regardless of its > alpha or color values... Thanks I'll make our test cases more clear and complete. I think I agree on all of this. http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:170: if (!shader) { On 2011/12/21 22:21:03, reed1 wrote: > Why is this test excluding shaders? The draw function does not take a SkPaint so it can't be using a shader to do the drawing. http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... tests/BlitRectTest.cpp:171: dstBM.eraseColor(gSrcRec[k].fDst); On 2011/12/21 22:21:03, reed1 wrote: > canvas.drawColor(paint.getColor()); is simpler thanks.
Sign in to reply to this message.
On 2011/12/21 22:21:03, reed1 wrote: > This is an important idea, but its a big change. > > 1. Lets beef up the unittest as much as possible to confirm our desired > invariants: > - src-alpha == 0 never changes anything > - src-alpha == 0xFF always results in dst-alpha == 0xFF > - dst-alpha == 0xFF is always left unchanged regardless of src-alpha > - (are there others) Another invariant of sorts that I saw not honored would be that different code paths give the same results (eg. with/without a shader). Different versions of blending functions computed some things in different ways: 1. How to compute dst_scale (aka invScale). - Some code did 256 - <scaled source alpha> > Perhaps we could add this test right now, since its useful to review/tweak > independent of this CL, and it will help document where our bugs are now. > > We can also beef it up by testing "real" shaders (bitmap or gradients), as skia > sometimes is sneaky and will recognize that a shader is a color_shader, and take > short cuts (though we should include color_shader as well). > > 2. Lets definitely preflight this or any similar CL against webkit's DRT before > committing it, as I anticipate it may affect some expected images. > > Summary: good idea, but big enough to require more thinking/testing before we > can commit. > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h > File include/core/SkBlitRow.h (right): > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... > include/core/SkBlitRow.h:77: Factory32 and ColorProcFactory. > ... "The default setting is true." > ... "This is only present for testing, and may be ignored in a production > environment". > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... > include/core/SkBlitRow.h:78: */ > Skia static methods are Capitalized. > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h > File include/core/SkColorPriv.h (left): > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h#ol... > include/core/SkColorPriv.h:356: unsigned db = SkGetPackedB16(dst); > This is a big change for a function used in so many places. Have you preflighted > this against chrome and dumprendertree (layouttests) to see the extent of the > changes? > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h#ol... > include/core/SkColorPriv.h:362: db = (sb + SkMul16ShiftRound(db, isa, > SK_B16_BITS)) >> (8 - SK_B16_BITS); > why use '/' instead of >> > have you confirmed that compilers generate exactly the same output for both? > > http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp > File src/core/SkBlitRow_D32.cpp (right): > > http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp#ne... > src/core/SkBlitRow_D32.cpp:29: unsigned src_scale = SkAlpha255To256(alpha); > This change seems to break the invariant that (src_scale + dst_scale) == 256. Do > you think that invariant is not valid? > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp > File tests/BlitRectTest.cpp (right): > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:112: SkBitmap::kRGB_565_Config, > Are these other two configs not testable, or failing? > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:121: uint16_t fResult16; > Other interesting combinations to test (imho): > > src=0, dst=0, black, white > src=0x80..., dst=0, black, white (in addition to your 0x7F...) > > On this 2nd one (0x80 or 0x7F) I'm not sure we care exactly if the answers come > back as exactly 0x7F or 0x80 per-se, other than we care that drawing any > src-alpha onto an dst-0xFF should always result in dst-0xFF. Similar think > should apply to src-0x00, which should never change the dst regardless of its > alpha or color values... > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:170: if (!shader) { > Why is this test excluding shaders? > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:171: dstBM.eraseColor(gSrcRec[k].fDst); > canvas.drawColor(paint.getColor()); is simpler
Sign in to reply to this message.
On 2011/12/21 22:21:03, reed1 wrote: > This is an important idea, but its a big change. > > 1. Lets beef up the unittest as much as possible to confirm our desired > invariants: > - src-alpha == 0 never changes anything > - src-alpha == 0xFF always results in dst-alpha == 0xFF > - dst-alpha == 0xFF is always left unchanged regardless of src-alpha > - (are there others) Another invariant of sorts that I saw not honored would be that different code paths give the same results (eg. with/without a shader). Different versions of blending functions computed some things in different ways: 1. How to compute dst_scale (aka invScale). - Some code did 256 - <scaled source alpha> > Perhaps we could add this test right now, since its useful to review/tweak > independent of this CL, and it will help document where our bugs are now. > > We can also beef it up by testing "real" shaders (bitmap or gradients), as skia > sometimes is sneaky and will recognize that a shader is a color_shader, and take > short cuts (though we should include color_shader as well). > > 2. Lets definitely preflight this or any similar CL against webkit's DRT before > committing it, as I anticipate it may affect some expected images. > > Summary: good idea, but big enough to require more thinking/testing before we > can commit. > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h > File include/core/SkBlitRow.h (right): > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... > include/core/SkBlitRow.h:77: Factory32 and ColorProcFactory. > ... "The default setting is true." > ... "This is only present for testing, and may be ignored in a production > environment". > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkBlitRow.h#newc... > include/core/SkBlitRow.h:78: */ > Skia static methods are Capitalized. > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h > File include/core/SkColorPriv.h (left): > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h#ol... > include/core/SkColorPriv.h:356: unsigned db = SkGetPackedB16(dst); > This is a big change for a function used in so many places. Have you preflighted > this against chrome and dumprendertree (layouttests) to see the extent of the > changes? > > http://codereview.appspot.com/5494076/diff/5011/include/core/SkColorPriv.h#ol... > include/core/SkColorPriv.h:362: db = (sb + SkMul16ShiftRound(db, isa, > SK_B16_BITS)) >> (8 - SK_B16_BITS); > why use '/' instead of >> > have you confirmed that compilers generate exactly the same output for both? > > http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp > File src/core/SkBlitRow_D32.cpp (right): > > http://codereview.appspot.com/5494076/diff/5011/src/core/SkBlitRow_D32.cpp#ne... > src/core/SkBlitRow_D32.cpp:29: unsigned src_scale = SkAlpha255To256(alpha); > This change seems to break the invariant that (src_scale + dst_scale) == 256. Do > you think that invariant is not valid? > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp > File tests/BlitRectTest.cpp (right): > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:112: SkBitmap::kRGB_565_Config, > Are these other two configs not testable, or failing? > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:121: uint16_t fResult16; > Other interesting combinations to test (imho): > > src=0, dst=0, black, white > src=0x80..., dst=0, black, white (in addition to your 0x7F...) > > On this 2nd one (0x80 or 0x7F) I'm not sure we care exactly if the answers come > back as exactly 0x7F or 0x80 per-se, other than we care that drawing any > src-alpha onto an dst-0xFF should always result in dst-0xFF. Similar think > should apply to src-0x00, which should never change the dst regardless of its > alpha or color values... > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:170: if (!shader) { > Why is this test excluding shaders? > > http://codereview.appspot.com/5494076/diff/5011/tests/BlitRectTest.cpp#newcod... > tests/BlitRectTest.cpp:171: dstBM.eraseColor(gSrcRec[k].fDst); > canvas.drawColor(paint.getColor()); is simpler
Sign in to reply to this message.
Sorry, misclicked send there. On 2011/12/21 22:21:03, reed1 wrote: > This is an important idea, but its a big change. > > 1. Lets beef up the unittest as much as possible to confirm our desired > invariants: > - src-alpha == 0 never changes anything > - src-alpha == 0xFF always results in dst-alpha == 0xFF > - dst-alpha == 0xFF is always left unchanged regardless of src-alpha > - (are there others) Another invariant of sorts that I saw not honored would be that different code paths give the same results (eg. with/without a shader). Different versions of blending functions computed some things in different ways: 1. How to compute dst_scale (aka invScale). - Some code did 256 - <scaled source alpha> - Some code did 256 - <source alpha> - Some code did SkAlpha255To256(255 - <source alpha>) - Some code did SkAlpha255To256(255 - <scaled source alpha>) I went with the 2nd one. 2. How to blend SrcOver under RGBA4444 - Some code did src_color + (dst_color * dst_scale >> 4) - Some code did (src_color + dst_color * dst_scale) >> 4 I went with the 1st one. 3. How to blend with alpha under RGBA4444 - Some code did (src_color * src_scale >> 4) + (dst_color * dst_scale >> 4) - Some code did (src_color * src_scale + dst_color * dst_scale) >> 4 I went with the 1st one. We're getting consistent results now at least in all the code paths I've managed to hit for RGBA32, RGBA4444, and A8. I dropped RGB565 from the test because it doesn't have alpha in it anyways, and while it may be inconsistent too, I don't feel motivated to spend the time looking at that one at the moment as I'm really concerned for alpha cases. > Perhaps we could add this test right now, since its useful to review/tweak > independent of this CL, and it will help document where our bugs are now. I'm happy to do that if you want a broken unit test sitting in the skia tree for a bit (Is that going to make people miserable?). > We can also beef it up by testing "real" shaders (bitmap or gradients), as skia > sometimes is sneaky and will recognize that a shader is a color_shader, and take > short cuts (though we should include color_shader as well). I've added a linear gradient which gives slightly different color values than the color shader, but the alpha values are coming out correct, so I'm only testing the alpha values in the unit test and I'm happy with it. > 2. Lets definitely preflight this or any similar CL against webkit's DRT before > committing it, as I anticipate it may affect some expected images. We're going to break every layout test with anti-aliasing, or scrollbars on linux at least. I am a bit intimidated, but I will definitely ride this CL out to the end with some support. I'm not sure what will be the best way to get some rebaseline of this magnitude into webkit but I think this is an important change and the unit test will be super useful in the future.
Sign in to reply to this message.
On 2011/12/22 22:41:18, danakj wrote: > Different versions of blending functions computed some things in different ways: > > 1. How to compute dst_scale (aka invScale). > - Some code did 256 - <scaled source alpha> > - Some code did 256 - <source alpha> > - Some code did SkAlpha255To256(255 - <source alpha>) > - Some code did SkAlpha255To256(255 - <scaled source alpha>) > > I went with the 2nd one. I think #3 would actually be better (assuming it produces exactly the same instruction stream as #2). The reason is, it leaves us the option of switching all the code over to using an accurate blending mode with no change to the call sites. SkAlpha255To256 would become a no-op, and SkAlphaMul() could use SkDiv255Round(). I did this experimentally a long time ago, and although it does introduce a perf hit, I think it might be nice to leave the door open for the future. > 2. How to blend SrcOver under RGBA4444 > - Some code did src_color + (dst_color * dst_scale >> 4) > - Some code did (src_color + dst_color * dst_scale) >> 4 > > I went with the 1st one. > > 3. How to blend with alpha under RGBA4444 > - Some code did (src_color * src_scale >> 4) + (dst_color * dst_scale >> 4) > - Some code did (src_color * src_scale + dst_color * dst_scale) >> 4 > > I went with the 1st one. > > > We're getting consistent results now at least in all the code paths I've managed > to hit > for RGBA32, RGBA4444, and A8. I dropped RGB565 from the test because it doesn't > have > alpha in it anyways, and while it may be inconsistent too, I don't feel > motivated to > spend the time looking at that one at the moment as I'm really concerned for > alpha cases. > > > Perhaps we could add this test right now, since its useful to review/tweak > > independent of this CL, and it will help document where our bugs are now. > > I'm happy to do that if you want a broken unit test sitting in the skia tree for > a > bit (Is that going to make people miserable?). > > > We can also beef it up by testing "real" shaders (bitmap or gradients), as > skia > > sometimes is sneaky and will recognize that a shader is a color_shader, and > take > > short cuts (though we should include color_shader as well). > > I've added a linear gradient which gives slightly different color values than > the color > shader, but the alpha values are coming out correct, so I'm only testing the > alpha > values in the unit test and I'm happy with it. > > > 2. Lets definitely preflight this or any similar CL against webkit's DRT > before > > committing it, as I anticipate it may affect some expected images. > > We're going to break every layout test with anti-aliasing, or scrollbars on > linux at least. I am a bit intimidated, but I will definitely ride this CL out > to the end with some support. I'm not sure what will be the best way to get some > rebaseline of this magnitude into webkit but I think this is an important change > and the unit test will be super useful in the future. I've done this type of change once (for ~1500 tests), and let's just say it was a learning experience. It should be done on a weekend if possible. You'll also need a WebKit committer to land the baselines for you directly, since the WebKit commit queue is hopeless for a task of this magnitude. Finally, it should be done in manageable chunks. (In my case, I tried to land all the Linux baselines at once, and svn commit ran for 30 minutes, ran out of memory and failed!)
Sign in to reply to this message.
On 2011/12/23 16:16:07, Stephen White wrote: > On 2011/12/22 22:41:18, danakj wrote: > > Different versions of blending functions computed some things in different > ways: > > > > 1. How to compute dst_scale (aka invScale). > > - Some code did 256 - <scaled source alpha> > > - Some code did 256 - <source alpha> > > - Some code did SkAlpha255To256(255 - <source alpha>) > > - Some code did SkAlpha255To256(255 - <scaled source alpha>) > > > > I went with the 2nd one. > > I think #3 would actually be better (assuming it produces exactly the same > instruction stream as #2). The reason is, it leaves us the option of switching > all the code over to using an accurate blending mode with no change to the call > sites. SkAlpha255To256 would become a no-op, and SkAlphaMul() could use > SkDiv255Round(). I did this experimentally a long time ago, and although it > does introduce a perf hit, I think it might be nice to leave the door open for > the future. They are different in the 15/16 case, so it gave different values than in the 255/256 case. For that reason I chose #2 to make the 15/16 and 255/256 code the same modulo # of bit differences. I thought of one more clarification to the above last night also. When an external alpha (eg. AA) is being applied.. The <source alpha> is multiplied against it, and the external alpha /does/ get scaled. Something like.. if (aa_alpha < 255) src_scale = SkAlpha255To256(aa_alpha) dst_scale = 256 - SkAlphaMul(source_alpha, src_scale) result = hibits(src * src_scale) + hibits(dst * dst_scale) else dst_scale = 256 - source_alpha result = src + hibits(dst * dst_scale) > I've done this type of change once (for ~1500 tests), and let's just say it was > a learning experience. It should be done on a weekend if possible. You'll also > need a WebKit committer to land the baselines for you directly, since the WebKit > commit queue is hopeless for a task of this magnitude. Finally, it should be > done in manageable chunks. (In my case, I tried to land all the Linux baselines > at once, and svn commit ran for 30 minutes, ran out of memory and failed!) Hah.. I spoke with @abarth about this last night also and he had some good tips. I guess this is why he suggested 100 at a time. I'd be fine to do it on the weekend if I can get a committer willing to join me. We currently have two in the office here, so I'll talk to them. Thanks.
Sign in to reply to this message.
Is this accruate:
We want to standardize on the following macros/arithmatic for computing scaling
factors from alpha:
int SkAlphaToScale(int alpha) { return alpha + 1 }
int SkAlphaToInvScale(int alpha) { return 256 - alpha }
If so, can we
- create and use these in the CL (added to SkColorPriv.h I presume)
- write a unittest that shows that these maintain our invariants as mentioned
earlier
- src==0 -> no change to dst
- src==0xFF -> dst == src
- etc.
If so, then I think we could possibly stage this CL by first adding these new
functions, and writing the unittest. That would be a stand-alone CL that would
have no effect on existing call sites, but would slightly reduce the size of
this CL, and would allow several of us to write further tests/experiments...
Sign in to reply to this message.
FYI I believe this is affecting https://bugs.webkit.org/show_bug.cgi?id=77667 and https://bugs.webkit.org/show_bug.cgi?id=77478
Sign in to reply to this message.
On 2011/12/23 17:04:00, danakj wrote: > I thought of one more clarification to the above last night also. When an > external alpha (eg. AA) is being applied.. The <source alpha> is multiplied > against it, and the external alpha /does/ get scaled. Something like.. > > if (aa_alpha < 255) > src_scale = SkAlpha255To256(aa_alpha) > dst_scale = 256 - SkAlphaMul(source_alpha, src_scale) > result = hibits(src * src_scale) + hibits(dst * dst_scale) This choice seems wrong as it's scaling the src, and we should be using premultiplied alpha, and IIUC all computation should look like dst = src + dst * dstScale > else > dst_scale = 256 - source_alpha > result = src + hibits(dst * dst_scale)
Sign in to reply to this message.
On 2012/01/03 15:34:29, reed1 wrote:
> Is this accruate:
>
> We want to standardize on the following macros/arithmatic for computing
scaling
> factors from alpha:
>
> int SkAlphaToScale(int alpha) { return alpha + 1 }
> int SkAlphaToInvScale(int alpha) { return 256 - alpha }
>
> If so, can we
> - create and use these in the CL (added to SkColorPriv.h I presume)
> - write a unittest that shows that these maintain our invariants as mentioned
> earlier
> - src==0 -> no change to dst
> - src==0xFF -> dst == src
> - etc.
>
> If so, then I think we could possibly stage this CL by first adding these new
> functions, and writing the unittest. That would be a stand-alone CL that would
> have no effect on existing call sites, but would slightly reduce the size of
> this CL, and would allow several of us to write further tests/experiments...
SGTM
Sign in to reply to this message.
Lets do what we can to break up this CL, just to make it understandable. e.g. can we make a separate CL just for the UsePortableBlits option?
Sign in to reply to this message.
Moved to these: https://codereview.appspot.com/5752045/ - Just the scale functions and tests to demonstrate them https://codereview.appspot.com/5758043 - Correctness fixes with 32bit stuff behind a compile flag There's some more cleanup that can be done later, but this catches the correctness issues.
Sign in to reply to this message.
is this review dead?
Sign in to reply to this message.
1. I like the gUsePlatformOptsForFactory change. That perhaps should land on its own if it is still seen as useful. 2. The 4444 files are now gone, so this CL will need to be rebased https://codereview.appspot.com/5494076/diff/4024/include/core/SkColorPriv.h File include/core/SkColorPriv.h (right): https://codereview.appspot.com/5494076/diff/4024/include/core/SkColorPriv.h#n... include/core/SkColorPriv.h:358: unsigned scale = SkAlpha255To256(255 - SkGetPackedA32(src)); I presume the changes to this function is fixing a bug ... in chrome? How many of the skia GMs are affected? https://codereview.appspot.com/5494076/diff/4024/src/core/SkBlitRow_D4444.cpp File src/core/SkBlitRow_D4444.cpp (right): https://codereview.appspot.com/5494076/diff/4024/src/core/SkBlitRow_D4444.cpp... src/core/SkBlitRow_D4444.cpp:61: uint32_t src_expand = SkExpand32_4444(c); This file is now gone.
Sign in to reply to this message.
As per https://codereview.appspot.com/5494076/#msg21 this was moved to 2 other CLs. I'll look at rebasing them.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
