cc: Add image decode queue functionality to image manager.

cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537683002
Cr-Original-Commit-Position: refs/heads/master@{#443085}
Committed: https://chromium.googlesource.com/chromium/src/+/bb991d41668aa0f37aa6d6712e3bf9b52a5480ae
Review-Url: https://codereview.chromium.org/2537683002
Cr-Commit-Position: refs/heads/master@{#443341}
Committed: https://chromium.googlesource.com/chromium/src/+/a6b30160a85adeaa983877116a089ed46fa4bd62

[vmpstr, 2016-11-29 01:02:43]

Description was changed from

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, we should issue NotifyFrameFinished from somewhere.

R=enne@chromium.org
==========

to

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, we should issue NotifyFrameFinished from somewhere.

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2016-11-29 01:03:07]

Description was changed from

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, we should issue NotifyFrameFinished from somewhere.

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, 
- we should issue NotifyFrameFinished from somewhere.
- we should ensure we get the right task from the GIDC.

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2016-11-29 01:04:09]

Behold, a patch! There are still some things to fix, which are mentioned in the
description, but I'd like to see if this approach makes sense to you. Thanks!

[vmpstr, 2016-11-29 01:12:54]

Description was changed from

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, 
- we should issue NotifyFrameFinished from somewhere.
- we should ensure we get the right task from the GIDC.

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, 
- we should issue NotifyFrameFinished from somewhere.
- we should ensure we get the right task from the GIDC.
- ensure that a task that runs on raster won't ruin everything.

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2016-12-05 19:24:38]

Description was changed from

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, 
- we should issue NotifyFrameFinished from somewhere.
- we should ensure we get the right task from the GIDC.
- ensure that a task that runs on raster won't ruin everything.

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, 
- we should issue NotifyFrameFinished from somewhere.
- we should ensure we get the right task from the GIDC.
- ensure that a task that runs on raster won't ruin everything.

(ensure to reuse tasks)

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2016-12-06 22:06:37]

vmpstr@chromium.org changed reviewers:
+ ericrk@chromium.org

[ericrk, 2016-12-06 23:22:53]

GPU part seems good - a few nits - will continue to review the rest.

https://codereview.chromium.org/2537683002/diff/20001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2537683002/diff/20001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:184: GpuImageDecodeCache::DecodeTaskType
task_type_;
nit: const

https://codereview.chromium.org/2537683002/diff/20001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:730: RefImageDecode(draw_image);
Should we just move this out of this function, to the callsite where we create
the upload task? Might make it easier to understand who owns what ref?

https://codereview.chromium.org/2537683002/diff/20001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/20001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:102: enum DecodeTaskType {
PART_OF_UPLOAD_TASK, STAND_ALONE_DECODE_TASK };
nit: I forget if we were using enum class for things like this?

[vmpstr, 2016-12-22 21:02:07]

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

[commit-bot: I haz the power, 2016-12-22 21:02:35]

Dry run: 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-12-22 21:35:45]

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

[commit-bot: I haz the power, 2016-12-22 21:35:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vmpstr, 2016-12-22 23:47:57]

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

[commit-bot: I haz the power, 2016-12-22 23:48:20]

Dry run: 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-12-23 01:48:19]

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

[commit-bot: I haz the power, 2016-12-23 01:48:20]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2016-12-23 19:02:55]

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

[commit-bot: I haz the power, 2016-12-23 19:03:09]

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

[vmpstr, 2016-12-23 19:04:20]

Description was changed from

==========
WIP cc: Add image decode queue functionality to image manager.

Before landing, 
- we should issue NotifyFrameFinished from somewhere.
- we should ensure we get the right task from the GIDC.
- ensure that a task that runs on raster won't ruin everything.

(ensure to reuse tasks)

R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2016-12-23 19:04:20]

vmpstr@chromium.org changed reviewers:
+ danakj@chromium.org

[commit-bot: I haz the power, 2016-12-23 20:48:38]

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

