cc: Refactored Tile::GetResourceId

cc: Refactored Tile::GetResourceId

With alternative methods of rending a quad being developed,
GetResourceId needs to change to accomodate this. Instead,
TileDrawingInfo class is introduce to encapsulate the different
methods (solid tile, streaming picture pile, etc)

BUG=173011
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187784

[enne (OOO), 2013-02-27 22:13:57]

https://codereview.chromium.org/12353003/diff/2001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12353003/diff/2001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:158: resource = iter->drawing_info().get_resource_id();
I think this isn't safe, given your changed implementation of get_resource_id. 
get_resource_id() should behave like the previous GetResourceId function did,
returning 0 if the tile has no gpu resource or has not been initialized. 
Otherwise, you'll get DCHECKs here.

https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.cc
File cc/tile_drawing_info.cc (right):

https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.cc#ne...
cc/tile_drawing_info.cc:13: return tile_->managed_state_.resource &&
This conditional should just call GetResourceId().

https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.cc#ne...
cc/tile_drawing_info.cc:21: return false;
Maybe also NOTREACHED()?

https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h
File cc/tile_drawing_info.h (right):

https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h#new...
cc/tile_drawing_info.h:15: class CC_EXPORT TileDrawingInfo {
I'm not really convinced that Tile is so complicated that mode_ and solid_color_
can't just go in the cc::Tile class itself, especially since TileDrawingInfo
needs a backpointer to the Tile in order to work.  It also makes calling code
more verbose since they have to reach into drawing_info() to get at information
about the tile.

https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h#new...
cc/tile_drawing_info.h:33: bool is_ready_to_draw() const;
style nit: don't name functions using hacker_style if your function is not an
inline simple accessor.

https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h#new...
cc/tile_drawing_info.h:35: ResourceProvider::ResourceId get_resource_id() const;
style nit: don't name functions using hacker_style if your function is not an
inline simple accessor.

[vmpstr, 2013-02-28 00:46:46]

On 2013/02/27 22:13:57, enne wrote:
> https://codereview.chromium.org/12353003/diff/2001/cc/picture_layer_impl.cc
> File cc/picture_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/12353003/diff/2001/cc/picture_layer_impl.cc#n...
> cc/picture_layer_impl.cc:158: resource =
iter->drawing_info().get_resource_id();
> I think this isn't safe, given your changed implementation of get_resource_id.

> get_resource_id() should behave like the previous GetResourceId function did,
> returning 0 if the tile has no gpu resource or has not been initialized. 
> Otherwise, you'll get DCHECKs here.

I fixed this. I tried to make as little changes as possible, since the rest of
the code doesn't yet use the other tile information, but I've put more in now. 

I do prefer that GetResourceId DCHECKs if the resource is not available, since
otherwise it gives a special meaning to "0" as the resource id... I would prefer
that the caller ensures that the tile is ready to draw (using IsReadyToDraw),
checks that it's texture mode, and then use the resource. I think it's more
clear in the calling code.

Let me know if you still want me to change it, as I don't feel too strongly
about this.

> 
> https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.cc
> File cc/tile_drawing_info.cc (right):
> 
>
https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.cc#ne...
> cc/tile_drawing_info.cc:13: return tile_->managed_state_.resource &&
> This conditional should just call GetResourceId().

See the comment above.

> 
>
https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.cc#ne...
> cc/tile_drawing_info.cc:21: return false;
> Maybe also NOTREACHED()?

Fixed.

> 
> https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h
> File cc/tile_drawing_info.h (right):
> 
>
https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h#new...
> cc/tile_drawing_info.h:15: class CC_EXPORT TileDrawingInfo {
> I'm not really convinced that Tile is so complicated that mode_ and
solid_color_
> can't just go in the cc::Tile class itself, especially since TileDrawingInfo
> needs a backpointer to the Tile in order to work.  It also makes calling code
> more verbose since they have to reach into drawing_info() to get at
information
> about the tile.

I was trying to put a bit more separation between what the tile holds, and how
to draw that tile... But I'm guessing that what the tile holds _is_ the
information on how to draw it? :) Doing it this way would allow adding more
"types" of drawing later without affecting the tile class... However, if you
feel that this information belongs in Tile itself, I'll change it.

> 
>
https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h#new...
> cc/tile_drawing_info.h:33: bool is_ready_to_draw() const;
> style nit: don't name functions using hacker_style if your function is not an
> inline simple accessor.

Fixed.

> 
>
https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h#new...
> cc/tile_drawing_info.h:35: ResourceProvider::ResourceId get_resource_id()
const;
> style nit: don't name functions using hacker_style if your function is not an
> inline simple accessor.

Fixed.

[enne (OOO), 2013-02-28 01:03:49]

On 2013/02/28 00:46:46, vmpstr wrote:

> I do prefer that GetResourceId DCHECKs if the resource is not available, since
> otherwise it gives a special meaning to "0" as the resource id... I would
prefer
> that the caller ensures that the tile is ready to draw (using IsReadyToDraw),
> checks that it's texture mode, and then use the resource. I think it's more
> clear in the calling code.

Ok, that's fine.  It looks like you've added more checks (or maybe I missed
them) in the calling code, so it looks safer now.  Thanks.  :)

