cc: The recent texture manager bug uncovered some unnecessary code

One good thing (the only good thing?) about the recent texture
    manager bug, is that it clarified where in-use-on-impl textures need
    to be protected (in the texture manager - the lack of which was the
    bug).
    
    The deleteTextureAfterCommit mechanism protects the 'front-buffer'
    when double-buffering updates, but it was never quite functional
    since it doesn't protect 'front-buffer' textures for layers which
    are deleted entirely while they still exist in the impl-tree.
    
    Since the texture manager takes care of this, we can remove this
    redundant protection code and just discard the texture backing when
    a buffered update is required.
    
    Once textures live primarily on the impl-thread and can be removed
    from the impl-tree, the notion of 'in-use' can become much more
    accurate, and the number of textures which are not safe to be evicted
    or recycled will be greatly reduced.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164131

[epenner, 2012-10-17 23:00:22]

Rebased. ptal

[danakj, 2012-10-17 23:19:37]

https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc
File cc/tiled_layer.cc (left):

https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc#oldcode620
cc/tiled_layer.cc:620: tile->dirtyRect = tileRect;
who's doing this now?

[epenner, 2012-10-17 23:33:14]

https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc
File cc/tiled_layer.cc (left):

https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc#oldcode620
cc/tiled_layer.cc:620: tile->dirtyRect = tileRect;
Throwing the texture away will make it dirty.... But... maybe that's not early
enough! I will look into it deeper. Thanks!

[danakj, 2012-10-17 23:39:44]

lgtm