[commit-bot: I haz the power, 2016-12-23 20:48:39]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-01-03 19:42:02]

Please take a look.

[enne (OOO), 2017-01-03 19:51:29]

What do you think about separating out the logic of knowing about frames and the
heuristic about holding for so many frames out of ImageController itself and
into some other class?  It seems a bit in the future that we'll want an image
decode service for out of process rasterization, and that particular behavior
won't be tied to frames at all.  Probably the requester from Blink will end up
asking through LayerTreeHost(Impl) and not through a particular layer, which
sounds a bit like maybe LayerTreeHostImpl (which already knows about frames)
could maybe proxy these requests to the ImageController and manage their
lifetimes?  And then ImageController no longer needs to know about these sorts
of things.

Still looking at details, but that's my first impression.

[vmpstr, 2017-01-03 19:57:02]

On 2017/01/03 19:51:29, enne wrote:
> What do you think about separating out the logic of knowing about frames and
the
> heuristic about holding for so many frames out of ImageController itself and
> into some other class?  It seems a bit in the future that we'll want an image
> decode service for out of process rasterization, and that particular behavior
> won't be tied to frames at all.  Probably the requester from Blink will end up
> asking through LayerTreeHost(Impl) and not through a particular layer, which
> sounds a bit like maybe LayerTreeHostImpl (which already knows about frames)
> could maybe proxy these requests to the ImageController and manage their
> lifetimes?  And then ImageController no longer needs to know about these sorts
> of things.
> 
> Still looking at details, but that's my first impression.

That makes sense. In ToT, LTHI owns a tile manager, which owns an image
controller, so I think it would be very much equivalent. I would have to teach
LTHI about locked images essentially and just request that TM unlocks them via
image controller. Does that sound better as a split? Then the callback from the
image controller will go to LTHI. I was kind of thinking of structuring the next
piece that way anyway

[enne (OOO), 2017-01-03 21:01:51]

On 2017/01/03 at 19:57:02, vmpstr wrote:
> On 2017/01/03 19:51:29, enne wrote:
> > What do you think about separating out the logic of knowing about frames and
the
> > heuristic about holding for so many frames out of ImageController itself and
> > into some other class?  It seems a bit in the future that we'll want an
image
> > decode service for out of process rasterization, and that particular
behavior
> > won't be tied to frames at all.  Probably the requester from Blink will end
up
> > asking through LayerTreeHost(Impl) and not through a particular layer, which
> > sounds a bit like maybe LayerTreeHostImpl (which already knows about frames)
> > could maybe proxy these requests to the ImageController and manage their
> > lifetimes?  And then ImageController no longer needs to know about these
sorts
> > of things.
> > 
> > Still looking at details, but that's my first impression.
> 
> That makes sense. In ToT, LTHI owns a tile manager, which owns an image
controller, so I think it would be very much equivalent. I would have to teach
LTHI about locked images essentially and just request that TM unlocks them via
image controller. Does that sound better as a split? Then the callback from the
image controller will go to LTHI. I was kind of thinking of structuring the next
piece that way anyway

Yeah.  I think it makes more sense for LTHI to know about requests from Blink
than it does for the ImageController to know about frames.

[enne (OOO), 2017-01-03 21:15:25]

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:136: params.image_worker_task_runner = nullptr;
Doesn't this default to nullptr?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.h:102: enum class DecodeTaskType {
PART_OF_UPLOAD_TASK, STAND_ALONE_DECODE_TASK };
I'm not sure I understand this distinction.  Shouldn't an out of raster decode
also be uploaded?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:29: scoped_refptr<base::SequencedTaskRunner>
worker_task_runner)
Do you need the "sequenced" part of sequenced task runner? It seems like the
guarantees about ordering are not important for an image decode service.

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:265: base::AutoUnlock unlock(lock_);
Will this cause an extra lock/unlock to have this here as opposed to just
scoping the AutoLock above?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:294: // If we need to urnef this decode, then we
have to put it into the locked
⚱ ef

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_cache.h:285: pending_in_raster_image_tasks_;
Why do you need a second map for out of raster image tasks? How is the same
image appearing in both maps handled? Does it get decoded twice?

