Implement srcRect and dstRect functionality in SkBitmapSource. This is required for the "preserveAs…

Implement srcRect and dstRect functionality in SkBitmapSource. This is required for the "preserveAspectRatio" options of SVG's feImage. Covered by new GM "bitmapsource".

This also includes some changes to the xfermodeimagefilter and tileimagefilter GMs to properly handle the CTM. This worked before only because SkBitmapSource was ignoring the CTM. Now that it respects it, we need to give the correct transform. This also means the GMs now work while zoomed. It also implements CTM support for SkTileImageFilter.

NOTE: this will require rebaselining a number of imagefilter GMs on Nexus4, since they render in perspective (using the CTM). The changes to the results should all be improvements.

R=reed@google.com, robertphillips@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12571

[Stephen White, 2013-12-05 18:27:31]

PTAL. Thanks!

[reed1, 2013-12-05 18:48:24]

https://codereview.chromium.org/106933002/diff/1/include/effects/SkBitmapSour...
File include/effects/SkBitmapSource.h (right):

https://codereview.chromium.org/106933002/diff/1/include/effects/SkBitmapSour...
include/effects/SkBitmapSource.h:16: explicit SkBitmapSource(const SkBitmap&
bitmap, const SkRect* srcRect = NULL, const SkRect* dstRect = NULL);
Two option params + explicit?

Can we make two (at least) different constuctors, if NULL, NULL is common, or
have none of them have default values?

Not sure it applies here, but SkCanvas' convention was that src was optional,
but dst was required (in which case it was & instead of *). Maybe that doesn't
apply here...

[Stephen White, 2013-12-05 18:59:54]

https://codereview.chromium.org/106933002/diff/1/include/effects/SkBitmapSour...
File include/effects/SkBitmapSource.h (right):

https://codereview.chromium.org/106933002/diff/1/include/effects/SkBitmapSour...
include/effects/SkBitmapSource.h:16: explicit SkBitmapSource(const SkBitmap&
bitmap, const SkRect* srcRect = NULL, const SkRect* dstRect = NULL);
On 2013/12/05 18:48:25, reed1 wrote:
> Two option params + explicit?

Yep, explicit is still needed since call sites of the 1-arg version can still be
interpreted as an implicit conversion.

> Can we make two (at least) different constuctors, if NULL, NULL is common, or
> have none of them have default values?

> Not sure it applies here, but SkCanvas' convention was that src was optional,
> but dst was required (in which case it was & instead of *). Maybe that doesn't
> apply here...

Blink will shortly be calling only the 3-arg version. I'd like to keep the 1-arg
version around to keep tests concise. So I could do 1-arg and 3-arg, with all
parameters required. That would cover the known use cases. Not really sure about
the usefulness of the other two (src-only or dst-only). Maybe we could add them
later if needed.

[Stephen White, 2013-12-05 19:05:02]

New patch uploaded with separate 1-arg and 3-arg constructors.

[reed1, 2013-12-05 21:06:25]

lgtm

[Stephen White, 2013-12-05 21:46:06]

While cleaning up some Win32 errors, I realized we weren't doing the right thing
for non-integer dest rects. New patch uploaded.

[reed1, 2013-12-05 21:58:31]

lgtm

https://codereview.chromium.org/106933002/diff/40001/src/effects/SkBitmapSour...
File src/effects/SkBitmapSource.cpp (right):

https://codereview.chromium.org/106933002/diff/40001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:54: dstRect.roundOut(&dstIRect);
Just curious if this is more correctly round or roundOut, esp. since we also use
the value in offset-> down below.

[Stephen White, 2013-12-05 22:06:31]

Thanks for your review.

https://codereview.chromium.org/106933002/diff/40001/src/effects/SkBitmapSour...
File src/effects/SkBitmapSource.cpp (right):

https://codereview.chromium.org/106933002/diff/40001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:54: dstRect.roundOut(&dstIRect);
On 2013/12/05 21:58:31, reed1 wrote:
> Just curious if this is more correctly round or roundOut, esp. since we also
use
> the value in offset-> down below.

I'm pretty sure it should be roundOut, since we want our offscreen to accomodate
all the (possibly fractional) pixels. We use the drawBitmapRectToRect() to apply
the fractional portion of the offset, and the offset-> to apply the integer
portion.

[Stephen White, 2013-12-05 22:41:58]

Committed patchset #3 manually as r12522 (presubmit successful).

[Stephen White, 2013-12-06 15:25:16]

Landing again with test fixes.

[commit-bot: I haz the power, 2013-12-06 15:25:29]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/106933002/60001

[commit-bot: I haz the power, 2013-12-06 15:59:07]

Change committed as 12528

[robertphillips, 2013-12-06 17:47:47]

