Fix shadow blurs of RGBA bitmaps

Shadow blurs of RGBA bitmaps currently apply the bitmap alpha after blurring, which leads to incorrect results.  Instead, we extract the alpha before the blur.  There was already a special case for A8 masks; this code simply extends it to ARGB8 masks with transform-only matrices.  This restriction is OK for blur masks, which are always drawn 1:1, but could be extended to scaled/rotated masks if required.

This patch does the raster case; an upcoming patch will handle the GPU case.

[TomH, 2012-04-05 14:25:00]

http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp
File gm/shadows.cpp (right):

http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp#newcode44
gm/shadows.cpp:44: canvas.drawRect(SkRect::MakeXYWH(0, 10, 10, 10), p);
IIRC we're trying to get away from drawing in constructors, and do it lazily in
onDraw().

[bsalomon, 2012-04-05 14:34:11]

On 2012/04/04 22:11:59, Stephen White wrote:

TBH I don't feel qualified to review this change. I know there was some
disagreement on blur semantics between you and Mike. IIRC the Chrome bug has
been around for a long time and therefore the fix isn't likely to be pushed to
M18. If that's right, I'd feel more comfortable sitting on this until Monday
when Mike is back and you guys can hash it out. Alternatively, if getting it in
this week is urgent I'd say let's put an #if SK_FIX_RGBA_BITMAP_BLURS_FOR_CHROME
around the changes.

[Stephen White, 2012-04-05 15:35:34]

On 2012/04/05 14:34:11, bsalomon wrote:
> On 2012/04/04 22:11:59, Stephen White wrote:
> 
> TBH I don't feel qualified to review this change. I know there was some
> disagreement on blur semantics between you and Mike. IIRC the Chrome bug has
> been around for a long time and therefore the fix isn't likely to be pushed to
> M18. If that's right, I'd feel more comfortable sitting on this until Monday
> when Mike is back and you guys can hash it out. Alternatively, if getting it
in
> this week is urgent I'd say let's put an #if
SK_FIX_RGBA_BITMAP_BLURS_FOR_CHROME
> around the changes.

No, there's no rush.  This more a patch for discussion.

[bsalomon, 2012-04-05 15:54:57]

On 2012/04/05 15:35:34, Stephen White wrote:
> On 2012/04/05 14:34:11, bsalomon wrote:
> > On 2012/04/04 22:11:59, Stephen White wrote:
> > 
> > TBH I don't feel qualified to review this change. I know there was some
> > disagreement on blur semantics between you and Mike. IIRC the Chrome bug has
> > been around for a long time and therefore the fix isn't likely to be pushed
to
> > M18. If that's right, I'd feel more comfortable sitting on this until Monday
> > when Mike is back and you guys can hash it out. Alternatively, if getting it
> in
> > this week is urgent I'd say let's put an #if
> SK_FIX_RGBA_BITMAP_BLURS_FOR_CHROME
> > around the changes.
> 
> No, there's no rush.  This more a patch for discussion.

Ok cool :)

It's a shame we have to copy out the alpha channel. I suppose adding a blitter
that only takes A from an RGBA source is a bit of an undertaking.

[Stephen White, 2012-04-05 17:20:10]

http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp
File gm/shadows.cpp (right):

http://codereview.appspot.com/5981063/diff/1/gm/shadows.cpp#newcode44
gm/shadows.cpp:44: canvas.drawRect(SkRect::MakeXYWH(0, 10, 10, 10), p);
On 2012/04/05 14:25:00, TomH wrote:
> IIRC we're trying to get away from drawing in constructors, and do it lazily
in
> onDraw().

Done.

[Stephen White, 2012-04-05 17:32:18]

On 2012/04/05 15:54:57, bsalomon wrote:
> It's a shame we have to copy out the alpha channel. I suppose adding a blitter
> that only takes A from an RGBA source is a bit of an undertaking.

I thought of that, but aside from the usual macro hell^H^H^H^Hfun of writing a
blitter, I was worried it might require multiple permutations, for the other
bits in the paint.  I could be wrong about that, though.

[Steve VanDeBogart, 2012-04-05 17:36:48]

On 2012/04/05 17:32:18, Stephen White wrote:
> On 2012/04/05 15:54:57, bsalomon wrote:
> > It's a shame we have to copy out the alpha channel. I suppose adding a
blitter
> > that only takes A from an RGBA source is a bit of an undertaking.
> 
> I thought of that, but aside from the usual macro hell^H^H^H^Hfun of writing a
> blitter, I was worried it might require multiple permutations, for the other
> bits in the paint.  I could be wrong about that, though.

FYI - PDF always has to extract the alpha channel to a separate mask image.

[reed1, 2012-04-10 14:06:20]

This is a tricky area.

    drawBitmap(bm, paing)

needs to always draw the same as

    drawRect(bm_bounds, paint_with_bm_shader)

and

    drawPath(bm_bounds_as_path, paint_with_bm_shader)

Does this CL preserve that invariant?

Bitmap shaders must be treated just like any shader (e.g. gradients), including
just a single color (which has to draw the same as SkColorShader), which are not
referenced until after the mask has been computed.

[Stephen White, 2012-04-10 15:58:56]

On 2012/04/10 14:06:20, reed1 wrote:
> This is a tricky area.
> 
>     drawBitmap(bm, paing)
> 
> needs to always draw the same as
> 
>     drawRect(bm_bounds, paint_with_bm_shader)
> 
> and
> 
>     drawPath(bm_bounds_as_path, paint_with_bm_shader)
> 
> Does this CL preserve that invariant?

