Fixing shadow rendering in Skia

Added support for alpha mask-based shadow rendering under ARGB bitmaps.
Improved mask blurring to produce higher quality blurred shadows using three-pass box blurring

[junov, 2011-02-14 15:57:00]

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

http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp#newcode1134
src/core/SkDraw.cpp:1134: bitmap.extractAlpha(&tmpBitmap);
There is an opportunity to optimize here by implementing a version of
drawBitmapAsMask that supports ARGB (and only uses alpha).  That would avoid
creating a temp image.  For now, this way is low hanging fruit.

[Stephen White, 2011-02-14 16:20:31]

Some style nits, and I'd like to see the bool param to SkBlurMask::Blur() go
away.
Other than that, looks good.

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

http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp#newcode1132
src/core/SkDraw.cpp:1132: } else if (paint.getMaskFilter() && bitmap.getConfig()
== SkBitmap::kARGB_8888_Config) {
What about other configs?  Should those also be handled here?  (Doesn't have to
be in this CL; I'm just curious).

http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp#newcode1134
src/core/SkDraw.cpp:1134: bitmap.extractAlpha(&tmpBitmap);
On 2011/02/14 15:57:00, junov wrote:
> There is an opportunity to optimize here by implementing a version of
> drawBitmapAsMask that supports ARGB (and only uses alpha).  That would avoid
> creating a temp image.  For now, this way is low hanging fruit.

Please add this as a comment in the code.

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurDrawLooper.cpp
File src/effects/SkBlurDrawLooper.cpp (right):

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurDrawLooper.cpp...
src/effects/SkBlurDrawLooper.cpp:35: fColorFilter =
SkColorFilter::CreateModeFilter(color, SkXfermode::kDst_Mode);
The comment says SrcIn, but the code says Dst.  Who's right here?

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp#newco...
src/effects/SkBlurMask.cpp:249: SkScalar radius, Style style, bool highQuality)
Anonymous bools tend to be hard to read at the call site.  Could we just pass
all the BlurFlags down here, and pull out the highQuality part?

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp#newco...
src/effects/SkBlurMask.cpp:259: SkScalar passRadius =
SkScalarDiv(radius,SkScalarSqrt(SkIntToScalar(passCount)));
Please use a space after the comma here.

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp#newco...
src/effects/SkBlurMask.cpp:295: SkAutoTMalloc<uint32_t> storage((sw + 2 *
(passCount - 1) * rx + 1) * (sh + 2 * (passCount - 1) * ry + 1));
I'm guessing this is to allow space for the padding?  A comment might be
helpful.

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp#newco...
src/effects/SkBlurMask.cpp:313: dump_sum_buffer(sumBuffer, tmp_sw, tmp_sh);
Is this still needed?  Callee is #if 0'ed.

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp#newco...
src/effects/SkBlurMask.cpp:323: dump_sum_buffer(sumBuffer, tmp_sw, tmp_sh);
Ibid.

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp#newco...
src/effects/SkBlurMask.cpp:343: dp + passCount*(rx + ry*dst->fRowBytes),
dst->fRowBytes,
Please add space around * (seems to be the local convention).

http://codereview.appspot.com/4174049/diff/1/src/effects/SkBlurMask.cpp#newco...
src/effects/SkBlurMask.cpp:347: clamp_with_orig(dp + passCount*(rx +
ry*dst->fRowBytes), dst->fRowBytes,
Ibid.

[reed1, 2011-02-14 16:45:47]

Two meta-comments:

1. Lets definitely add some tests/samples for this new high-quality mode as part
of the checkin. Do you have some screenshots showing low and high quality
side-by-side?

2. Lets make the "feature" in SkDraw.cpp a separate consideration. I'm not sure
we can do this automatically, as it may change existing behavior in a way that
might break some apps (thinking of android).

[reed1, 2011-02-14 16:46:05]

3. How much better does a real gaussian implementation look?

[junov, 2011-02-14 20:24:34]

According to the literature, a three-pass box filter is guaranteed to yield
discrepancies of less than 3% compared to an actual Gaussian. According to the
central limit theorem, the more passes you add, the closer you get to an
Authentic Gaussian.  Three-pass seems to be the generally accepted sweet-spot. 
It is what is suggested in the SVG and CSS specifications.

On 2011/02/14 16:46:05, reed1 wrote:
> 3. How much better does a real gaussian implementation look?

[junov, 2011-02-14 20:29:53]

For comment 2: I guess what your are saying is that the choice of the
highQuality path should be moved up to WebKit (i.e. GraphicsContextSkia.cpp). 
Correct?

On 2011/02/14 16:45:47, reed1 wrote:
> Two meta-comments:
> 
> 1. Lets definitely add some tests/samples for this new high-quality mode as
part
> of the checkin. Do you have some screenshots showing low and high quality
> side-by-side?
> 
> 2. Lets make the "feature" in SkDraw.cpp a separate consideration. I'm not
sure
> we can do this automatically, as it may change existing behavior in a way that
> might break some apps (thinking of android).

[junov, 2011-02-14 20:46:08]

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

http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp#newcode1132
src/core/SkDraw.cpp:1132: } else if (paint.getMaskFilter() && bitmap.getConfig()
== SkBitmap::kARGB_8888_Config) {
On 2011/02/14 16:20:31, Stephen White wrote:
> What about other configs?  Should those also be handled here?  (Doesn't have
to
> be in this CL; I'm just curious).
I have not tried, but the available functionality for blitting and manipulating
the 1-bit A format and kARGB_4444 is insufficient for what we need to do to draw
shadows. A lot of work would be necessary to support them.  It is not clear
under which circumstances they would be necessary.  If any of those formats were
ever encountered here, the fallback path below would produce an opaque mask
corresponding to the bitmap rectangle (ignores alpha), which is not
catastrophic.  That is the path that was taken for ARGB_8888 before this fix.

[reed1, 2011-02-15 19:51:39]

I think we may regress some clients if we change the existing path taken for
ARGB_8888. I would like to revert your change to SkDraw.cpp in the current CL,
so we can consider that separately from the Blur changes.

On 2011/02/14 20:46:08, junov wrote:
> http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp
> File src/core/SkDraw.cpp (right):
> 
> http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp#newcode1132
> src/core/SkDraw.cpp:1132: } else if (paint.getMaskFilter() &&
bitmap.getConfig()
> == SkBitmap::kARGB_8888_Config) {
> On 2011/02/14 16:20:31, Stephen White wrote:
> > What about other configs?  Should those also be handled here?  (Doesn't have
> to
> > be in this CL; I'm just curious).
> I have not tried, but the available functionality for blitting and
manipulating
> the 1-bit A format and kARGB_4444 is insufficient for what we need to do to
draw
> shadows. A lot of work would be necessary to support them.  It is not clear
> under which circumstances they would be necessary.  If any of those formats
were
> ever encountered here, the fallback path below would produce an opaque mask
> corresponding to the bitmap rectangle (ignores alpha), which is not
> catastrophic.  That is the path that was taken for ARGB_8888 before this fix.

[reed1, 2011-02-15 21:11:12]

I've run the patch on SampleApp, and the results look nice!

[junov, 2011-02-16 14:51:44]

Fair enough, there are actually two independent bug fixes in that CL.  The
change to the ARGB_8888 path was to fix a failing layout test that was
causing shadows behind bitmap images to not be rendered.

On Tue, Feb 15, 2011 at 2:51 PM, <reed@google.com> wrote:

> I think we may regress some clients if we change the existing path taken
> for ARGB_8888. I would like to revert your change to SkDraw.cpp in the
> current CL, so we can consider that separately from the Blur changes.
>
>
> On 2011/02/14 20:46:08, junov wrote:
>
>> http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp
>> File src/core/SkDraw.cpp (right):
>>
>
>
>
> http://codereview.appspot.com/4174049/diff/1/src/core/SkDraw.cpp#newcode1132
>
>> src/core/SkDraw.cpp:1132: } else if (paint.getMaskFilter() &&
>>
> bitmap.getConfig()
>
>> == SkBitmap::kARGB_8888_Config) {
>> On 2011/02/14 16:20:31, Stephen White wrote:
>> > What about other configs?  Should those also be handled here?
>>
> (Doesn't have
>
>> to
>> > be in this CL; I'm just curious).
>> I have not tried, but the available functionality for blitting and
>>
> manipulating
>
>> the 1-bit A format and kARGB_4444 is insufficient for what we need to
>>
> do to draw
>
>> shadows. A lot of work would be necessary to support them.  It is not
>>
> clear
>
>> under which circumstances they would be necessary.  If any of those
>>
> formats were
>
>> ever encountered here, the fallback path below would produce an opaque
>>
> mask
>
>> corresponding to the bitmap rectangle (ignores alpha), which is not
>> catastrophic.  That is the path that was taken for ARGB_8888 before
>>
> this fix.
>
>
>
> http://codereview.appspot.com/4174049/
>

[junov, 2011-02-16 22:07:28]

Reverted SkDraw.cpp, reacted to all comments, added test set for gm

[junov, 2011-02-16 22:10:11]

http://codereview.appspot.com/4174049/diff/11002/gm/shadows.cpp
File gm/shadows.cpp (right):

http://codereview.appspot.com/4174049/diff/11002/gm/shadows.cpp#newcode18
gm/shadows.cpp:18: fPaint.setStrokeWidth(SkIntToScalar(4));
Oops, indent booboo. I'm on it

[Stephen White, 2011-02-16 22:28:23]

http://codereview.appspot.com/4174049/diff/11002/src/effects/SkBlurDrawLooper...
File src/effects/SkBlurDrawLooper.cpp (right):

http://codereview.appspot.com/4174049/diff/11002/src/effects/SkBlurDrawLooper...
src/effects/SkBlurDrawLooper.cpp:28: {
Looks like skia code style is to omit the braces for single-line if-else clauses
(when in Rome..).

http://codereview.appspot.com/4174049/diff/11002/src/effects/SkBlurMask.h
File src/effects/SkBlurMask.h (right):

http://codereview.appspot.com/4174049/diff/11002/src/effects/SkBlurMask.h#new...
src/effects/SkBlurMask.h:39: static bool Blur(SkMask* dst, const SkMask& src,
SkScalar radius, Style, Quality quality = kLow_Quality);
Hmm, default param.  I wonder if we should bite the bullet and break compat (and
remove the default).  Mike, what do you think?  Is this function called a lot
from outside Skia?

[reed1, 2011-02-17 14:07:17]

LGTM

The code base is inconsistent in its { } usage, so I won't focus on that in this
CL. Going forward the style is:

1. always use { } for "if" and "while" and "for" and "else"
2. the braces are always on the same line as their keyword (e.g. } else { )