cc: Add ImageManager to keep all the image locking in the same place.

cc: Add ImageManager to keep all the image locking in the same place.

This patch refactors some tile manager code into ImageManager with
an intent of moving all image related stuff (pre-decode images /
checker images) into that class.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/26ce8fb70fa85ccc412a1251f76d1883dea581a5
Cr-Commit-Position: refs/heads/master@{#418963}

[vmpstr, 2016-09-15 00:29:26]

Description was changed from

==========
cc: Add ImageManager to keep all the image locking in the same place.

This patch refactors some tile manager code into ImageManager with
an intent of moving all image related stuff (pre-decode images /
checker images) into that class.

R=enne
==========

to

==========
cc: Add ImageManager to keep all the image locking in the same place.

This patch refactors some tile manager code into ImageManager with
an intent of moving all image related stuff (pre-decode images /
checker images) into that class.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[vmpstr, 2016-09-15 00:30:14]

Please take a look. I'm planning on moving the predecode stuff into ImageManager
as a followup

[enne (OOO), 2016-09-15 17:42:16]

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.cc
File cc/tiles/image_manager.cc (right):

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.cc#n...
cc/tiles/image_manager.cc:13: controller_ = controller;
This is an awkward API.  I realize that the image decode controller you get is
created elsewhere.  But, what happens when this changes? Should this be a const
constructor arg and then you create a new image manager?

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.h
File cc/tiles/image_manager.h (right):

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.h#ne...
cc/tiles/image_manager.h:19: class CC_EXPORT ImageManager {
What else do you see going into this class? I think this patch seems fine on its
own, but because I'm not sure I see what else is moving in here it's hard to
tell.

[vmpstr, 2016-09-15 18:16:35]

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.cc
File cc/tiles/image_manager.cc (right):

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.cc#n...
cc/tiles/image_manager.cc:13: controller_ = controller;
On 2016/09/15 17:42:15, enne wrote:
> This is an awkward API.  I realize that the image decode controller you get is
> created elsewhere.  But, what happens when this changes? Should this be a
const
> constructor arg and then you create a new image manager?

So you're absolutely right that this can change. For example when we fallback
from ganesh to sw, this would be a new controller. What I'm trying to do here is
isolate common code from the IDC which is specific to gpu/sw. 

When we create TileManager (which owns this ImageManager), we don't actually
have an image decode controller, it gets created later and we get a
TileManager::SetResources call... I think it makes sense that this class handles
the fact that old controller is gone and a new one is set. WDYT?

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.h
File cc/tiles/image_manager.h (right):

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.h#ne...
cc/tiles/image_manager.h:19: class CC_EXPORT ImageManager {
On 2016/09/15 17:42:15, enne wrote:
> What else do you see going into this class? I think this patch seems fine on
its
> own, but because I'm not sure I see what else is moving in here it's hard to
> tell.

The next thing I was thinking is "SetPredecodeImages" which takes a set of
images and returns a set of tasks. This eliminates 2 members from tile manager.
This would also handle unreffing old images, eliminating that code from TM. 

Later, I was thinking this would be the class keeping track of which images are
checkered, providing tasks for it, and providing invalidation signals for when
the images are decoded.

[enne (OOO), 2016-09-15 18:27:03]

lgtm

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.cc
File cc/tiles/image_manager.cc (right):

https://codereview.chromium.org/2339703005/diff/1/cc/tiles/image_manager.cc#n...
cc/tiles/image_manager.cc:13: controller_ = controller;
On 2016/09/15 at 18:16:35, vmpstr wrote:
> On 2016/09/15 17:42:15, enne wrote:
> > This is an awkward API.  I realize that the image decode controller you get
is
> > created elsewhere.  But, what happens when this changes? Should this be a
const
> > constructor arg and then you create a new image manager?
> 
> So you're absolutely right that this can change. For example when we fallback
from ganesh to sw, this would be a new controller. What I'm trying to do here is
isolate common code from the IDC which is specific to gpu/sw. 
> 
> When we create TileManager (which owns this ImageManager), we don't actually
have an image decode controller, it gets created later and we get a
TileManager::SetResources call... I think it makes sense that this class handles
the fact that old controller is gone and a new one is set. WDYT?

Ok, that's fine if you want to just handle it here when this class has members
where it matters.

[vmpstr, 2016-09-15 18:53:25]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2016-09-15 18:54:01]

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-09-15 19:21:17]

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

[commit-bot: I haz the power, 2016-09-15 19:21:18]

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

[vmpstr, 2016-09-15 19:59:31]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2016-09-15 20:00:02]

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-09-15 20:47:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-15 20:50:22]

Description was changed from

==========
cc: Add ImageManager to keep all the image locking in the same place.

This patch refactors some tile manager code into ImageManager with
an intent of moving all image related stuff (pre-decode images /
checker images) into that class.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Add ImageManager to keep all the image locking in the same place.

This patch refactors some tile manager code into ImageManager with
an intent of moving all image related stuff (pre-decode images /
checker images) into that class.

R=enne
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/26ce8fb70fa85ccc412a1251f76d1883dea581a5
Cr-Commit-Position: refs/heads/master@{#418963}
==========

[commit-bot: I haz the power, 2016-09-15 20:50:23]

Patchset 1 (id:??) landed as
https://crrev.com/26ce8fb70fa85ccc412a1251f76d1883dea581a5
Cr-Commit-Position: refs/heads/master@{#418963}