[vmpstr, 2017-01-03 21:40:35]

Just responses for now, I haven't uploaded a new PS yet.

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:136: params.image_worker_task_runner = nullptr;
On 2017/01/03 21:15:25, enne wrote:
> Doesn't this default to nullptr?

Yes. Uh... Well, this is a relic of the past. As in, I went through a lot of
iterations some of which required this. In the end, I decided to leave it here
as an explicit "I know there's a param here called image_worker_task_runner, and
I'm OK with it being nullptr".

I can remove references to this if you prefer.

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.h:102: enum class DecodeTaskType {
PART_OF_UPLOAD_TASK, STAND_ALONE_DECODE_TASK };
On 2017/01/03 21:15:25, enne wrote:
> I'm not sure I understand this distinction.  Shouldn't an out of raster decode
> also be uploaded?

Eventually it would be (at raster/tile management). The distinction is that when
we use GetTaskForImageAndRef, what we get back is actually an upload task and it
has a dependency of the decode task. For this to happen, we do extra reffing of
the decode task, because we know that the upload task will unref it.

Since this image queue is only requesting a decode task and not an upload task,
it has to skip these extra refs (so that the behavior is the same as the
software image decode cache). Thus, these are the names. The _decode_ task
itself is either a part of the upload task, or it's a stand alone decode task.

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:29: scoped_refptr<base::SequencedTaskRunner>
worker_task_runner)
On 2017/01/03 21:15:25, enne wrote:
> Do you need the "sequenced" part of sequenced task runner? It seems like the
> guarantees about ordering are not important for an image decode service.

I don't really need it, but it's what was recommended I use <_< I can
investigate whether there's a non sequenced thing I could use.

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:265: base::AutoUnlock unlock(lock_);
On 2017/01/03 21:15:25, enne wrote:
> Will this cause an extra lock/unlock to have this here as opposed to just
> scoping the AutoLock above?

Oh good point.

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:294: // If we need to urnef this decode, then we
have to put it into the locked
On 2017/01/03 21:15:25, enne wrote:
> ⚱ ef

<_<

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_cache.h:285: pending_in_raster_image_tasks_;
On 2017/01/03 21:15:25, enne wrote:
> Why do you need a second map for out of raster image tasks? How is the same
> image appearing in both maps handled? Does it get decoded twice?

Yeah this is here as a safeguard against the task returned for the image queue
being used at the same time in raster/tile management. Because the image queue
calls "schedule/run/complete" sequences manually, it will crash/bad things will
happen if the raster system also uses the same task.

If the tile management requests the same image as the one queued in image queue,
then yes it will be decoded twice. However, the intent of the system is to not
give the images to the raster system unless the decode system says it's done, so
I think that's fine.

Note also that this is just for pending tasks, once the decode itself is done
and locked it goes into a common map.

[enne (OOO), 2017-01-03 21:49:42]

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:136: params.image_worker_task_runner = nullptr;
On 2017/01/03 at 21:40:34, vmpstr wrote:
> On 2017/01/03 21:15:25, enne wrote:
> > Doesn't this default to nullptr?
> 
> Yes. Uh... Well, this is a relic of the past. As in, I went through a lot of
iterations some of which required this. In the end, I decided to leave it here
as an explicit "I know there's a param here called image_worker_task_runner, and
I'm OK with it being nullptr".
> 
> I can remove references to this if you prefer.

