SkImage-only bitmap patterns

SkImage-only bitmap patterns

The current BitmapPatternBase/BitmapPattern/StaticBitmapPattern class hierarchy exists to support
SkBitmap vs. SkImage-backed patterns.  But with the recent SkImage conversion effort, all Images
are now backed by SkImage and BitmapPattern has to go through an unnecessary conversion phase to
get its SkBitmap.

IOW, this dichotomy is now obsolete.  The CL removes BitmapPatternBase, BitmapPattern,
StaticBitmapPattern, and replaces them with ImagePattern (which is always/naturally backed by
SkImage).

Also, since we're in this area, convert the current non-repeating tile mode workaround to a
SkPicture-based implementation: instead of allocating/drawing into an expanded bitmap, allocate
an expanded SkPictureImageGenerator-backed SkImage and defer the actual tile drawing until
rasterization time.

BUG=449197
R=junov@chromium.org,senorblanco@chromium.org,reed@google.com

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200707

[Justin Novosad, 2015-08-17 15:03:24]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2015-08-17 15:03:25]

https://codereview.chromium.org/1290983003/diff/20001/Source/platform/graphic...
File Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/1290983003/diff/20001/Source/platform/graphic...
Source/platform/graphics/ImagePattern.cpp:65:
adoptRef(SkImage::NewFromPicture(tilePicture.get(), tileSize, nullptr,
nullptr));
Why is it necessary to go through a picture here?

[f(malita), 2015-08-17 15:09:30]

https://codereview.chromium.org/1290983003/diff/20001/Source/platform/graphic...
File Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/1290983003/diff/20001/Source/platform/graphic...
Source/platform/graphics/ImagePattern.cpp:65:
adoptRef(SkImage::NewFromPicture(tilePicture.get(), tileSize, nullptr,
nullptr));
On 2015/08/17 15:03:24, Justin Novosad wrote:
> Why is it necessary to go through a picture here?

It is a way to avoid record-time rasterization.  Not strictly necessary (we
could allocate an expanded raster surface here and draw the image, equivalent to
the previous behavior) - but it seems desirable to avoid this sort of thing on
the main thread.

[Justin Novosad, 2015-08-17 16:51:32]

On 2015/08/17 15:09:30, f(malita) wrote:
>
https://codereview.chromium.org/1290983003/diff/20001/Source/platform/graphic...
> File Source/platform/graphics/ImagePattern.cpp (right):
> 
>
https://codereview.chromium.org/1290983003/diff/20001/Source/platform/graphic...
> Source/platform/graphics/ImagePattern.cpp:65:
> adoptRef(SkImage::NewFromPicture(tilePicture.get(), tileSize, nullptr,
> nullptr));
> On 2015/08/17 15:03:24, Justin Novosad wrote:
> > Why is it necessary to go through a picture here?
> 
> It is a way to avoid record-time rasterization.  Not strictly necessary (we
> could allocate an expanded raster surface here and draw the image, equivalent
to
> the previous behavior) - but it seems desirable to avoid this sort of thing on
> the main thread.

I am concerned that in cases where the source image is already resterized we
will end-up with an unnecessary extra copy in ram, and unnecessary SkPicture
overhead.
Also is there a risk that the same source image will end-up being re-rasterized
repeatedly?  Or does the SkImage have some sort of cache. Also, In the case
where the source and destinations canvases are GPU-accelerated, will the result
in unnecessary readback/re-upload round trip?

I think you should make this CL just about the refactor, and experiment with the
optimization in a separate change. I am looking for the perf tests that Rémi
(piotaixr) created last summer for these use cases, but I can't find them. I
will look some more and come back to you.

[Justin Novosad, 2015-08-17 16:54:02]

> It is a way to avoid record-time rasterization.  Not strictly necessary (we
> could allocate an expanded raster surface here and draw the image, equivalent
to
> the previous behavior) - but it seems desirable to avoid this sort of thing on
> the main thread.

Wait, why allocate an expanded raster surface? Can't we just ref the SkImage
that was passed in?

[f(malita), 2015-08-17 17:59:03]

On 2015/08/17 16:54:02, Justin Novosad wrote:
> > It is a way to avoid record-time rasterization.  Not strictly necessary (we
> > could allocate an expanded raster surface here and draw the image,
equivalent
> to
> > the previous behavior) - but it seems desirable to avoid this sort of thing
on
> > the main thread.
> 
> Wait, why allocate an expanded raster surface? Can't we just ref the SkImage
> that was passed in?

