Option for BlurDrawLooper to not apply canvas transform to blur offset

The HTML5 canvas client of BlurDrawLooper needs the option to not apply the canvas transform to the blur offset. 

see http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#shadows - "The shadowOffsetX and shadowOffsetY attributes specify the distance that the shadow will be offset in the positive horizontal and positive vertical distance respectively. Their values are in coordinate space units. They are not affected by the current transformation matrix."

This patch is part of fixing http://code.google.com/p/chromium/issues/detail?id=64647.

[mikelawther, 2010-12-02 00:47:33]

Hi guys - this is my first Skia patch :) Sorry if I've included the wrong
reviewers - if so, can you please add/remove reviewers as appropriate?

[Stephen White, 2010-12-02 15:13:53]

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

http://codereview.appspot.com/3391041/diff/1/src/effects/SkBlurDrawLooper.cpp...
src/effects/SkBlurDrawLooper.cpp:20: : fBlurFlags(flags)
I think this flag should be read from the SkFlattenableReadBuffer (and written
to it in flatten()), not passed as an argument to the constructor.  That will
make this version incompatible with previously-serialized data, of course, but I
don't see a versioning mechanism.  Neither Chrome nor Android use this code
path, but I think we should keep the API consistent.

http://codereview.appspot.com/3391041/diff/1/src/effects/SkBlurDrawLooper.cpp...
src/effects/SkBlurDrawLooper.cpp:66: fCanvas->translate(fDx /
current.getScaleX(), fDy / current.getScaleY());
Potential division by zero here.

Also, this removes the scale component, but what about a rotation component? 
How about a non-affine transform?  I don't think getScaleX() is valid in that
case.

Perhaps it would be more general to reset the current transform to identity,
apply the translation, then re-apply the matrix (or equivalently, left-multiply
the current transform with a translation matrix).  Would that work?

[wjmaclean, 2010-12-02 15:29:48]

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

http://codereview.appspot.com/3391041/diff/1/src/effects/SkBlurDrawLooper.cpp...
src/effects/SkBlurDrawLooper.cpp:66: fCanvas->translate(fDx /
current.getScaleX(), fDy / current.getScaleY());
Agreed, I think it would be much safer to apply the translation matrix from the
left if possible.

[mikelawther, 2010-12-03 04:43:22]

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

http://codereview.appspot.com/3391041/diff/1/src/effects/SkBlurDrawLooper.cpp...
src/effects/SkBlurDrawLooper.cpp:20: : fBlurFlags(flags)
On 2010/12/02 15:13:53, Stephen White wrote:
> I think this flag should be read from the SkFlattenableReadBuffer (and written
> to it in flatten()), not passed as an argument to the constructor.  That will
> make this version incompatible with previously-serialized data, of course, but
I
> don't see a versioning mechanism.  Neither Chrome nor Android use this code
> path, but I think we should keep the API consistent.

Good point. Done.

http://codereview.appspot.com/3391041/diff/1/src/effects/SkBlurDrawLooper.cpp...
src/effects/SkBlurDrawLooper.cpp:66: fCanvas->translate(fDx /
current.getScaleX(), fDy / current.getScaleY());
On 2010/12/02 15:29:48, wjmaclean wrote:
> Agreed, I think it would be much safer to apply the translation matrix from
the
> left if possible.

Excellent suggestion. I've done it with Skia's postTranslate function - much
better.

I realised that I also had to not transform the blur radius. Since that actually
gets transformed way down in SkBlurMaskFilterImpl::filterMask() - I calculated
the reciprocal of the transformed radius here and create a new FilterMask. Feels
a little hacky :(

[mikelawther, 2010-12-03 05:19:37]

btw - here is the corresponding Webkit patch that uses this code:
https://bugs.webkit.org/show_bug.cgi?id=50437

[Stephen White, 2010-12-03 20:22:08]

Looking much better; thanks for the postTranslate().

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

http://codereview.appspot.com/3391041/diff/6001/src/effects/SkBlurDrawLooper....
src/effects/SkBlurDrawLooper.cpp:71: untransformedRadius,
SkBlurMaskFilter::kNormal_BlurStyle));
Hmm, this is a bit unfortunate.  I don't know how often next() is called, but it
might be thrashing the heap a bit to constant re-create the filter.

Would it make any sense to create a similar flag for SkBlurMaskFilter, and make
it responsible for doing the transform / no transform adjustment instead?

[mikelawther, 2010-12-06 03:57:18]

I added a similar flag to SkBlurMaskFilter as suggested.

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

http://codereview.appspot.com/3391041/diff/6001/src/effects/SkBlurDrawLooper....
src/effects/SkBlurDrawLooper.cpp:71: untransformedRadius,
SkBlurMaskFilter::kNormal_BlurStyle));
On 2010/12/03 20:22:08, Stephen White wrote:
> Hmm, this is a bit unfortunate.  I don't know how often next() is called, but
it
> might be thrashing the heap a bit to constant re-create the filter.
> 
> Would it make any sense to create a similar flag for SkBlurMaskFilter, and
make
> it responsible for doing the transform / no transform adjustment instead?

Yes - it did feel a bit dirty. I've uploaded a new patch that moves the
responsibility into SkBlurMaskFilter. While it creates more code in
SkBlurMaskFilter, it's probably cleaner.

[Stephen White, 2010-12-06 19:07:21]

This looks much cleaner, thanks.

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

http://codereview.appspot.com/3391041/diff/12001/src/effects/SkBlurDrawLooper...
src/effects/SkBlurDrawLooper.cpp:9: : fRadius(radius), fDx(dx), fDy(dy),
fBlurColor(color), fBlurFlags(flags)
Do you still need the fRadius member var?  It seems to be unused now.

[mikelawther, 2010-12-06 23:31:12]

OK - I think I've addressed everything :) How do I go about getting this patch
landed in Skia?

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

http://codereview.appspot.com/3391041/diff/12001/src/effects/SkBlurDrawLooper...
src/effects/SkBlurDrawLooper.cpp:9: : fRadius(radius), fDx(dx), fDy(dy),
fBlurColor(color), fBlurFlags(flags)
On 2010/12/06 19:07:21, Stephen White wrote:
> Do you still need the fRadius member var?  It seems to be unused now.

Good catch! Removed.

[Stephen White, 2010-12-06 23:42:33]

On 2010/12/06 23:31:12, mikelawther wrote:
> OK - I think I've addressed everything :) How do I go about getting this patch
> landed in Skia?

I'll land it.
> 
>
http://codereview.appspot.com/3391041/diff/12001/src/effects/SkBlurDrawLooper...
> File src/effects/SkBlurDrawLooper.cpp (right):
> 
>
http://codereview.appspot.com/3391041/diff/12001/src/effects/SkBlurDrawLooper...
> src/effects/SkBlurDrawLooper.cpp:9: : fRadius(radius), fDx(dx), fDy(dy),
> fBlurColor(color), fBlurFlags(flags)
> On 2010/12/06 19:07:21, Stephen White wrote:
> > Do you still need the fRadius member var?  It seems to be unused now.
> 
> Good catch! Removed.

[Stephen White, 2010-12-06 23:46:45]

Landed as r631.

[mikelawther, 2010-12-06 23:49:30]

On 2010/12/06 23:46:45, Stephen White wrote:
> Landed as r631.

Thanks!