https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc#newcode329
cc/tiled_layer.cc:329: if (!haveTexturesForTiles(left, top, right, bottom,
ignoreOcclusions)) {
I see. This will make it dirty now.

[epenner, 2012-10-18 00:30:19]

On 2012/10/17 23:39:44, danakj wrote:
> lgtm
> 
> https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc
> File cc/tiled_layer.cc (right):
> 
>
https://codereview.chromium.org/11028132/diff/6001/cc/tiled_layer.cc#newcode329
> cc/tiled_layer.cc:329: if (!haveTexturesForTiles(left, top, right, bottom,
> ignoreOcclusions)) {
> I see. This will make it dirty now.

Yes, but you were on to something! This code happens before the tile is
discarded. So we could calculate the paintRect using a partial dirty rect and
then discard the entire contents. I've moved the partial update code to a better
place in my new patch.

[danakj, 2012-10-18 00:39:04]

https://codereview.chromium.org/11028132/diff/15001/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

https://codereview.chromium.org/11028132/diff/15001/cc/tiled_layer.cc#newcode432
cc/tiled_layer.cc:432: tile->managedTexture()->returnBackingTexture();
Can we do this stuff before haveTexturesForTiles is called? Or perhaps inside
haveTextureForTiles? Then it will be very clear that this returnBackingTexture
happens before it checks if haveBackingTexture(). And we don't have to add
another tile->dirtyRect= anywhere.

[epennerAtGoogle, 2012-10-18 00:52:27]

https://codereview.chromium.org/11028132/diff/15001/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

https://codereview.chromium.org/11028132/diff/15001/cc/tiled_layer.cc#newcode432
cc/tiled_layer.cc:432: tile->managedTexture()->returnBackingTexture();
On 2012/10/18 00:39:05, danakj wrote:
> Can we do this stuff before haveTexturesForTiles is called? Or perhaps inside
> haveTextureForTiles? Then it will be very clear that this returnBackingTexture
> happens before it checks if haveBackingTexture(). And we don't have to add
> another tile->dirtyRect= anywhere.

I thought about that. But I kind of like it here since it's related to updating
the tile, right together with markForUpdate and expanding the paintRect.

The dirtyRect= in haveTextures seems out of place to me actually, but it's
that's the first thing that touches the tiles. It's almost like there should be
tile->dirtyRect() that checks the texture status every time.

[danakj, 2012-10-18 01:05:09]

On Wed, Oct 17, 2012 at 8:52 PM,  <epenner@google.com> wrote:
>
> https://codereview.chromium.org/11028132/diff/15001/cc/tiled_layer.cc
> File cc/tiled_layer.cc (right):
>
>
https://codereview.chromium.org/11028132/diff/15001/cc/tiled_layer.cc#newcode432
> cc/tiled_layer.cc:432: tile->managedTexture()->returnBackingTexture();
> On 2012/10/18 00:39:05, danakj wrote:
>>
>> Can we do this stuff before haveTexturesForTiles is called? Or perhaps
>
> inside
>>
>> haveTextureForTiles? Then it will be very clear that this
>
> returnBackingTexture
>>
>> happens before it checks if haveBackingTexture(). And we don't have to
>
> add
>>
>> another tile->dirtyRect= anywhere.
>
>
> I thought about that. But I kind of like it here since it's related to
> updating the tile, right together with markForUpdate and expanding the
> paintRect.

Hm, to me it feels like its dealing with "does the tile have a
texture?" more than with updating, as it is deciding whether to
release the tile's texture. I don't like that we previously checked if
(!tile->managedTexture()->haveBackingTexture()) and now we are
modifying that result. I feel like it's making the code more brittle
or difficult to understand. If we're going to change the result of
haveBackingTexture() we should do it before we query it. Then we only
need to respond to the value in haveBackingTexture() once. Right now
we do it in haveTexturesForTiles() and then we do it again,
implicitly, here.

> The dirtyRect= in haveTextures seems out of place to me actually, but
> it's that's the first thing that touches the tiles. It's almost like
> there should be tile->dirtyRect() that checks the texture status every
> time.

Maybe it is.. I just don't like having us duplicate this setting of
the dirtyRect if we don't need to. What if the check for
haveBackingTexture() moved into markTilesForUpdate() then?

> https://codereview.chromium.org/11028132/

[epenner, 2012-10-18 01:21:00]

> >> Can we do this stuff before haveTexturesForTiles is called? Or perhaps
inside haveTextureForTiles? Then it will be very clear that this
returnBackingTexture happens before it checks if haveBackingTexture(). And we
don't have to add another tile->dirtyRect= anywhere.

I like the idea this won't work. That function is meant to be totally passive as
far as updating is concerned since it can return early. So we shouldn't throw
away textures in there that may not be updated in the end. It's another reason
why maybe the dirtyRect shouldn't be set there either.

> > The dirtyRect= in haveTextures seems out of place to me actually, but
> > it's that's the first thing that touches the tiles. It's almost like
> > there should be tile->dirtyRect() that checks the texture status every
> > time.
> 
> Maybe it is.. I just don't like having us duplicate this setting of
> the dirtyRect if we don't need to. What if the check for
> haveBackingTexture() moved into markTilesForUpdate() then?

It's not totally duplicate since one is checking whether we have the texture in
the first place and the other is actively discarding it. However, I do like your
idea of moving it all in to markTilesForUpdate if we can. I can't see any reason
not to .. I'll look into it.

[danakj, 2012-10-18 01:35:45]

On Thu, Oct 18, 2012 at 1:21 AM,  <epenner@chromium.org> wrote:
>> >> Can we do this stuff before haveTexturesForTiles is called? Or perhaps
>
> inside haveTextureForTiles? Then it will be very clear that this
> returnBackingTexture happens before it checks if haveBackingTexture(). And
> we
> don't have to add another tile->dirtyRect= anywhere.
>
> I like the idea this won't work. That function is meant to be totally
> passive as
> far as updating is concerned since it can return early. So we shouldn't
> throw
> away textures in there that may not be updated in the end. It's another
> reason
> why maybe the dirtyRect shouldn't be set there either.


Oh, I see. Yeh if we move the dirtyRect setter then we can make that
function const as well to designate that it's not changing state,
right? Yay.

>> > The dirtyRect= in haveTextures seems out of place to me actually, but
>> > it's that's the first thing that touches the tiles. It's almost like
>> > there should be tile->dirtyRect() that checks the texture status every
>> > time.
>
>
>> Maybe it is.. I just don't like having us duplicate this setting of
>> the dirtyRect if we don't need to. What if the check for
>> haveBackingTexture() moved into markTilesForUpdate() then?
>
>
> It's not totally duplicate since one is checking whether we have the texture
> in
> the first place and the other is actively discarding it. However, I do like
> your
> idea of moving it all in to markTilesForUpdate if we can. I can't see any
> reason
> not to .. I'll look into it.

sg!

[danakj, 2012-10-18 01:51:38]

On Thu, Oct 18, 2012 at 12:30 AM,  <epenner@chromium.org> wrote:
> Yes, but you were on to something! This code happens before the tile is
> discarded. So we could calculate the paintRect using a partial dirty rect
> and
> then discard the entire contents. I've moved the partial update code to a
> better
> place in my new patch.

I guess this points at a missing unit test?

[epennerAtGoogle, 2012-10-18 02:52:17]

On 2012/10/18 01:51:38, danakj wrote:
> On Thu, Oct 18, 2012 at 12:30 AM,  <mailto:epenner@chromium.org> wrote:
> > Yes, but you were on to something! This code happens before the tile is
> > discarded. So we could calculate the paintRect using a partial dirty rect
> > and
> > then discard the entire contents. I've moved the partial update code to a
> > better
> > place in my new patch.
> 
> I guess this points at a missing unit test?

I added the const. I think this is a more involved refactor. We should move the
choice to buffer textures into another place entirely, possibly the updater or
maybe texture manager. Then it could just never discard data when it only gets
partial data in the update. There is tests covering partial updates that this
does pass.

[danakj, 2012-10-18 03:03:42]

On 2012/10/18 02:52:17, epennerAtGoogle wrote:
> I added the const. I think this is a more involved refactor. We should move
the
> choice to buffer textures into another place entirely, possibly the updater or
> maybe texture manager. Then it could just never discard data when it only gets
> partial data in the update. There is tests covering partial updates that this
> does pass.

Thanks! This LGTM! I am wondering about a test because the first version of this
change had ordering messed up but also passed tests. Maybe we should have a test
that would have failed there to make sure it doesn't get messed up by future
refactors - moving things into PTM etc.

https://codereview.chromium.org/11028132/diff/9002/cc/tiled_layer.cc
File cc/tiled_layer.cc (right):

https://codereview.chromium.org/11028132/diff/9002/cc/tiled_layer.cc#newcode391
cc/tiled_layer.cc:391:
occlusion->overdrawMetrics().didCullTilesForUpload(occludedTileCount);
Nice, thanks for this :)

[epennerAtGoogle, 2012-10-18 03:24:23]

> Thanks! This LGTM! I am wondering about a test because the first version of
this
> change had ordering messed up but also passed tests. Maybe we should have a
test
> that would have failed there to make sure it doesn't get messed up by future
> refactors - moving things into PTM etc.
> 

You are right, I'll think up a good way to test it. Although ideally this can
land before the big rename! ;)