>
https://codereview.chromium.org/12353003/diff/2001/cc/tile_drawing_info.h#new...
> > cc/tile_drawing_info.h:15: class CC_EXPORT TileDrawingInfo {
> > I'm not really convinced that Tile is so complicated that mode_ and
> solid_color_
> > can't just go in the cc::Tile class itself, especially since TileDrawingInfo
> > needs a backpointer to the Tile in order to work.  It also makes calling
code
> > more verbose since they have to reach into drawing_info() to get at
> information
> > about the tile.
> 
> I was trying to put a bit more separation between what the tile holds, and how
> to draw that tile... But I'm guessing that what the tile holds _is_ the
> information on how to draw it? :) Doing it this way would allow adding more
> "types" of drawing later without affecting the tile class... However, if you
> feel that this information belongs in Tile itself, I'll change it.

I don't really feel too strongly about this.  It just seems like the abstraction
isn't really clean here, since for texture types, you have to reach back in the
tile.  Maybe if the managed tile state also lived in the TileDrawInfo, then the
draw info would have everything it needed and TileManager would just be touching
the TileDrawInfo.  What do you think of that?

[enne (OOO), 2013-02-28 02:53:23]

On 2013/02/28 01:03:49, enne wrote:

> I don't really feel too strongly about this.  It just seems like the
abstraction
> isn't really clean here, since for texture types, you have to reach back in
the
> tile.  Maybe if the managed tile state also lived in the TileDrawInfo, then
the
> draw info would have everything it needed and TileManager would just be
touching
> the TileDrawInfo.  What do you think of that?

Alternatively, maybe everything in TileDrawInfo should move into
ManagedTileState, since the TileManager is responsible for populating both of
them?

[vmpstr, 2013-02-28 17:28:40]

On 2013/02/28 02:53:23, enne wrote:
> On 2013/02/28 01:03:49, enne wrote:
> 
> > I don't really feel too strongly about this.  It just seems like the
> abstraction
> > isn't really clean here, since for texture types, you have to reach back in
> the
> > tile.  Maybe if the managed tile state also lived in the TileDrawInfo, then
> the
> > draw info would have everything it needed and TileManager would just be
> touching
> > the TileDrawInfo.  What do you think of that?
> 
> Alternatively, maybe everything in TileDrawInfo should move into
> ManagedTileState, since the TileManager is responsible for populating both of
> them?

I just don't really like adding things to the ManagedTileState. It kind of seems
to be a catch-all for all information, from time to needed, bin information,
resolution, resources, etc... I'm hoping to replace ManagedTileState with
somewhat more explicit "this is the drawing state", "this is a bin/priority
state" type of items. I fully acknowledge that I might be missing a subtle issue
that this might be addressing, but it seems that it would be a bit cleaner (even
if its more verbose). Feel free to correct me. :) For now I just eliminated the
dependency on tile from the drawing state.

As an aside, would it make more sense if ManagedTileState owned the tile, and
not the other way around? It seems that a lot of that state is initialized in
tile manager and is only used in tile manager. I understand that there is some
persistent info that is carried within managed state across multiple calls to
tile manager, but that's the thing that can move to the tile/drawing info type
of things. Other things that do not persist would be created at register time.
Does that make sense?