It's a workaround for Skia's kClamp_TileMode semantics: we want the non-repeat
dimension to be filled with transparent color, but kClamp_TileMode repeats the
edge color.

So we're currently allocating an expanded bitmap (padded by one transparent
row/column as needed), draw the original bm in the upper-left corner and then
return a shader based on this new bm (kClamp_TileMode now replicates a
transparent row/column -> \o/):
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I think non-repeat patterns are quite rare (all SVG patterns are repeatXY, and
hopefully most others too), but I agree that this is pretty darn awkward.

Ideally, we'd want a Skia shader mode which implements Blink's semantics.  I
think it's on the radar (kDecal_TileMode), but the priority is not very high.

Second to that, it would be nice to be able to super-set SkImages, efficiently
(which is what you're probably hinting).  Not sure whether that's possible in
general, but it's not supported currently AFAIK.

Next is the approach taken in this CL: we're still performing the
expand-by-one-transparent-edge trick, but using an picture image => at least we
defer the work till rasterization time.

Old way:

* repeatXY => efficient, just return a shader for the original SkImage/SkBitmap
(handled in {StaticBitmapPattern,BitmapPattern}::createShader)

* repeatX-only or repeatY-only (handled in BitmapPatternBase::createShader):

  - allocate a temp expanded SkBitmap in RAM
  - clear it, then draw the orig in the upper-left corner
  - return a shader for the new bitmap

New way:

* repeatXY => still efficient, just return a shader for the original
SkImage/SkBitmap

* repeatX-only or repeatY-only:

  - allocate an expanded picture image, backed by a recording of drawing the
orig image in the upper-left corner
  - return a shader for this new SkImage

> I am concerned that in cases where the source image is already resterized we
> will end-up with an unnecessary extra copy in ram, and unnecessary SkPicture
> overhead.
> Also is there a risk that the same source image will end-up being
re-rasterized
> repeatedly?  Or does the SkImage have some sort of cache. Also, In the case
> where the source and destinations canvases are GPU-accelerated, will the
result
> in unnecessary readback/re-upload round trip?

These are all valid concerns, but they apply equally to the current impl
(BitmapPatternBase::createShader).  The difference with the new approach is that
allocation/rasterization is deferred until raster time, and the record time
overhead is replaced with a (cheap) SKP allocation.

> 
> I think you should make this CL just about the refactor, and experiment with
the
> optimization in a separate change.

Makes sense, I'll update the patch.

> I am looking for the perf tests that Rémi
> (piotaixr) created last summer for these use cases, but I can't find them. I
> will look some more and come back to you.

Thanks, let me know if you find anything.

[f(malita), 2015-08-17 18:51:30]

On 2015/08/17 17:59:03, f(malita) wrote:
> On 2015/08/17 16:54:02, Justin Novosad wrote:
> > I think you should make this CL just about the refactor, and experiment with
> the
> > optimization in a separate change.
> 
> Makes sense, I'll update the patch.

Done, PTAL.

https://codereview.chromium.org/1290983003/diff/40001/Source/platform/graphic...
File Source/platform/graphics/ImagePattern.cpp (right):

https://codereview.chromium.org/1290983003/diff/40001/Source/platform/graphic...
Source/platform/graphics/ImagePattern.cpp:47: // Skia does not have a "draw the
tile only once" option. Clamp_TileMode
This is the legacy fallback code from BitmapPatternBase::createShader(),
translated to the SkSurface/SkImage API.

[Justin Novosad, 2015-08-17 18:53:42]

lgtm

FWIW, I think the deferred SkPicture rendering approach may not be safe: If the
source image is an SkImage_Gpu, and the canvas to which the pattern is
ultimately drawn is in display list mode (rendered on the impl side), then you
will end up consuming the snapshot in a separate thread/GrContext/GL context,
and that's bad. To implement this safely, you would have to use the mailbox API
to package the texture.

[Justin Novosad, 2015-08-17 19:05:31]

On 2015/08/17 18:53:42, Justin Novosad wrote:
> lgtm
> 
> FWIW, I think the deferred SkPicture rendering approach may not be safe: If
the
> source image is an SkImage_Gpu, and the canvas to which the pattern is
> ultimately drawn is in display list mode (rendered on the impl side), then you
> will end up consuming the snapshot in a separate thread/GrContext/GL context,
> and that's bad. To implement this safely, you would have to use the mailbox
API
> to package the texture.

Actually, this may already be broken today in repeat mode. We have no layout
tests that check heterogenous canvas rendering modes because the virtual test
suite configs force all the canvases into the same mode.  Uh-oh.

[f(malita), 2015-08-17 19:56:13]

On 2015/08/17 19:05:31, Justin Novosad wrote:
> On 2015/08/17 18:53:42, Justin Novosad wrote:
> > lgtm
> > 
> > FWIW, I think the deferred SkPicture rendering approach may not be safe: If
> the
> > source image is an SkImage_Gpu, and the canvas to which the pattern is
> > ultimately drawn is in display list mode (rendered on the impl side), then
you
> > will end up consuming the snapshot in a separate thread/GrContext/GL
context,
> > and that's bad. To implement this safely, you would have to use the mailbox
> API
> > to package the texture.
> 
> Actually, this may already be broken today in repeat mode. We have no layout
> tests that check heterogenous canvas rendering modes because the virtual test
> suite configs force all the canvases into the same mode.  Uh-oh.

Is this limited to patterns?  I would expect even stuff like drawImage to be
affected by this issue (but not familiar with the code paths that would trigger
them).

Anyway, I can shelve that change for now, and I assume this CL is not making
things any worse.

[f(malita), 2015-08-17 19:57:53]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-08-17 19:58:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290983003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290983003/40001

[Justin Novosad, 2015-08-17 20:04:39]

On 2015/08/17 19:56:13, f(malita) wrote:
> On 2015/08/17 19:05:31, Justin Novosad wrote:
> > On 2015/08/17 18:53:42, Justin Novosad wrote:
> > > lgtm
> > > 
> > > FWIW, I think the deferred SkPicture rendering approach may not be safe:
If
> > the
> > > source image is an SkImage_Gpu, and the canvas to which the pattern is
> > > ultimately drawn is in display list mode (rendered on the impl side), then
> you
> > > will end up consuming the snapshot in a separate thread/GrContext/GL
> context,
> > > and that's bad. To implement this safely, you would have to use the
mailbox
> > API
> > > to package the texture.
> > 
> > Actually, this may already be broken today in repeat mode. We have no layout
> > tests that check heterogenous canvas rendering modes because the virtual
test
> > suite configs force all the canvases into the same mode.  Uh-oh.
> 
> Is this limited to patterns?  I would expect even stuff like drawImage to be
> affected by this issue (but not familiar with the code paths that would
trigger
> them).
> 
> Anyway, I can shelve that change for now, and I assume this CL is not making
> things any worse.

A canvas-to-canvas drawImage is safe because it causes the destination canvas to
exit display list mode

[Justin Novosad, 2015-08-17 20:06:18]

On 2015/08/17 20:04:39, Justin Novosad wrote:
> On 2015/08/17 19:56:13, f(malita) wrote:
> > On 2015/08/17 19:05:31, Justin Novosad wrote:
> > > On 2015/08/17 18:53:42, Justin Novosad wrote:
> > > > lgtm
> > > > 
> > > > FWIW, I think the deferred SkPicture rendering approach may not be safe:
> If
> > > the
> > > > source image is an SkImage_Gpu, and the canvas to which the pattern is
> > > > ultimately drawn is in display list mode (rendered on the impl side),
then
> > you
> > > > will end up consuming the snapshot in a separate thread/GrContext/GL
> > context,
> > > > and that's bad. To implement this safely, you would have to use the
> mailbox
> > > API
> > > > to package the texture.
> > > 
> > > Actually, this may already be broken today in repeat mode. We have no
layout
> > > tests that check heterogenous canvas rendering modes because the virtual
> test
> > > suite configs force all the canvases into the same mode.  Uh-oh.
> > 
> > Is this limited to patterns?  I would expect even stuff like drawImage to be
> > affected by this issue (but not familiar with the code paths that would
> trigger
> > them).
> > 
> > Anyway, I can shelve that change for now, and I assume this CL is not making
> > things any worse.
> 
> A canvas-to-canvas drawImage is safe because it causes the destination canvas
to
> exit display list mode

I tested, ToT and there is currently no issue with patterns. Not sure how come.

[commit-bot: I haz the power, 2015-08-17 22:33:41]

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

[commit-bot: I haz the power, 2015-08-17 22:33:42]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[f(malita), 2015-08-18 05:36:10]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-08-18 05:36:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290983003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290983003/40001

[commit-bot: I haz the power, 2015-08-18 06:49:14]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200707