[epenner, 2012-10-18 05:11:43]

Darn. Layout test trybot seem to be failing on masks since I moved that
dirtyRect= Maybe I missed something.

[danakj, 2012-10-18 16:52:41]

On Thu, Oct 18, 2012 at 1:11 AM,  <epenner@chromium.org> wrote:
> Darn. Layout test trybot seem to be failing on masks since I moved that
> dirtyRect= Maybe I missed something.

Ya those weren't failing in webkit ToT yesterday.

> https://codereview.chromium.org/11028132/

[enne (OOO), 2012-10-18 18:26:33]

I like this cleanup a lot.  lgtm, assuming you sort out the layout test mask
issue before landing.

https://codereview.chromium.org/11028132/diff/9002/cc/tiled_layer.h
File cc/tiled_layer.h (right):

https://codereview.chromium.org/11028132/diff/9002/cc/tiled_layer.h#newcode84
cc/tiled_layer.h:84: bool haveTexturesForTiles(int left, int top, int right, int
bottom, bool ignoreOcclusions) const;
<3.  I've wanted that change for a while.

[reveman, 2012-10-19 04:53:05]

On 2012/10/18 02:52:17, epennerAtGoogle wrote:
> On 2012/10/18 01:51:38, danakj wrote:
> > On Thu, Oct 18, 2012 at 12:30 AM,  <mailto:epenner@chromium.org> wrote:
> > > Yes, but you were on to something! This code happens before the tile is
> > > discarded. So we could calculate the paintRect using a partial dirty rect
> > > and
> > > then discard the entire contents. I've moved the partial update code to a
> > > better
> > > place in my new patch.
> > 
> > I guess this points at a missing unit test?
> 
> I added the const. I think this is a more involved refactor. We should move
the
> choice to buffer textures into another place entirely, possibly the updater or
> maybe texture manager. Then it could just never discard data when it only gets
> partial data in the update. There is tests covering partial updates that this
> does pass.

I like the idea of moving the buffered update choice into the texture manager.
The current mechanism is pretty dumb. It just allows a fixed number of
unbuffered (partial) updates and the first 12 updates we find will be used. It
doesn't in any way prioritize smaller updates over larger updates.

[commit-bot: I haz the power, 2012-10-25 00:11:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11028132/40001

[commit-bot: I haz the power, 2012-10-25 01:45:43]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-10-25 02:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11028132/40001

[commit-bot: I haz the power, 2012-10-25 07:17:47]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-10-25 17:08:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11028132/51002

[commit-bot: I haz the power, 2012-10-25 19:06:44]

Change committed as 164131