Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(399)

Issue 5494076: The SSE32 BlitRow function mistakenly degrades opaque pixels to be non-opaque when blending (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 1 month ago by danakj
Modified:
7 years, 5 months ago
Reviewers:
Stephen White, reed1
CC:
backer_chromium.org, piman, enne, skia-review_googlegroups.com, reed, reveman
Base URL:
https://skia.googlecode.com/svn/trunk
Visibility:
Public.

Description

The 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+306 lines, -61 lines) Patch
M gyp/tests.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M include/core/SkBlitRow.h View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M include/core/SkColorPriv.h View 1 2 3 4 5 6 1 chunk +4 lines, -4 lines 1 comment Download
M src/core/SkBlitRow_D32.cpp View 1 2 3 4 5 6 5 chunks +11 lines, -4 lines 0 comments Download
M src/core/SkBlitRow_D4444.cpp View 1 2 3 4 5 6 2 chunks +8 lines, -8 lines 1 comment Download
M src/core/SkBlitter_4444.cpp View 1 2 3 4 5 6 7 chunks +27 lines, -34 lines 0 comments Download
M src/core/SkBlitter_A8.cpp View 1 2 3 4 5 6 5 chunks +7 lines, -8 lines 0 comments Download
M src/core/SkBlitter_ARGB32.cpp View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M src/opts/SkBlitRow_opts_SSE2.cpp View 1 2 3 4 5 2 chunks +4 lines, -2 lines 0 comments Download
A tests/BlitRectTest.cpp View 1 2 3 4 5 6 1 chunk +236 lines, -0 lines 0 comments Download

Messages

Total messages: 24
danakj
9 years, 1 month ago (2011-12-19 23:47:15 UTC) #1
Stephen White
Thanks for looking into this. We know that the blending is inaccurate (a price we ...
9 years, 1 month ago (2011-12-20 00:51:00 UTC) #2
danakj
On 2011/12/20 00:51:00, Stephen White wrote: > Thanks for looking into this. We know that ...
9 years, 1 month ago (2011-12-20 15:03:13 UTC) #3
Stephen White
On 2011/12/20 15:03:13, danakj wrote: > On 2011/12/20 00:51:00, Stephen White wrote: > > Thanks ...
9 years, 1 month ago (2011-12-20 15:47:28 UTC) #4
reed1
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): ...
9 years, 1 month ago (2011-12-20 16:54:05 UTC) #5
danakj
On 2011/12/20 16:54:05, reed1 wrote: > Can/should you do the same fix for the non-sse2 ...
9 years, 1 month ago (2011-12-20 20:30:37 UTC) #6
danakj
There were a few functions escaping coverage in the last patch. I was going to ...
9 years, 1 month ago (2011-12-20 21:20:56 UTC) #7
reed1
This is an important idea, but its a big change. 1. Lets beef up the ...
9 years, 1 month ago (2011-12-21 22:21:03 UTC) #8
danakj
Thanks for review, I'll address the comments. But I've just been working on running this ...
9 years, 1 month ago (2011-12-21 23:00:20 UTC) #9
danakj
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#newcode77 include/core/SkBlitRow.h:77: Factory32 and ColorProcFactory. On 2011/12/21 22:21:03, reed1 wrote: > ...
9 years, 1 month ago (2011-12-21 23:48:23 UTC) #10
danakj
On 2011/12/21 22:21:03, reed1 wrote: > This is an important idea, but its a big ...
9 years, 1 month ago (2011-12-22 22:31:22 UTC) #11
danakj
On 2011/12/21 22:21:03, reed1 wrote: > This is an important idea, but its a big ...
9 years, 1 month ago (2011-12-22 22:31:22 UTC) #12
danakj
Sorry, misclicked send there. On 2011/12/21 22:21:03, reed1 wrote: > This is an important idea, ...
9 years, 1 month ago (2011-12-22 22:41:18 UTC) #13
Stephen White
On 2011/12/22 22:41:18, danakj wrote: > Different versions of blending functions computed some things in ...
9 years, 1 month ago (2011-12-23 16:16:07 UTC) #14
danakj
On 2011/12/23 16:16:07, Stephen White wrote: > On 2011/12/22 22:41:18, danakj wrote: > > Different ...
9 years, 1 month ago (2011-12-23 17:04:00 UTC) #15
reed1
Is this accruate: We want to standardize on the following macros/arithmatic for computing scaling factors ...
9 years ago (2012-01-03 15:34:29 UTC) #16
danakj
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
8 years, 12 months ago (2012-02-02 21:09:18 UTC) #17
danakj
On 2011/12/23 17:04:00, danakj wrote: > I thought of one more clarification to the above ...
8 years, 10 months ago (2012-03-05 20:47:56 UTC) #18
danakj
On 2012/01/03 15:34:29, reed1 wrote: > Is this accruate: > > We want to standardize ...
8 years, 10 months ago (2012-03-05 20:48:30 UTC) #19
reed1
Lets do what we can to break up this CL, just to make it understandable. ...
8 years, 10 months ago (2012-03-05 20:55:13 UTC) #20
danakj
Moved to these: https://codereview.appspot.com/5752045/ - Just the scale functions and tests to demonstrate them https://codereview.appspot.com/5758043 ...
8 years, 10 months ago (2012-03-09 16:39:37 UTC) #21
nduca
is this review dead?
7 years, 5 months ago (2013-08-04 19:33:29 UTC) #22
reed1
1. I like the gUsePlatformOptsForFactory change. That perhaps should land on its own if it ...
7 years, 5 months ago (2013-08-05 21:06:54 UTC) #23
danakj
7 years, 5 months ago (2013-08-06 20:51:58 UTC) #24
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b