This was reverted (along with r12533 - Fix Windows compiler
warnings/errorshttps://codereview.chromium.org/108563002/) due to GM image
differences on the N4 in r12534 (Reverting 12528 & 12533 due to N4 GM diffs
https://codereview.chromium.org/108623002/).

[Stephen White, 2013-12-08 19:15:59]

On 2013/12/06 17:47:47, robertphillips wrote:
> This was reverted (along with r12533 - Fix Windows compiler
> warnings/errorshttps://codereview.chromium.org/108563002/) due to GM image
> differences on the N4 in r12534 (Reverting 12528 & 12533 due to N4 GM diffs
> https://codereview.chromium.org/108623002/).

All of the Nexus 4 failures appear occur when running in "force perspective
matrix" mode, for which it seems only the N4 has results. All of the new results
are improvements (the input bitmaps are now rendered perspective-correct),
except for the GPU result for tileimagefilter, which shows some garbage on the
bottom left in the first sample. The reason for this failure: 

1) BitmapSource now applies the CTM, so the input bitmap is resized to a
texture-backed SkBitmap (good).
2) SkTileImageFilter takes the GPU-backed SkBitmap, and attempts to call
extractSubset() with a rect of (20, 20, 80x80). This exceeds the bounds of the
source bitmap (0, 0, 80x80).
3) SkBitmap::extractSubset() on the GPU will actually assert in Debug, or return
garbage in Release when given a rect that exceeds the source bitmap bounds.
(Without this patch, It was working because the bitmap was not texture-backed,
and the CPU path for extractSubset() seems to simply return the intersected
subset).

Fixes to SkTileImageFilter: apply the CTM to both the srcRect and dstRect, and
intersect() the srcRect with the source bitmap bounds, so the rect passed to
extractSubset() is always, in fact, a subset.

This should only affect the perspective results, so I'm just going to land this
without suppressions, and rebaseline the N4 results.

[Stephen White, 2013-12-09 12:50:57]

New patch is up BTW; PTAL.

[robertphillips, 2013-12-09 16:23:00]

lgtm + nits & suggestions

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp
File gm/bitmapsource.cpp (right):

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:11: 
// This GM ...?

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:17: protected:
override

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:21: 
makeBitmap?

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:35: 
override

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:37: 
override

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:38: virtual void onDraw(SkCanvas* canvas) {
maybe just do this in onOnceBeforeDraw?

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:54: 
subroutine/loop?

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:86: private:
micro-nit: this is supposed to go last

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:93: 
DEF_GM

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.cpp
File gm/xfermodeimagefilter.cpp (right):

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:92: if (!fInitialized) {
onOnceBeforeDraw?

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:142: paint.setImageFilter(filter);
this->

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:153: paint.setImageFilter(filter);
this->

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:162: paint.setImageFilter(filter);
this->

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:180: paint.setImageFilter(filter);
this-> on all drawClippedPaint calls

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:223: private:
last?

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:230: 
DEF_GM?

https://codereview.chromium.org/106933002/diff/80001/src/effects/SkBitmapSour...
File src/effects/SkBitmapSource.cpp (right):

https://codereview.chromium.org/106933002/diff/80001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:49: if (fSrcRect == bounds && dstRect == bounds)
{
// no portion cropped out

https://codereview.chromium.org/106933002/diff/80001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:68: // None filtering when it's translate-only
\n somewhere?

[Stephen White, 2013-12-09 17:04:25]

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp
File gm/bitmapsource.cpp (right):

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:11: 
On 2013/12/09 16:23:01, robertphillips wrote:
> // This GM ...?

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:17: protected:
On 2013/12/09 16:23:01, robertphillips wrote:
> override

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:21: 
On 2013/12/09 16:23:01, robertphillips wrote:
> makeBitmap?

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:35: 
On 2013/12/09 16:23:01, robertphillips wrote:
> override

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:37: 
On 2013/12/09 16:23:01, robertphillips wrote:
> override

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:38: virtual void onDraw(SkCanvas* canvas) {
On 2013/12/09 16:23:01, robertphillips wrote:
> maybe just do this in onOnceBeforeDraw?

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:54: 
On 2013/12/09 16:23:01, robertphillips wrote:
> subroutine/loop?

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:86: private:
On 2013/12/09 16:23:01, robertphillips wrote:
> micro-nit: this is supposed to go last

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/bitmapsource.cpp#newc...
gm/bitmapsource.cpp:93: 
On 2013/12/09 16:23:01, robertphillips wrote:
> DEF_GM

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.cpp
File gm/xfermodeimagefilter.cpp (right):

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:92: if (!fInitialized) {
On 2013/12/09 16:23:01, robertphillips wrote:
> onOnceBeforeDraw?

Done.

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:142: paint.setImageFilter(filter);
On 2013/12/09 16:23:01, robertphillips wrote:
> this->

Made static instead.

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:180: paint.setImageFilter(filter);
On 2013/12/09 16:23:01, robertphillips wrote:
> this-> on all drawClippedPaint calls

See above.

https://codereview.chromium.org/106933002/diff/80001/gm/xfermodeimagefilter.c...
gm/xfermodeimagefilter.cpp:230: 
On 2013/12/09 16:23:01, robertphillips wrote:
> DEF_GM?

Done.

https://codereview.chromium.org/106933002/diff/80001/src/effects/SkBitmapSour...
File src/effects/SkBitmapSource.cpp (right):

https://codereview.chromium.org/106933002/diff/80001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:49: if (fSrcRect == bounds && dstRect == bounds)
{
On 2013/12/09 16:23:01, robertphillips wrote:
> // no portion cropped out

Done.

https://codereview.chromium.org/106933002/diff/80001/src/effects/SkBitmapSour...
src/effects/SkBitmapSource.cpp:68: // None filtering when it's translate-only
On 2013/12/09 16:23:01, robertphillips wrote:
> \n somewhere?

Done.

[Stephen White, 2013-12-09 18:31:50]

Committed patchset #6 manually as r12571 (presubmit successful).