Yah, I think I'd prefer them gone if they're not needed.  :)

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.h:102: enum class DecodeTaskType {
PART_OF_UPLOAD_TASK, STAND_ALONE_DECODE_TASK };
On 2017/01/03 at 21:40:34, vmpstr wrote:
> On 2017/01/03 21:15:25, enne wrote:
> > I'm not sure I understand this distinction.  Shouldn't an out of raster
decode
> > also be uploaded?
> 
> Eventually it would be (at raster/tile management). The distinction is that
when we use GetTaskForImageAndRef, what we get back is actually an upload task
and it has a dependency of the decode task. For this to happen, we do extra
reffing of the decode task, because we know that the upload task will unref it.
> 
> Since this image queue is only requesting a decode task and not an upload
task, it has to skip these extra refs (so that the behavior is the same as the
software image decode cache). Thus, these are the names. The _decode_ task
itself is either a part of the upload task, or it's a stand alone decode task.

Maybe my question is more fundamental? Why is the image queue only requesting a
decode task and not an upload task.  It's going to want to be uploaded
eventually, right?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/software_imag...
cc/tiles/software_image_decode_cache.h:285: pending_in_raster_image_tasks_;
On 2017/01/03 at 21:40:34, vmpstr wrote:
> On 2017/01/03 21:15:25, enne wrote:
> > Why do you need a second map for out of raster image tasks? How is the same
> > image appearing in both maps handled? Does it get decoded twice?
> 
> Yeah this is here as a safeguard against the task returned for the image queue
being used at the same time in raster/tile management. Because the image queue
calls "schedule/run/complete" sequences manually, it will crash/bad things will
happen if the raster system also uses the same task.
> 
> If the tile management requests the same image as the one queued in image
queue, then yes it will be decoded twice. However, the intent of the system is
to not give the images to the raster system unless the decode system says it's
done, so I think that's fine.
> 
> Note also that this is just for pending tasks, once the decode itself is done
and locked it goes into a common map.

Ah, ok thanks.  That makes more sense why they need to be separate.

[ericrk, 2017-01-04 07:48:32]

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.cc:747: scoped_refptr<TileTask>& existing_task =
I assume we can't end up decoding an image twice in parallel by enqueuing a
normal decode of an image that is already has a pending stand alone image? Once
we have decided to checkerboard an image, I assume that we just keep using the
checkerboarded version until the decode task completes?

Can you add a DCHECK to ensure that if we are PART_OF_UPLOAD_TASK then
stand_alone_task is nullptr?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.h:102: enum class DecodeTaskType {
PART_OF_UPLOAD_TASK, STAND_ALONE_DECODE_TASK };
On 2017/01/03 21:49:42, enne wrote:
> On 2017/01/03 at 21:40:34, vmpstr wrote:
> > On 2017/01/03 21:15:25, enne wrote:
> > > I'm not sure I understand this distinction.  Shouldn't an out of raster
> decode
> > > also be uploaded?
> > 
> > Eventually it would be (at raster/tile management). The distinction is that
> when we use GetTaskForImageAndRef, what we get back is actually an upload task
> and it has a dependency of the decode task. For this to happen, we do extra
> reffing of the decode task, because we know that the upload task will unref
it.
> > 
> > Since this image queue is only requesting a decode task and not an upload
> task, it has to skip these extra refs (so that the behavior is the same as the
> software image decode cache). Thus, these are the names. The _decode_ task
> itself is either a part of the upload task, or it's a stand alone decode task.
> 
> Maybe my question is more fundamental? Why is the image queue only requesting
a
> decode task and not an upload task.  It's going to want to be uploaded
> eventually, right?

I think the problem here is that the upload task needs to take the worker
context lock, and could contend with other rasterization / upload tasks. This is
more of an issue as the stand alone tasks here are run in their own queue, not
in the tile task worker pool. The worker pool ensures that multiple tasks don't
contend for the worker context lock, but we don't get that for the stand alone
tasks.

I guess the question is whether these tasks *should* actually go in the worker
pool. I think vmpstr@ decided not to do this, as putting them in that pool
requires that they either be marked as low priority, in which case they may not
run until raster has become idle, delaying them longer than a parallel,
always-running, queue. Alternately, they could be scheduled normally, but that
would cause them to block other raster tasks, defeating the purpose of
checkerboarding.

