Explicit tile bounds for SkPictureShader

Explicit tile bounds for SkPictureShader

The integer picture size is not granular enough to allow precise tiling
in arbitrary coordinate systems. This CL adds an optional tile bounds
float rect param to control the tile size and location.

(this also allows tile spacing emulation for picture
shaders).

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

Committed: https://skia.googlesource.com/skia/+/b5f7826c51af2862aebcabe61e1ba684f925e488

[reed1, 2014-08-05 15:51:25]

(gulp) is this a general feature we could expose?

newShader = oldShader + tileRect

https://codereview.chromium.org/437393003/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/437393003/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:437: const SkRect* tile = NULL);
Some documentation questions:

Does adding a tile rect affect the translation of the contents? i.e. in what
order are localMatrix and tileRect applied?

[f(malita), 2014-08-05 16:39:58]

On 2014/08/05 15:51:25, reed1 wrote:
> (gulp) is this a general feature we could expose?
> 
> newShader = oldShader + tileRect

Possibly, yes. I can imagine it would solve the tile spacing issue (Blink
currently emulates that by re-drawing into a larger tile).

It did cross my mind to look into making the tile rect a general shader param,
but it may not play too well with some of the specialized procs. This
composition approach seems more elegant, although I can't tell whether the
overhead would be significant.

My problem at hand is refactoring SVG patterns using picture shaders. To make
that work, I just need a more precise tile *size* - so I'm a bit wary of
over-engineering :)

The options I see:

1) add an SkSize tile size param to SkPictureShader (minimal change)
2) add an SkRect tile rect param to SkPictureShader (this CL, slightly more
flexible but not really needed for the problem at hand)
3) add an SkRect tile rect param to all shaders (general solution, most likely
too intrusive)
4) add SkTileShader to support the feature via composition (general elegant
solution, possible overhead)

We can chat about these and pick the best one.

https://codereview.chromium.org/437393003/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/437393003/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:437: const SkRect* tile = NULL);
On 2014/08/05 15:51:25, reed1 wrote:
> Some documentation questions:
> 
> Does adding a tile rect affect the translation of the contents? i.e. in what
> order are localMatrix and tileRect applied?

Yes, the tile offset is in "picture-space" (not affected by localMatrix) and
translates the contents. I'll update the documentation.

[f(malita), 2014-08-05 16:48:18]

On 2014/08/05 16:39:58, Florin Malita wrote:
> On 2014/08/05 15:51:25, reed1 wrote:
> > (gulp) is this a general feature we could expose?
> > 
> > newShader = oldShader + tileRect
> 
> Possibly, yes. I can imagine it would solve the tile spacing issue (Blink
> currently emulates that by re-drawing into a larger tile).
> 
> It did cross my mind to look into making the tile rect a general shader param,
> but it may not play too well with some of the specialized procs. This
> composition approach seems more elegant, although I can't tell whether the
> overhead would be significant.
> 
> My problem at hand is refactoring SVG patterns using picture shaders. To make
> that work, I just need a more precise tile *size* - so I'm a bit wary of
> over-engineering :)
> 
> The options I see:
> 
> 1) add an SkSize tile size param to SkPictureShader (minimal change)
> 2) add an SkRect tile rect param to SkPictureShader (this CL, slightly more
> flexible but not really needed for the problem at hand)
> 3) add an SkRect tile rect param to all shaders (general solution, most likely
> too intrusive)
> 4) add SkTileShader to support the feature via composition (general elegant
> solution, possible overhead)

I forgot option #5 (which is also something senorblanco might be interested in):
convert the SKP size to floats, and maybe extend it to a full bounds rect (with
the offset to be observed on playback/use).

[Stephen White, 2014-08-05 17:18:49]