[enne (OOO), 2013-02-28 18:01:03]

+reveman, ccameron

I'm not totally sure that that inversion makes sense.  At the very least, Tile
is the shared/refcounted type that says "I represent some piece of content at
some scale at some frame number" and can be used in multiple places so work
isn't duplicated.

Tile is the "what to draw and current priorities" and ManagedTileState currently
encapsulates "how to draw, ready to draw, raster scratch state, and combined
priority information".  That's kind of a mess now that I right it out.

I've come back around.  Maybe your TileDrawInfo is a good way to encapsulate the
"how to draw" state, and perhaps in the future some of ManagedTileState can move
there eventually if it makes sense.

[Leandro Graciá Gil, 2013-03-07 20:48:12]

In case it helps, I'm looking into adding a new mode to TileDrawingInfo to
support on-demand quad rasterization from a PicturePile in a different patch
based on this one.

Are there any major pending changes that I should consider before writing my
code over this patch?

[ccameron, 2013-03-07 21:49:49]

On 2013/02/28 18:01:03, enne wrote:
> +reveman, ccameron
> 
> I'm not totally sure that that inversion makes sense.  At the very least, Tile
> is the shared/refcounted type that says "I represent some piece of content at
> some scale at some frame number" and can be used in multiple places so work
> isn't duplicated.
> 
> Tile is the "what to draw and current priorities" and ManagedTileState
currently
> encapsulates "how to draw, ready to draw, raster scratch state, and combined
> priority information".  That's kind of a mess now that I right it out.
> 
> I've come back around.  Maybe your TileDrawInfo is a good way to encapsulate
the
> "how to draw" state, and perhaps in the future some of ManagedTileState can
move
> there eventually if it makes sense.

Sorry, I missed this. It seems reasonable to separate out TileDrawInfo. Maybe
that belongs in MangagedTileState or maybe Tile is the right place. If we do
some refactoring on TileManager to work on MTSes then that might involve moving
the member. But for now, I think reveman and I think that allocating Tiles
sparsely makes the most sense (and keeping Tiles as the refcounted structure),
when we get around to that.

[nduca, 2013-03-07 21:52:29]

https://codereview.chromium.org/12353003/diff/16001/cc/tile_manager.h
File cc/tile_manager.h (left):

https://codereview.chromium.org/12353003/diff/16001/cc/tile_manager.h#oldcode78
cc/tile_manager.h:78: bool can_be_freed;
I would like to see drawing_info_ be a member of ManagedTileState. Can you
update this patch, but with that change?

[reveman, 2013-03-07 22:02:14]

https://codereview.chromium.org/12353003/diff/16001/cc/tile.h
File cc/tile.h (right):