That invariant does not hold today, even without my changes, since
drawBitmapAsMask() is only called in drawBitmap(), not drawPath() or drawRect().
 I'll file a bug once project hosting comes out of the cryochamber.

> Bitmap shaders must be treated just like any shader (e.g. gradients),
including
> just a single color (which has to draw the same as SkColorShader), which are
not
> referenced until after the mask has been computed.

[reed1, 2012-04-10 18:19:00]

Yes and no.

The invariant I stated doesn't hold, but alpha-only bitmaps are themselves
breaking other "rules", since they respect the paint's color (and shader), while
all other bitmaps do *not*.

[Stephen White, 2012-04-11 14:44:52]

On 2012/04/10 18:19:00, reed1 wrote:
> Yes and no.
> 
> The invariant I stated doesn't hold, but alpha-only bitmaps are themselves
> breaking other "rules", since they respect the paint's color (and shader),
while
> all other bitmaps do *not*.

One idea would be to have an SkMaskFilter::filterBitmap() function which handles
drawing of masked bitmaps, similar to how we have an SkMaskFilter::filterPath()
function for filtering paths.  Then any special-case logic could be put in
there.

Another option would be to pass along the original paint to 
SkMaskFilter::filterPath() (and SkDraw::DrawToMask), check for the presence of
an SkBitmapProcShader, and use a shader that modulates the primitive's mask with
the bitmap alpha at that point (maybe the existing SkBitmapProcShader would work
when drawing to a mask?  Not sure).  That would handle the SkBitmapProcShader
drawRect/drawPath cases too.

[reed1, 2012-04-11 14:59:53]

We could change skia's architecture to always sample the shader/color when
computing the mask, but that is a huge change, and one that will seriously
affect performance, and breaks the current feature that modulating the color's
alpha is always applied *after* the maskfilter (as the proposed change would
mean we applied it before the filter).

The current architecture offer's two distinct features:

1. filter the mask and then compose it with the color (SkMaskFilter)
2. filter the composed mask+color (SkImageFilter)

Does the current SkBlurImageFilter meet the current webkit need? If it is a
question of performance and/or not wanting to blur the color changes, can we add
an option to SkBlurImageFilter that only blurs the resulting alpha channel?

[Stephen White, 2012-04-11 15:46:43]

On 2012/04/11 14:59:53, reed1 wrote:
> We could change skia's architecture to always sample the shader/color when
> computing the mask, but that is a huge change, and one that will seriously
> affect performance, and breaks the current feature that modulating the color's
> alpha is always applied *after* the maskfilter (as the proposed change would
> mean we applied it before the filter).
> 
> The current architecture offer's two distinct features:
> 
> 1. filter the mask and then compose it with the color (SkMaskFilter)
> 2. filter the composed mask+color (SkImageFilter)
> 
> Does the current SkBlurImageFilter meet the current webkit need? If it is a
> question of performance and/or not wanting to blur the color changes, can we
add
> an option to SkBlurImageFilter that only blurs the resulting alpha channel?

Yes, we could do the work to extend image filters to A8, but even if we did,
we'd have to do all of the shadow passes differently at the WebKit level,
depending on whether there was a blur or not, whether the shadow alpha needed to
modulate the image color, etc.  This is all doable, but they are also cases that
SkBlurMaskFilter already handles.

Could we add a flag to the maskfilter to indicate that shaders should be
respected?  That would be a much smaller amount of work, and would make the
SkBlurMaskFilter more generally useful to other clients, IMHO.  It might also
mean we could get rid of the drawBitmapAsMask() workaround as well.

[junov1, 2012-04-11 17:16:22]

http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp#newcode1065
src/core/SkDraw.cpp:1065: for (int y = 0; y < height; y++) {
Do you really need your own pixel loop here? Looking at the code for
SkA8_Shader_Blitter, it seems to me like blitting RGBA to A8 could be done by
setting an bitmap shader on the paint. I think doing that blit directly would
eliminate the need for an intermediate copy in 'mask'.

[junov1, 2012-04-11 17:24:34]

http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

http://codereview.appspot.com/5981063/diff/8001/src/core/SkDraw.cpp#newcode1065
src/core/SkDraw.cpp:1065: for (int y = 0; y < height; y++) {
On 2012/04/11 17:16:22, junov1 wrote:
> Do you really need your own pixel loop here? Looking at the code for
> SkA8_Shader_Blitter, it seems to me like blitting RGBA to A8 could be done by
> setting an bitmap shader on the paint. I think doing that blit directly would
> eliminate the need for an intermediate copy in 'mask'.
Never mind that comment I see why you absolutely need an SkMask.  Argh.

[Stephen White, 2012-04-11 20:02:15]

This version handles SkBitmapProcShader, by deferring the blitter choice and
passing the shader through to draw_into_mask().

Note that this implementation is probably all kinds of wrong (and needs a flag);
I just wanted to see how hard it would be to get it working.  

I do think the change in SkBlitter_A8.cpp is good, and perhaps should be pulled
out:  if there's no xfermode, SkA8_Shader_Blitter::blitMask() does nothing!
(took me a while to figure that one out..)

[Stephen White, 2012-04-11 20:12:32]

On 2012/04/11 20:02:15, Stephen White wrote:

> I do think the change in SkBlitter_A8.cpp is good, and perhaps should be
pulled
> out:  if there's no xfermode, SkA8_Shader_Blitter::blitMask() does nothing!
> (took me a while to figure that one out..)

... although it should probably be doing a blend, instead of a straight copy.