On 2014/08/05 16:48:18, Florin Malita wrote:
> On 2014/08/05 16:39:58, Florin Malita wrote:
> > On 2014/08/05 15:51:25, reed1 wrote:
> > > (gulp) is this a general feature we could expose?
> > > 
> > > newShader = oldShader + tileRect
> > 
> > Possibly, yes. I can imagine it would solve the tile spacing issue (Blink
> > currently emulates that by re-drawing into a larger tile).
> > 
> > It did cross my mind to look into making the tile rect a general shader
param,
> > but it may not play too well with some of the specialized procs. This
> > composition approach seems more elegant, although I can't tell whether the
> > overhead would be significant.
> > 
> > My problem at hand is refactoring SVG patterns using picture shaders. To
make
> > that work, I just need a more precise tile *size* - so I'm a bit wary of
> > over-engineering :)
> > 
> > The options I see:
> > 
> > 1) add an SkSize tile size param to SkPictureShader (minimal change)
> > 2) add an SkRect tile rect param to SkPictureShader (this CL, slightly more
> > flexible but not really needed for the problem at hand)
> > 3) add an SkRect tile rect param to all shaders (general solution, most
likely
> > too intrusive)
> > 4) add SkTileShader to support the feature via composition (general elegant
> > solution, possible overhead)
> 
> I forgot option #5 (which is also something senorblanco might be interested
in):
> convert the SKP size to floats, and maybe extend it to a full bounds rect
(with
> the offset to be observed on playback/use).

FYI, I've logged this option as
https://code.google.com/p/skia/issues/detail?id=2801

[f(malita), 2014-08-05 21:30:42]

On 2014/08/05 16:39:58, Florin Malita wrote:
> On 2014/08/05 15:51:25, reed1 wrote:
> https://codereview.chromium.org/437393003/diff/1/include/core/SkShader.h
> File include/core/SkShader.h (right):
> 
>
https://codereview.chromium.org/437393003/diff/1/include/core/SkShader.h#newc...
> include/core/SkShader.h:437: const SkRect* tile = NULL);
> On 2014/08/05 15:51:25, reed1 wrote:
> > Some documentation questions:
> > 
> > Does adding a tile rect affect the translation of the contents? i.e. in what
> > order are localMatrix and tileRect applied?
> 
> Yes, the tile offset is in "picture-space" (not affected by localMatrix) and
> translates the contents. I'll update the documentation.

Updated, PTAL.

[reed1, 2014-08-06 13:14:35]

lgtm w/ parameter order nit.

https://codereview.chromium.org/437393003/diff/20001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/437393003/diff/20001/include/core/SkShader.h#...
include/core/SkShader.h:440: const SkRect* tile,
nit: I would not change the parameter order from the previous version per-se. I
was thinking:

Create(pic, tx, ty, localmatrix, tilerect)

with no default args at all.

[f(malita), 2014-08-06 14:56:02]

https://codereview.chromium.org/437393003/diff/20001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/437393003/diff/20001/include/core/SkShader.h#...
include/core/SkShader.h:440: const SkRect* tile,
On 2014/08/06 13:14:35, reed1 wrote:
> nit: I would not change the parameter order from the previous version per-se.
I
> was thinking:
> 
> Create(pic, tx, ty, localmatrix, tilerect)
> 
> with no default args at all.

Done.

[f(malita), 2014-08-06 14:56:17]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-08-06 14:56:37]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/437393003/40001

[commit-bot: I haz the power, 2014-08-06 18:07:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 18:07:01]

Failed to apply patch for gyp/gmslides.gypi:
While running git apply --index -p1;
  error: patch failed: gyp/gmslides.gypi:134
  error: gyp/gmslides.gypi: patch does not apply

Patch:       gyp/gmslides.gypi
Index: gyp/gmslides.gypi
diff --git a/gyp/gmslides.gypi b/gyp/gmslides.gypi
index
1ccccffed48ae974fdf80c5df27dbe9a982d8dcf..3ff168508ba63b14fc69c27f92fc99c3a88a6b97
100644
--- a/gyp/gmslides.gypi
+++ b/gyp/gmslides.gypi
@@ -134,6 +134,7 @@
     '../gm/perlinnoise.cpp',
     '../gm/pictureimagefilter.cpp',
     '../gm/pictureshader.cpp',
+    '../gm/pictureshadertile.cpp',
     '../gm/points.cpp',
     '../gm/poly2poly.cpp',
     '../gm/polygons.cpp',

[f(malita), 2014-08-06 19:58:57]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-08-06 19:59:38]

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

[commit-bot: I haz the power, 2014-08-06 20:07:20]

Change committed as b5f7826c51af2862aebcabe61e1ba684f925e488