https://codereview.chromium.org/12353003/diff/16001/cc/tile.h#newcode80
cc/tile.h:80: inline size_t bytes_consumed_if_texture_allocated() const {
I don't think you want to use texture here. texture is a type of resource and
tiles should not be aware of the specific resource type used. maybe it should be
bytes_consumed_if_resource_is_allocated() but not clear to me why this need to
change at all.

https://codereview.chromium.org/12353003/diff/16001/cc/tile.h#newcode101
cc/tile.h:101: TileDrawingInfo drawing_info_;
The drawing state is maintained by the tile manager so it should probably be
moved to ManagedTileState.

https://codereview.chromium.org/12353003/diff/16001/cc/tile_drawing_info.h
File cc/tile_drawing_info.h (right):

https://codereview.chromium.org/12353003/diff/16001/cc/tile_drawing_info.h#ne...
cc/tile_drawing_info.h:68: bool initializing_resource_;
is there a good reason for not calling this resource_is_being_initialized as
before? not changing this might make the tile_manager.cc changes a bit cleaner.

[reveman, 2013-03-07 22:27:35]

I generally think of the tile manager as consuming cc::Tile and producing
cc::ManagedTileState. If these classes had separate instances then TileManager
would be able to only operate on "const Tile*". I'm not suggesting we make such
a change. Just how I think of relationship between these classes.

I'm OK with tile manager consuming cc::Tile and producing cc::TileDrawingInfo
but keeping those properties directly on the tile rather than a separate class
makes it hard to tell who is maintaining what state.

[vmpstr, 2013-03-07 22:57:25]

I've updated the patch. I moved the drawing info into the managed tile state. I
kept the drawing_info getters in the tile, since I think that it provides a
better interface to the "outside", but feel free to disagree.

On 2013/03/07 22:27:35, David Reveman wrote:
> I generally think of the tile manager as consuming cc::Tile and producing
> cc::ManagedTileState. If these classes had separate instances then TileManager
> would be able to only operate on "const Tile*". I'm not suggesting we make
such
> a change. Just how I think of relationship between these classes.

That's a useful way of thinking about it... All I meant about MTS owning the
Tile is that there's a bunch of state that is only used in tile manager, and it
feels bulky to tack that on to the Tile. Right now we have something like this:

RegisterTile(Tile* tile) { all_tiles_.insert(tile); }

I was wondering if it would be cleaner to have

RegisterTile(Tile* tile) { all_tiles_.insert(TemporaryTileManagerStuff(tile)); }

But I realized that ManagedTileState is more than just temporary stuff and I'm
getting used to ManagedTileState being a part of the tile, so I think it's
perfectly fine as it is now.

> 
> I'm OK with tile manager consuming cc::Tile and producing cc::TileDrawingInfo
> but keeping those properties directly on the tile rather than a separate class
> makes it hard to tell who is maintaining what state.

[Leandro Graciá Gil, 2013-03-12 19:37:20]

If there are no further comments I'll start implementing my patch over the
current contents of this one. If I should wait a bit longer please let me know.
Thanks!

[enne (OOO), 2013-03-12 19:42:13]

lgtm.  Thanks for all the changes;  I like this last patch quite a bit.  :)

https://chromiumcodereview.appspot.com/12353003/diff/27002/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://chromiumcodereview.appspot.com/12353003/diff/27002/cc/tile_manager.cc...
cc/tile_manager.cc:162: state->SetBoolean("resource_is_being_initialized",
drawing_info.resource_is_being_initialized_);
style nit: 80 col, here and the rest of this function.

[nduca, 2013-03-12 19:44:34]

looks mostly good. tweaks and eitehr get final review from reveman or very
proactively remind me to review agian. :)