The compromise here was to do the decode in parallel on its own queue, but to
leave the upload as part of the eventual raster task, which allows for it to be
more intelligently scheduled.

I guess there is some concern (maybe on Android), that this change may introduce
another parallel thread?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:79: // finished. It would have posted a completion
callback back to the compositor
nit: sentence is a bit weird - did you mean "finished it", rather than
"finished. It"?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
File cc/tiles/image_controller.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.h:87: base::Lock lock_;
nit: can you indicate which variables can be accessed with/without locking? It
seems like the general pattern for most of these is only to initialize at setup
or when no tasks are running, but it would be good to indicate this more
clearly.

[enne (OOO), 2017-01-04 22:19:37]

On 2017/01/04 at 07:48:32, ericrk wrote:
> > Maybe my question is more fundamental? Why is the image queue only
requesting a
> > decode task and not an upload task.  It's going to want to be uploaded
> > eventually, right?
> 
> I think the problem here is that the upload task needs to take the worker
context lock, and could contend with other rasterization / upload tasks. This is
more of an issue as the stand alone tasks here are run in their own queue, not
in the tile task worker pool. The worker pool ensures that multiple tasks don't
contend for the worker context lock, but we don't get that for the stand alone
tasks.
> 
> I guess the question is whether these tasks *should* actually go in the worker
pool. I think vmpstr@ decided not to do this, as putting them in that pool
requires that they either be marked as low priority, in which case they may not
run until raster has become idle, delaying them longer than a parallel,
always-running, queue. Alternately, they could be scheduled normally, but that
would cause them to block other raster tasks, defeating the purpose of
checkerboarding.
> 
> The compromise here was to do the decode in parallel on its own queue, but to
leave the upload as part of the eventual raster task, which allows for it to be
more intelligently scheduled.

Isn't the upload step still a good bit of time? Is there a future plan to handle
uploads (for either Blink requests or image decode requests in a mus salamander
world) or is it a non-issue?

[vmpstr, 2017-01-04 22:53:07]

On 2017/01/04 22:19:37, enne wrote:
> On 2017/01/04 at 07:48:32, ericrk wrote:
> > > Maybe my question is more fundamental? Why is the image queue only
> requesting a
> > > decode task and not an upload task.  It's going to want to be uploaded
> > > eventually, right?
> > 
> > I think the problem here is that the upload task needs to take the worker
> context lock, and could contend with other rasterization / upload tasks. This
is
> more of an issue as the stand alone tasks here are run in their own queue, not
> in the tile task worker pool. The worker pool ensures that multiple tasks
don't
> contend for the worker context lock, but we don't get that for the stand alone
> tasks.
> > 
> > I guess the question is whether these tasks *should* actually go in the
worker
> pool. I think vmpstr@ decided not to do this, as putting them in that pool
> requires that they either be marked as low priority, in which case they may
not
> run until raster has become idle, delaying them longer than a parallel,
> always-running, queue. Alternately, they could be scheduled normally, but that
> would cause them to block other raster tasks, defeating the purpose of
> checkerboarding.
> > 
> > The compromise here was to do the decode in parallel on its own queue, but
to
> leave the upload as part of the eventual raster task, which allows for it to
be
> more intelligently scheduled.
> 
> Isn't the upload step still a good bit of time? Is there a future plan to
handle
> uploads (for either Blink requests or image decode requests in a mus
salamander
> world) or is it a non-issue?

My impression from the github conversation is that we want to punt on the
uploads/scales and start with just decodes. I think in the future we do want to
investigate handling uploads, much in the same way that we should handle scales
as well, since those can be slow too. I was hoping that this would be an
iteration on top of "just decode" though.

[vmpstr, 2017-01-06 01:19:03]

