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

Issue 5758043: Blend alpha correctly for ARGB32, ARGB16 (4444), and A8 modes.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 6 months ago by danakj
Modified:
8 years, 4 months ago
Reviewers:
reed, epoger, Stephen White, ojan
CC:
backer_chromium.org, senorblanco, piman
Base URL:
https://skia.googlecode.com/svn/trunk
Visibility:
Public.

Description

Blend alpha correctly for ARGB32, ARGB16 (4444), and A8 modes. I have the changes that affect layout tests behind SK_BLEND_CORRECTLY, which can be enabled with the skia_blend_correctly=1 gyp define. BUG=420 (crbug.com/113309) TEST=Blend8888Test, Blend4444Test, BlendA8Test

Patch Set 1 #

Patch Set 2 : adds thorough unit tests for 32bit and 16bit ARGB #

Patch Set 3 : add A8 tests and fixes they caught #

Patch Set 4 : remove BlitRectTest, Blend*Test supercedes it #

Patch Set 5 : remove BlitRectTest, Blend*Test supercedes it #

Patch Set 6 : don't include 5752045 in this CL #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+1278 lines, -42 lines) Patch
M gyp/common.gypi View 1 chunk +10 lines, -0 lines 1 comment Download
M gyp/tests.gyp View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M include/core/SkBlitRow.h View 1 1 chunk +7 lines, -0 lines 1 comment Download
M include/core/SkColorPriv.h View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M src/core/SkBlitRow_D32.cpp View 1 5 chunks +18 lines, -3 lines 0 comments Download
M src/core/SkBlitRow_D4444.cpp View 1 2 chunks +7 lines, -7 lines 0 comments Download
M src/core/SkBlitter_4444.cpp View 1 9 chunks +19 lines, -24 lines 0 comments Download
M src/core/SkBlitter_A8.cpp View 1 2 8 chunks +14 lines, -8 lines 0 comments Download
M src/core/SkBlitter_ARGB32.cpp View 1 2 chunks +10 lines, -0 lines 2 comments Download
M src/core/SkCoreBlitters.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M src/opts/SkBlitRow_opts_SSE2.cpp View 1 2 chunks +8 lines, -0 lines 0 comments Download
A tests/Blend4444Test.cpp View 1 1 chunk +398 lines, -0 lines 0 comments Download
A tests/Blend8888Test.cpp View 1 1 chunk +400 lines, -0 lines 0 comments Download
A tests/BlendA8Test.cpp View 1 2 1 chunk +372 lines, -0 lines 0 comments Download

Messages

Total messages: 6
danakj
Hi Mike, Here's a thorough start to a set of unit tests to cover the ...
8 years, 6 months ago (2012-03-07 00:21:25 UTC) #1
danakj
FYI this depends on https://codereview.appspot.com/5752045/
8 years, 6 months ago (2012-03-07 00:21:49 UTC) #2
danakj
On 2012/03/07 00:21:49, danakj wrote: > FYI this depends on https://codereview.appspot.com/5752045/ Ah I think I ...
8 years, 6 months ago (2012-03-09 16:43:03 UTC) #3
ojan
Poke reviewers. This (hopefully) addresses a huge source of layout test failures on Mac.
8 years, 5 months ago (2012-04-13 22:42:09 UTC) #4
epoger
Looks OK to me, but Mike should definitely take a look. Mike???
8 years, 5 months ago (2012-04-16 13:24:51 UTC) #5
Stephen White
8 years, 4 months ago (2012-05-08 14:50:49 UTC) #6
Some vacation-delayed comments, and a ping for Mike:

Mike: ping!   :)

https://codereview.appspot.com/5758043/diff/3018/gyp/common.gypi
File gyp/common.gypi (right):

https://codereview.appspot.com/5758043/diff/3018/gyp/common.gypi#newcode53
gyp/common.gypi:53: 'direct_dependent_settings': {
Just wondering:  is this necessary?  Are there other targets which depend on
this one which require this flag to be set?  It looks like most of the flags set
in chrome's skia.gyp don't require direct_dependent_settings to be used (except
for GR_DLL and SKIA_DLL in the shared_library build).

https://codereview.appspot.com/5758043/diff/3018/include/core/SkBlitRow.h
File include/core/SkBlitRow.h (right):

https://codereview.appspot.com/5758043/diff/3018/include/core/SkBlitRow.h#new...
include/core/SkBlitRow.h:77: Factory32 and ColorProcFactory. The default setting
is true.
The default value in the .cpp file seems to be false, disagreeing with this
comment, although perhaps it's set to true programmatically elsewhere.

Bikeshed:  could we invert the logic, and name this something like
"DisablePlatformOpts", so that it could actually default to false?  It might be
clearer that way.

https://codereview.appspot.com/5758043/diff/3018/src/core/SkBlitter_ARGB32.cpp
File src/core/SkBlitter_ARGB32.cpp (right):

https://codereview.appspot.com/5758043/diff/3018/src/core/SkBlitter_ARGB32.cp...
src/core/SkBlitter_ARGB32.cpp:53: fSrcB = SkAlphaMul(SkColorGetB(color), scale);
It looks like fSrc[RGB] are not used outside the constructor.  Maybe they should
be locals instead?  (Not your fault, just trying to clean this up).  They should
have their "f" removed also (srcR, srcG, srcB) once they're no longer member
vars.

https://codereview.appspot.com/5758043/diff/3018/src/core/SkBlitter_ARGB32.cp...
src/core/SkBlitter_ARGB32.cpp:56: #ifndef SK_BLEND_CORRECTLY
Perhaps you could also merge these two #ifdef blocks, for clarity.
Sign in to reply to this message.

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