Fix MultiPictureDraw issues with GpuImageDecodeController

Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329, 628394
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/1259d6e73e7f8904243aa3451bc9071a18f93cbd
Cr-Commit-Position: refs/heads/master@{#405649}

[ericrk, 2016-07-14 21:14:28]

Description was changed from

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329
==========

to

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ericrk, 2016-07-14 21:17:51]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 21:18:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2016-07-14 21:41:54]

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.cc
File cc/playback/raster_source.cc (right):

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.c...
cc/playback/raster_source.cc:305: ImageDecodeController*
RasterSource::GetImageDecodeController() const {
You can just inline this and call it image_decode_controller() const { ... }

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.h
File cc/playback/raster_source.h (right):

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.h...
cc/playback/raster_source.h:122: // GpuRasterBufferProvider in order
nit: can you reformat the comment?

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.h...
cc/playback/raster_source.h:127: // to this class. See
gpu_raster_buffer_provider.cc for more information.
Can you file a bug and 

TODO(crbug.com/######): Redesign this to avoid exposing ImageDecodeController
from the raster source

https://codereview.chromium.org/2149843003/diff/1/cc/raster/gpu_raster_buffer...
File cc/raster/gpu_raster_buffer_provider.cc (right):

https://codereview.chromium.org/2149843003/diff/1/cc/raster/gpu_raster_buffer...
cc/raster/gpu_raster_buffer_provider.cc:91: const RasterSource* raster_source,
Can you just pass IDC here? and just "use_hijack_canvas" instead of full
playback settings?

[ericrk, 2016-07-14 22:04:20]

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.cc
File cc/playback/raster_source.cc (right):

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.c...
cc/playback/raster_source.cc:305: ImageDecodeController*
RasterSource::GetImageDecodeController() const {
On 2016/07/14 21:41:53, vmpstr wrote:
> You can just inline this and call it image_decode_controller() const { ... }

Sure, updated set_... as well.

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.h
File cc/playback/raster_source.h (right):

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.h...
cc/playback/raster_source.h:122: // GpuRasterBufferProvider in order
On 2016/07/14 21:41:54, vmpstr wrote:
> nit: can you reformat the comment?

Done.

https://codereview.chromium.org/2149843003/diff/1/cc/playback/raster_source.h...
cc/playback/raster_source.h:127: // to this class. See
gpu_raster_buffer_provider.cc for more information.
On 2016/07/14 21:41:54, vmpstr wrote:
> Can you file a bug and 
> 
> TODO(crbug.com/######): Redesign this to avoid exposing ImageDecodeController
> from the raster source

Done.

https://codereview.chromium.org/2149843003/diff/1/cc/raster/gpu_raster_buffer...
File cc/raster/gpu_raster_buffer_provider.cc (right):

https://codereview.chromium.org/2149843003/diff/1/cc/raster/gpu_raster_buffer...
cc/raster/gpu_raster_buffer_provider.cc:91: const RasterSource* raster_source,
On 2016/07/14 21:41:54, vmpstr wrote:
> Can you just pass IDC here? and just "use_hijack_canvas" instead of full
> playback settings?

yup, makes sense.

[ericrk, 2016-07-14 22:04:26]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 22:04:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2016-07-14 22:07:48]

lgtm

https://codereview.chromium.org/2149843003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2149843003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:6813: // Add an image that will be larger
than max texture size.
Is there a way to assert that this size is bigger than max texture size?

[commit-bot: I haz the power, 2016-07-14 22:52:07]

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

[commit-bot: I haz the power, 2016-07-14 22:52:08]

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

[ericrk, 2016-07-14 23:15:46]

https://codereview.chromium.org/2149843003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2149843003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:6813: // Add an image that will be larger
than max texture size.
On 2016/07/14 22:07:48, vmpstr wrote:
> Is there a way to assert that this size is bigger than max texture size?

Added.

[ericrk, 2016-07-14 23:15:50]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-07-14 23:15:51]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2149843003/#ps40001
(title: "Cleanup tests and add max texture size validation")

[commit-bot: I haz the power, 2016-07-14 23:16:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 23:31:11]

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

[commit-bot: I haz the power, 2016-07-14 23:31:12]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ericrk, 2016-07-15 00:02:32]

Description was changed from

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329,628394
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[ericrk, 2016-07-15 00:02:36]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-07-15 00:02:36]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2149843003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-15 00:03:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 00:47:45]

Description was changed from

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329,628394
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329,628394
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-15 00:47:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-15 00:48:59]

Description was changed from

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329,628394
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Fix MultiPictureDraw issues with GpuImageDecodeController

Currently, the GpuImageDecodeController (GPU IDC) is used during the
initial playback of a RasterSource. Unfortunately, the initial playback
is just used to generate an SkPicture, not to actually raster. Actual
rasterization happens later, via MultiPictureDraw. Because of this the
calls to GetDecodedImageForDraw and DrawWithImageFinished on the GPU IDC
do not scope Skia's usage of an image, as was intended. This can lead
to lifetime issues, especially with large, software-backed images.

This change moves the usage of ImageHijackCanvas and the GPU IDC to the
later MultiPictureDraw phase, allowing lifetimes to line up as expected.

R=vmpstr@chromium.org
BUG=612329,628394
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/1259d6e73e7f8904243aa3451bc9071a18f93cbd
Cr-Commit-Position: refs/heads/master@{#405649}
==========

[commit-bot: I haz the power, 2016-07-15 00:49:00]

Patchset 4 (id:??) landed as
https://crrev.com/1259d6e73e7f8904243aa3451bc9071a18f93cbd
Cr-Commit-Position: refs/heads/master@{#405649}