PTAL

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
File cc/layers/layer_unittest.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/layers/layer_unitte...
cc/layers/layer_unittest.cc:136: params.image_worker_task_runner = nullptr;
On 2017/01/03 21:49:42, enne wrote:
> On 2017/01/03 at 21:40:34, vmpstr wrote:
> > On 2017/01/03 21:15:25, enne wrote:
> > > Doesn't this default to nullptr?
> > 
> > Yes. Uh... Well, this is a relic of the past. As in, I went through a lot of
> iterations some of which required this. In the end, I decided to leave it here
> as an explicit "I know there's a param here called image_worker_task_runner,
and
> I'm OK with it being nullptr".
> > 
> > I can remove references to this if you prefer.
> 
> Yah, I think I'd prefer them gone if they're not needed.  :)

Done.

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.cc:747: scoped_refptr<TileTask>& existing_task =
On 2017/01/04 07:48:31, ericrk wrote:
> I assume we can't end up decoding an image twice in parallel by enqueuing a
> normal decode of an image that is already has a pending stand alone image?
Once
> we have decided to checkerboard an image, I assume that we just keep using the
> checkerboarded version until the decode task completes?
> 
> Can you add a DCHECK to ensure that if we are PART_OF_UPLOAD_TASK then
> stand_alone_task is nullptr?

That's not necessarily true, since which image is requested is up to the
developer (this is for a js controller image decoding). For image
checkerboarding I agree that that should be case, but I think in general it
doesn't have to be. You're right that we would decode twice, but that's still
possible :(

When adding image checkering, we can add specific dchecks for that. Does that
sound OK?

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.cc:79: // finished. It would have posted a completion
callback back to the compositor
On 2017/01/04 07:48:31, ericrk wrote:
> nit: sentence is a bit weird - did you mean "finished it", rather than
> "finished. It"?

Yes, my bad. Fixed.

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
File cc/tiles/image_controller.h (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/image_control...
cc/tiles/image_controller.h:87: base::Lock lock_;
On 2017/01/04 07:48:31, ericrk wrote:
> nit: can you indicate which variables can be accessed with/without locking? It
> seems like the general pattern for most of these is only to initialize at
setup
> or when no tasks are running, but it would be good to indicate this more
> clearly.

Done.

[enne (OOO), 2017-01-09 19:48:42]

re: uploading after decoding.  So long as there's some sort of plan for handling
this in the future (since we're going to want this for mus decode service, I
think), then I'm good with this as a first step.

lgtm

[ericrk, 2017-01-09 21:07:27]

One minor question, but LGTM

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2537683002/diff/120001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.cc:747: scoped_refptr<TileTask>& existing_task =
On 2017/01/06 01:19:03, vmpstr wrote:
> On 2017/01/04 07:48:31, ericrk wrote:
> > I assume we can't end up decoding an image twice in parallel by enqueuing a
> > normal decode of an image that is already has a pending stand alone image?
> Once
> > we have decided to checkerboard an image, I assume that we just keep using
the
> > checkerboarded version until the decode task completes?
> > 
> > Can you add a DCHECK to ensure that if we are PART_OF_UPLOAD_TASK then
> > stand_alone_task is nullptr?
> 
> That's not necessarily true, since which image is requested is up to the
> developer (this is for a js controller image decoding). For image
> checkerboarding I agree that that should be case, but I think in general it
> doesn't have to be. You're right that we would decode twice, but that's still
> possible :(
> 
> When adding image checkering, we can add specific dchecks for that. Does that
> sound OK?

Sounds good to me - there's probably something we could do to make it so that an
image which is in the process of being decoded doesn't have a second task
started (short-circuits)... can you open a bug for looking into this.

https://codereview.chromium.org/2537683002/diff/140001/cc/tiles/image_control...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2537683002/diff/140001/cc/tiles/image_control...
cc/tiles/image_controller.cc:61: FROM_HERE, base::Bind(+[](CompletionEvent*
event) { event->Signal(); },
never seen the "+" thing before... the more you know :D

https://codereview.chromium.org/2537683002/diff/140001/cc/tiles/image_control...
cc/tiles/image_controller.cc:138: if (cache_)
From looking at QueueImageDecode, it appears we would crash if you called it
without a cache_, so you can only add to image_decode_queue_ when cache_ exists.

It seems like it's only ever safe to transition from !cache_ to cache_ or from
cache_ to !cache_, in which case we will never have tasks to resume here?

If we were to switch caches (from one pointer to another), it seems like
refcounts would get messed up?

[vmpstr, 2017-01-09 21:13:24]

https://codereview.chromium.org/2537683002/diff/140001/cc/tiles/image_control...
File cc/tiles/image_controller.cc (right):

https://codereview.chromium.org/2537683002/diff/140001/cc/tiles/image_control...
cc/tiles/image_controller.cc:138: if (cache_)
On 2017/01/09 21:07:27, ericrk wrote:
> From looking at QueueImageDecode, it appears we would crash if you called it
> without a cache_, so you can only add to image_decode_queue_ when cache_
exists.
> 
> It seems like it's only ever safe to transition from !cache_ to cache_ or from
> cache_ to !cache_, in which case we will never have tasks to resume here?
> 
> If we were to switch caches (from one pointer to another), it seems like
> refcounts would get messed up?

This was a left over from an earlier approach I had, I'm gonna remove it here.
Thanks for catching it!

[vmpstr, 2017-01-09 21:39:49]

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

[commit-bot: I haz the power, 2017-01-09 21:40:21]

Dry run: 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, 2017-01-09 22:44:21]

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

[commit-bot: I haz the power, 2017-01-09 22:44:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vmpstr, 2017-01-10 00:24:12]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-01-10 00:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2537683002/#ps180001
(title: "update")

[commit-bot: I haz the power, 2017-01-10 00:25:05]

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, 2017-01-10 01:58:33]

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

[commit-bot: I haz the power, 2017-01-10 01:58:34]

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

[vmpstr, 2017-01-10 21:30:43]

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

[commit-bot: I haz the power, 2017-01-10 21:31:25]

Dry run: 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, 2017-01-10 21:35:17]

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

[commit-bot: I haz the power, 2017-01-10 21:35:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[vmpstr, 2017-01-10 22:00:37]

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

[commit-bot: I haz the power, 2017-01-10 22:01:03]

Dry run: 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, 2017-01-10 23:33:16]

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

[commit-bot: I haz the power, 2017-01-10 23:33:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vmpstr, 2017-01-10 23:53:42]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-01-10 23:53:42]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2537683002/#ps220001
(title: "update")

