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

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

[danakj, 2012-03-07 00:21:25]

Hi Mike,

Here's a thorough start to a set of unit tests to cover the various blit
functions. They test for our invariants, and that different code paths produce
the same results.

Included are a minimal set of changes required to make these tests pass.

The types of problems fixed are:
1. 256 - scaled alpha => 256 - alpha using SkAlphaToInvScale()
2. (src + dst) >> bits => (src >> bits) + (dst >> bits)
3. Consistent application of alpha multiplication.
4. Compute scale using the actual source value being blended to get accurate
results.

I have the 32bit changes behind a build flag (SK_BLEND_CORRECTLY) so they won't
break any layout tests. The others don't seem to affect layout tests at all.

I hope you like the tests! I think they are a far shot better than the ones I
previously proposed.

I'd like to follow up later with a CL to clean up the code to use
SkAlphaToScale/SkAlphaToInvScale everywhere consistently (without changing
functionality).

[danakj, 2012-03-07 00:21:49]

FYI this depends on https://codereview.appspot.com/5752045/

[danakj, 2012-03-09 16:43:03]

On 2012/03/07 00:21:49, danakj wrote:
> FYI this depends on https://codereview.appspot.com/5752045/

Ah I think I just got your comment on the previous CL. If you like I can pull
out the gUsePlatformOptsForFactory stuff, though I feel like this is pretty
understandable now, with the bare minimum changes. Not sure if that's just cuz
I've been staring at this stuff so long though.

[ojan, 2012-04-13 22:42:09]

Poke reviewers. This (hopefully) addresses a huge source of layout test failures
on Mac.

[epoger, 2012-04-16 13:24:51]

Looks OK to me, but Mike should definitely take a look.  Mike???

[Stephen White, 2012-05-08 14:50:49]

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.

[droidultra574, 2025-03-08 12:01:42]

For you information purposes only

[droidultra574, 2025-03-09 07:19:16]

Sorry shhhhh

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': {
On 2012/05/08 14:50:49, Stephen White wrote:
> 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).

Acknowledged.

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.
On 2012/05/08 14:50:49, Stephen White wrote:
> 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.

Done.

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);
On 2012/05/08 14:50:49, Stephen White wrote:
> 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.

Acknowledged.

https://codereview.appspot.com/5758043/diff/3018/src/core/SkBlitter_ARGB32.cp...
src/core/SkBlitter_ARGB32.cpp:56: #ifndef SK_BLEND_CORRECTLY
On 2012/05/08 14:50:49, Stephen White wrote:
> Perhaps you could also merge these two #ifdef blocks, for clarity.

Acknowledged.

[droidultra574, 2025-03-11 07:14:52]

It is a good thing

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.
On 2012/05/08 14:50:49, Stephen White wrote:
> 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.

Acknowledged.