https://codereview.chromium.org/12353003/diff/27002/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12353003/diff/27002/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:157: if (!*iter ||
!iter->drawing_info().IsReadyToDraw()) {
i'm concerned about you moving the !*iter below the iter.geometry_rect... if
iter is null, iter.geometry_rect() is maybe not safe?

https://codereview.chromium.org/12353003/diff/27002/cc/tile_drawing_info.h
File cc/tile_drawing_info.h (right):

https://codereview.chromium.org/12353003/diff/27002/cc/tile_drawing_info.h#ne...
cc/tile_drawing_info.h:15: class CC_EXPORT TileDrawingInfo {
Maybe you should instead pull ManagedTileState out to a file, and just make this
stuff part of that file?

https://codereview.chromium.org/12353003/diff/27002/cc/tile_drawing_info.h#ne...
cc/tile_drawing_info.h:20: TRANSPARENT_MODE,
Yeah, basically I see this as a inner struct on ManagedTileState. E.g.
ManagedTileState::DrawingInfo. That makes it clearer who controls it too

[enne (OOO), 2013-03-12 19:48:04]

https://codereview.chromium.org/12353003/diff/27002/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12353003/diff/27002/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:157: if (!*iter ||
!iter->drawing_info().IsReadyToDraw()) {
On 2013/03/12 19:44:34, nduca wrote:
> i'm concerned about you moving the !*iter below the iter.geometry_rect... if
> iter is null, iter.geometry_rect() is maybe not safe?

This is safe.  All iterators have a geometry rect, they just might not have a
tile to fill it.

[reveman, 2013-03-12 21:54:41]

latest patch looks good.

https://codereview.chromium.org/12353003/diff/27002/cc/tile_drawing_info.h
File cc/tile_drawing_info.h (right):

https://codereview.chromium.org/12353003/diff/27002/cc/tile_drawing_info.h#ne...
cc/tile_drawing_info.h:15: class CC_EXPORT TileDrawingInfo {
On 2013/03/12 19:44:34, nduca wrote:
> Maybe you should instead pull ManagedTileState out to a file, and just make
this
> stuff part of that file?

I like this idea. tile.h wouldn't have to include tile_manager.h anymore, which
I always found a bit awkward.

[vmpstr, 2013-03-12 22:14:45]

Thanks for the reviews!

I've addressed all issues that were mentioned. 
Note that I had to rebase on top of the solid
color prediction stuff, so the CL has more
changes than before.

PTAL.

[reveman, 2013-03-12 22:47:02]

https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.cc
File cc/managed_tile_state.cc (right):

https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.cc#...
cc/managed_tile_state.cc:26: switch(mode_) {
nit: space between switch and (

https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h
File cc/managed_tile_state.h (right):

https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
cc/managed_tile_state.h:21: 
nit: no blank line here please

https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
cc/managed_tile_state.h:53: 
nit: no trailing whitespace here please

https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
cc/managed_tile_state.h:95: std::list<skia::LazyPixelRef*> pending_pixel_refs;
add #include <list> above

https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
cc/managed_tile_state.h:114: } // namespace cc
nit: 2 spaces between } and // please

https://codereview.chromium.org/12353003/diff/42001/cc/tile.h
File cc/tile.h (right):

https://codereview.chromium.org/12353003/diff/42001/cc/tile.h#newcode47
cc/tile.h:47: void set_priority(WhichTree tree, const TilePriority& priority) {
I think this should be named SetPriority and implementation moved to tile.cc.
Surprised this builds without tile_manager.h being included. I guess another
header pulls this in. Maybe it doesn't have to?

[enne (OOO), 2013-03-12 22:54:32]

New patch still looks good and I don't have anything to add.  If nduca and
reveman are happy, I'm happy.

[vmpstr, 2013-03-12 23:06:30]

On 2013/03/12 22:47:02, David Reveman wrote:
> https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.cc
> File cc/managed_tile_state.cc (right):
> 
>
https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.cc#...
> cc/managed_tile_state.cc:26: switch(mode_) {
> nit: space between switch and (

Fixed.

> 
> https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h
> File cc/managed_tile_state.h (right):
> 
>
https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
> cc/managed_tile_state.h:21: 
> nit: no blank line here please

Fixed.

> 
>
https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
> cc/managed_tile_state.h:53: 
> nit: no trailing whitespace here please

Fixed.

> 
>
https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
> cc/managed_tile_state.h:95: std::list<skia::LazyPixelRef*> pending_pixel_refs;
> add #include <list> above

Fixed.

> 
>
https://codereview.chromium.org/12353003/diff/42001/cc/managed_tile_state.h#n...
> cc/managed_tile_state.h:114: } // namespace cc
> nit: 2 spaces between } and // please

Fixed.

> 
> https://codereview.chromium.org/12353003/diff/42001/cc/tile.h
> File cc/tile.h (right):
> 
> https://codereview.chromium.org/12353003/diff/42001/cc/tile.h#newcode47
> cc/tile.h:47: void set_priority(WhichTree tree, const TilePriority& priority)
{
> I think this should be named SetPriority and implementation moved to tile.cc.
> Surprised this builds without tile_manager.h being included. I guess another
> header pulls this in. Maybe it doesn't have to?

I agree that set_priority should be SetPriority, I changed it.
TileManager is still being pulled through from managed_tile_state.h,
which in turn needs it for TileManagerBin enum. I'm hesitant about
moving that too, since it really feels like it belongs in 
tile_manager.h

[reveman, 2013-03-12 23:24:13]

lgtm but we might want to move some of those enums in tile_manager.h to
managed_tile_state.h in a follow up. Maybe TileManagerBin should be
ManagedTileState::Bin.

[commit-bot: I haz the power, 2013-03-12 23:44:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12353003/40011

[commit-bot: I haz the power, 2013-03-13 00:06:13]

Failed to trigger a try job on linux_clang
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-03-13 00:11:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/12353003/63002

[nduca, 2013-03-13 01:44:21]

I am retroactively happylgtm

[commit-bot: I haz the power, 2013-03-13 05:05:23]

Change committed as 187784