[commit-bot: I haz the power, 2017-01-10 23:54:35]

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, 2017-01-11 01:29:32]

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

[commit-bot: I haz the power, 2017-01-11 01:29:33]

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

[vmpstr, 2017-01-11 18:54:55]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-01-11 18:55:27]

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, 2017-01-11 20:01:50]

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

[commit-bot: I haz the power, 2017-01-11 20:01:51]

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

[vmpstr, 2017-01-11 22:13:36]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-01-11 22:15:22]

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, 2017-01-12 00:38:52]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484172816661060,
"parent_rev": "760800d72f2e61cefab8d9fca7cb36fa29867a80", "commit_rev":
"bb991d41668aa0f37aa6d6712e3bf9b52a5480ae"}

[commit-bot: I haz the power, 2017-01-12 00:39:35]

Description was changed from

==========
cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537683002
Cr-Commit-Position: refs/heads/master@{#443085}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb991d41668aa0f37aa6d6712e3b...
==========

[commit-bot: I haz the power, 2017-01-12 00:39:38]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/bb991d41668aa0f37aa6d6712e3b...

[findit-for-me, 2017-01-12 01:37:29]

FYI: Findit identified this CL at revision 443085 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[dschuyler, 2017-01-12 02:35:52]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2631453002/ by dschuyler@chromium.org.

The reason for reverting is: Hoping to fix build/test errors such as
https://uberchromegw.corp.google.com/i/chromium.win/builders/Win%207%20Tests%...

-- sheriff. .

[vmpstr, 2017-01-12 18:50:49]

Description was changed from

==========
cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537683002
Cr-Commit-Position: refs/heads/master@{#443085}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb991d41668aa0f37aa6d6712e3b...
==========

to

==========
cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537683002
Cr-Commit-Position: refs/heads/master@{#443085}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb991d41668aa0f37aa6d6712e3b...
==========

[vmpstr, 2017-01-12 19:04:53]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-01-12 19:04:54]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2537683002/#ps240001
(title: "test fix")

[commit-bot: I haz the power, 2017-01-12 19:05:43]

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, 2017-01-12 20:23:09]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484247893872980,
"parent_rev": "f54fb63ff0782eb490a81bf41775b031468f42ca", "commit_rev":
"a6b30160a85adeaa983877116a089ed46fa4bd62"}

