Use SkImage snapshots for ImageBufferSurface texture access

Use SkImage snapshots for ImageBufferSurface texture access

Instead of accessing the underlying SkCanvas device and poking at render targets directly, use the
SkSurface to generate temporary SkImage snapshots, then grab texture IDs from these.

The main motivation is getting rid of deprecated SkCanvas::getTopDevice() users, but SkImage
wrappers also help making the texture ownership/lifetime more explicit.

(API refactoring, no functional side effects expected)

BUG=499001, skia:3940
R=reed@google.com,bsalomon@google.com,junov@chromium.org

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

[f(malita), 2015-06-18 16:32:59]

fmalita@chromium.org changed reviewers:
+ bsalomon@google.com, junov@chromium.org, reed@google.com

[f(malita), 2015-06-18 16:33:00]

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

https://codereview.chromium.org/1195513002/diff/20001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:518: if (!checkSurfaceValid())
This is the only place where getBackingTextureImage and newImageSnapshot (below)
diverge - all other implementations are identical.

It would be nice to unify them, but I'm not familiar with this code so not sure
what the implications of adding the check & flush to newImageSnapshot() are.

[Justin Novosad, 2015-06-22 18:22:03]

lgtm with nit.

I think you should add comments to explain the difference between
newImageSnapshot and getBackingTextureImage and why we need to have both. As far
as I am concerned, the main reason for having both is to avoid creating
snapshots in cases where we don't need them (in cases where we are only
interested in GPU-accelerated surface). This matters because capturing a
snapshot implies flushing deferred draws and triggering the fallback in
RecordingImageBufferSurface, which we want to avoid whenever possible.

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

https://codereview.chromium.org/1195513002/diff/20001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:518: if (!checkSurfaceValid())
On 2015/06/18 16:33:00, f(malita) wrote:
> This is the only place where getBackingTextureImage and newImageSnapshot
(below)
> diverge - all other implementations are identical.
> 
> It would be nice to unify them, but I'm not familiar with this code so not
sure
> what the implications of adding the check & flush to newImageSnapshot() are.

The check is necessary here, and does not make sense elsewhere. It is so 2D
canvas can handle GPU context losses

[f(malita), 2015-06-23 14:10:59]

On 2015/06/22 at 18:22:03, junov wrote:
> lgtm with nit.
> 
> I think you should add comments to explain the difference between
newImageSnapshot and getBackingTextureImage and why we need to have both. As far
as I am concerned, the main reason for having both is to avoid creating
snapshots in cases where we don't need them (in cases where we are only
interested in GPU-accelerated surface). This matters because capturing a
snapshot implies flushing deferred draws and triggering the fallback in
RecordingImageBufferSurface, which we want to avoid whenever possible.

Thanks, comments added.

[reed1, 2015-06-23 14:32:55]

brian, is this correct, that the snapshot is too heavy weight?

[Justin Novosad, 2015-06-23 14:33:49]

re-lgtm

[f(malita), 2015-06-23 14:35:20]

On 2015/06/23 at 14:32:55, reed wrote:
> brian, is this correct, that the snapshot is too heavy weight?

(my understanding is that the overhead is not necessarily in Skia, but in
RecordingImageBufferSurface fallback etc)

[f(malita), 2015-06-23 14:35:29]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-06-23 14:36:03]

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

[reed1, 2015-06-23 14:38:09]

https://codereview.chromium.org/1195513002/diff/40001/Source/platform/graphic...
File Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp (right):

https://codereview.chromium.org/1195513002/diff/40001/Source/platform/graphic...
Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp:66: // support
Canvas2DLayerBridge's slightly divergent semantics.
Lets document somewhere what those slightly divergent semantics are!

[f(malita), 2015-06-23 14:40:03]

The CQ bit was unchecked by fmalita@chromium.org

[reed1, 2015-06-23 14:48:18]

Junov, can you (after this CL) describe/docuemnt why we need two entry-points
that return the same thing?

[f(malita), 2015-06-23 14:49:05]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-06-23 14:49:33]

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

[Justin Novosad, 2015-06-23 14:50:56]

On 2015/06/23 14:48:18, reed1 wrote:
> Junov, can you (after this CL) describe/docuemnt why we need two entry-points
> that return the same thing?

Will do.

[commit-bot: I haz the power, 2015-06-23 15:13:50]

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