[commit-bot: I haz the power, 2017-01-12 20:23:40]

Description was changed from

==========
cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537683002
Cr-Commit-Position: refs/heads/master@{#443085}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb991d41668aa0f37aa6d6712e3b...
==========

to

==========
cc: Add image decode queue functionality to image manager.

This patch adds an ability to request an image decode from
the compositor. The caller should specify which image
(SkImage) to decode and a callback. In return, it gets an
id. When the image is decoded, the callback is issued with
this id. The decode is then kept alive for 2 compositor
frames. 

Right now, only unittests are using this functionality.

R=enne@chromium.org, ericrk@chromium.org, danakj@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2537683002
Cr-Original-Commit-Position: refs/heads/master@{#443085}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb991d41668aa0f37aa6d6712e3b...
Review-Url: https://codereview.chromium.org/2537683002
Cr-Commit-Position: refs/heads/master@{#443341}
Committed:
https://chromium.googlesource.com/chromium/src/+/a6b30160a85adeaa983877116a08...
==========

[commit-bot: I haz the power, 2017-01-12 20:23:42]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/a6b30160a85adeaa983877116a08...

[Peter Mayo, 2017-01-17 22:35:25]

On 2017/01/12 20:23:42, commit-bot: I haz the power wrote:
> Committed patchset #13 (id:240001) as
>
https://chromium.googlesource.com/chromium/src/+/a6b30160a85adeaa983877116a08...

FWIW, this change breaks cc_unittests when --single-process-tests is passed.

That's sad because it's really useful for making rr recordings a lot easier to
follow.

It ends up failing with a timeout, that seems to call Task->Quit() from a
different thread.  I'm not sure timeouts work in the tests.

[vmpstr, 2017-01-17 23:29:21]

On 2017/01/17 22:35:25, Peter Mayo wrote:
> On 2017/01/12 20:23:42, commit-bot: I haz the power wrote:
> > Committed patchset #13 (id:240001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/a6b30160a85adeaa983877116a08...
> 
> FWIW, this change breaks cc_unittests when --single-process-tests is passed.
> 
> That's sad because it's really useful for making rr recordings a lot easier to
> follow.
> 
> It ends up failing with a timeout, that seems to call Task->Quit() from a
> different thread.  I'm not sure timeouts work in the tests.

Hmm this was certainly not the intent. I'll take a look ASAP to see if we need
to revert this or if it's an easy fix.

[vmpstr, 2017-01-18 01:22:46]

On 2017/01/17 23:29:21, vmpstr wrote:
> On 2017/01/17 22:35:25, Peter Mayo wrote:
> > On 2017/01/12 20:23:42, commit-bot: I haz the power wrote:
> > > Committed patchset #13 (id:240001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/a6b30160a85adeaa983877116a08...
> > 
> > FWIW, this change breaks cc_unittests when --single-process-tests is passed.
> > 
> > That's sad because it's really useful for making rr recordings a lot easier
to
> > follow.
> > 
> > It ends up failing with a timeout, that seems to call Task->Quit() from a
> > different thread.  I'm not sure timeouts work in the tests.
> 
> Hmm this was certainly not the intent. I'll take a look ASAP to see if we need
> to revert this or if it's an easy fix.

This patch should fix it: https://codereview.chromium.org/2640553003/