Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12865017:
Makes tile-creation lazy (Closed)
|
Created:
7 years, 9 months ago by whunt Modified:
7 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMakes tile-creation lazy
This cl contains quite a few changes. I'll try to enumerate all of them here:
1) Some cleanup of the PictureLayerTiling definition (moved all members to the bottom and sorted them by lifespan)
2) Added painted_rect to Tile. This allows us to determine quickly if a new tileing can use an old tile (if the required painted rect is contained within the old painted rect). This lets us re-use partially painted tiles when the layer-bounds shrinks but nothing else changes.
3) Added an interest_rect field to PictureLayerTiling (we should determine if this is redundant with last_interest_rect
4) Added a new constructor to PictureLayerTiling for the sake of lazy cloning. This constructor copies another PictureLayerTiling but does *not* copy the tiles or interest_rect. The copy occurs lazily.
5) Added a ManageTiles function that creates all of the tiles required for a layer from its interest rect. It's interfact takes both the new and old rects, which isn't really required and could be changed if desired. This function deletes tiles outside of an old interest rect and adds tiles from the new interest rect.
6) Changed the definition of CreateTile to lazily pull from where it can. It no longer assumes we don't already have a tile for that I,J location. It first checks to see if this tiling already has a tile that has a big enough paint rect. If it doesn't, it checks its sibling for a tile in that I,J to see if it has a tile with a big enough paint rect, if not it creates a new tile.
7) Added two functions to the layer client, one lets a Tiling get its sibling and one gets the layer's invalidation.
8) Added a SimpleIterator class to iterate over a rectangular region. This iterator is substantially simpler than the current iterator and only has the functionality requried to iterate over a rectangle. We should probably rename SimpleIterator to RectIterator and Iterator to CoverageIterator because it's really part of the coverage algorithm.
There's probably a few more tweaks here and there and names/APIs are final but I wanted to put this up for review now that it appears to work.
BUG=190816
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193762
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressing much of Enne's feedback and rebasing to tip of tree (still no unit tests) #Patch Set 3 : a bug fix and removal of several now useless functions #
Total comments: 34
Patch Set 4 : removing SetLayerBounds and everything associated, also some minor cleanup #Patch Set 5 : unit tests now compile #Patch Set 6 : fixing unit tests and an optimization to CreateTile #
Total comments: 2
Patch Set 7 : rebasing to tip of tree and addressing new compiler warnings #Patch Set 8 : rebased onto 13726013 #Patch Set 9 : adding VerifyLiveTiles unit test and the required support, plus two minor fixes #
Total comments: 17
Patch Set 10 : Rebasing to tip of tree and fixing all of Enne's comments #
Total comments: 5
Patch Set 11 : addressing last round of feedback #Messages
Total messages: 38 (0 generated)
Just making sure an email goes out.
Thanks for the patch! I like the direction that this is going in moving the creation/deletion into UpdateTilePriorities, but I have questions about the details. Also, would you mind adding some representative unittests for tile creation and deletion to picture_layer_tiling_unittest.cc? https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.... cc/layers/picture_layer_impl.cc:322: const gfx::Rect& content_rect, No. Please stay consistent with passing rect by value in the rest of cc. If you want to change this policy, make some measurements and put up a separate patch that changes it everywhere. Don't just change it in this one place. Localized churn based on differing personal opinion is not how we do things. https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.h File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.... cc/layers/picture_layer_impl.h:56: virtual const PictureLayerTiling* GetSibling(const PictureLayerTiling* tiling) OVERRIDE; naming bikeshed: GetSibling => GetTwinTiling Layers are siblings when they have the same parent and are in the same tree. We've been calling layers "twins" if they have the same layer id but exist in a different tree. These tilings are from "layer twins", so I think it'd be more clear to call them twin tilings. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (left): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:101: // Any tiles outside our new bounds are invalid and should be dropped. I think you still need this block in the SetLayerBounds function. When you change the TilingData's total size to something smaller, you will never get tile indices outside of that total size, and so you will never be able to reach the keys of these now-invalid tiles with any iteration over TilingData keys. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:36: PictureLayerTiling::PictureLayerTiling(const PictureLayerTiling& other, int) I'm assuming this "int" here is a way to have an overloaded copy constructor, but that's an awkward way to overload something. If you need to do something like this please just use the default constructor and have Clone() set exactly the parameters you need with a comment about why you're not copying tiles_ and last_interest_rect_. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:75: gfx::Rect paint_rect = tiling_data_.TileBoundsWithBorder(i, j); I don't understand this paint_rect code at all. Within a given tiling, no tiles overlap, except on the borders. It is guaranteed that no two tiles at different indices will contain the other, considering either the bounds or bounds with border. A tile at the same index in a different tiling will have the exact same tile bounds with border. This seems like something you shouldn't need to store and certainly something you should never need to compare against in this code. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:118: if (layer_bounds_.IsEmpty()) { I don't think you need to special case this. The code below does all of these things if layer_bounds_.IsEmpty(), except for changing the interest_rect (which I don't think is necessary). https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:138: interest_rect_ = gfx::Rect(); Why does this need to happen? https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:369: // ManageTiles(gfx::Rect(), interest_rect); ? https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:371: interest_rect_ = interest_rect; I don't think you need to store this. Is last_interest_rect_ not sufficient? https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:406: DCHECK(!priority.is_live); My expectation for this patch was that TilePriority::is_live was going to get removed and instead we would create / delete tiles as they came into / came out of the interest rect. Is that not the case? How does this loop differ from your ManageTiles deletion function? Are you planning to remove TilePriority::is_live in a follow-up patch? https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:517: for (Region::Iterator iter(free_region); iter.has_rect(); iter.next()) { Both of these loops look like a job for TilingData::DifferenceIterator. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:521: if (found != tiles_.end() && Did you mean || here? I think this intersects condition would be better done in the SetLayerBounds function (the block I suggested not removing), and then you could just intersect last and current interest rects with the content bounds freely. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.h:172: // An iterator that iterates over the TileMapKeys within a gfx::Rect. Do you need a new iterator class here? Why not just use TilingData::Iterator? On a related note, I do agree with your offhand CL description suggestion that PictureLayerTiling::Iterator and PictureLayerTilingSet::Iterator should probably be named CoverageIterator. I'll put up a quick patch to do that. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.h:176: const gfx::Rect& rect); // we assume rect is contained Please don't make multi-line comments at the end of a line of code. They're really awkward to reformat. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling_set.cc (left): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling_set.cc:58: tilings_.back()->Invalidate(invalidation); I like being able to get rid of this function. :)
https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.... cc/layers/picture_layer_impl.cc:322: const gfx::Rect& content_rect, On 2013/03/27 16:16:27, enne wrote: > No. Please stay consistent with passing rect by value in the rest of cc. If > you want to change this policy, make some measurements and put up a separate > patch that changes it everywhere. Don't just change it in this one place. > > Localized churn based on differing personal opinion is not how we do things. I'll switch it back but the policy needs to be changed: https://docs.google.com/a/google.com/document/d/1LMBDUB64tR5ji3FkHYJCYJb3cgCz... https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (left): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:101: // Any tiles outside our new bounds are invalid and should be dropped. On 2013/03/27 16:16:27, enne wrote: > I think you still need this block in the SetLayerBounds function. When you > change the TilingData's total size to something smaller, you will never get tile > indices outside of that total size, and so you will never be able to reach the > keys of these now-invalid tiles with any iteration over TilingData keys. The old interest rect still contains those tiles. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:36: PictureLayerTiling::PictureLayerTiling(const PictureLayerTiling& other, int) On 2013/03/27 16:16:27, enne wrote: > I'm assuming this "int" here is a way to have an overloaded copy constructor, > but that's an awkward way to overload something. If you need to do something > like this please just use the default constructor and have Clone() set exactly > the parameters you need with a comment about why you're not copying tiles_ and > last_interest_rect_. I agree, that's reasonable. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:75: gfx::Rect paint_rect = tiling_data_.TileBoundsWithBorder(i, j); On 2013/03/27 16:16:27, enne wrote: > I don't understand this paint_rect code at all. Within a given tiling, no tiles > overlap, except on the borders. It is guaranteed that no two tiles at different > indices will contain the other, considering either the bounds or bounds with > border. > > A tile at the same index in a different tiling will have the exact same tile > bounds with border. This seems like something you shouldn't need to store and > certainly something you should never need to compare against in this code. The paint rect has to do with layers that change size. When we create a tile, we give store the portion of that tile that's covered by the layers bounds. If we shrink the layers bounds this tile is still good but if we grow the layers bounds this tile is no longer good (assuming it was only partially covered by the layer to begin with). When we create tiles for areas in the difference of two interest rects we may be able to re-use the old tile. paint_rect is used to determine if this is possible. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:118: if (layer_bounds_.IsEmpty()) { On 2013/03/27 16:16:27, enne wrote: > I don't think you need to special case this. The code below does all of these > things if layer_bounds_.IsEmpty(), except for changing the interest_rect (which > I don't think is necessary). I agree, it's not entirely necessary. The check was in there before as an early-out and I left it. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:138: interest_rect_ = gfx::Rect(); On 2013/03/27 16:16:27, enne wrote: > Why does this need to happen? It shouldn't happen. This line was left over from before and can be deleted. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:369: // ManageTiles(gfx::Rect(), interest_rect); On 2013/03/27 16:16:27, enne wrote: > ? Old debugging code, will be deleted. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:371: interest_rect_ = interest_rect; On 2013/03/27 16:16:27, enne wrote: > I don't think you need to store this. Is last_interest_rect_ not sufficient? Not sure yet. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:517: for (Region::Iterator iter(free_region); iter.has_rect(); iter.next()) { On 2013/03/27 16:16:27, enne wrote: > Both of these loops look like a job for TilingData::DifferenceIterator. I'll give that a look. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:521: if (found != tiles_.end() && On 2013/03/27 16:16:27, enne wrote: > Did you mean || here? I think this intersects condition would be better done in > the SetLayerBounds function (the block I suggested not removing), and then you > could just intersect last and current interest rects with the content bounds > freely. No, I don't mean ||. In this case we're looking for tiles that both exist and are not in the new interest rect. Iterating over the content bounds is going to be less efficient than iterating over the last interest rect and require extra code in set-layer-bounds.
https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.cc File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12865017/diff/1/cc/layers/picture_layer_impl.... cc/layers/picture_layer_impl.cc:322: const gfx::Rect& content_rect, On 2013/03/27 17:42:02, whunt wrote: > On 2013/03/27 16:16:27, enne wrote: > > No. Please stay consistent with passing rect by value in the rest of cc. If > > you want to change this policy, make some measurements and put up a separate > > patch that changes it everywhere. Don't just change it in this one place. > > > > Localized churn based on differing personal opinion is not how we do things. > > I'll switch it back but the policy needs to be changed: > https://docs.google.com/a/google.com/document/d/1LMBDUB64tR5ji3FkHYJCYJb3cgCz... I'm aware of your doc. This is outside the scope of this code review, but if you want to change this behavior, make some measurements to demonstrate that this has a performance impact and is worth changing, and I'll be convinced enough to lgtâ…¿ changing it everywhere. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (left): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:101: // Any tiles outside our new bounds are invalid and should be dropped. On 2013/03/27 17:42:02, whunt wrote: > On 2013/03/27 16:16:27, enne wrote: > > I think you still need this block in the SetLayerBounds function. When you > > change the TilingData's total size to something smaller, you will never get > tile > > indices outside of that total size, and so you will never be able to reach the > > keys of these now-invalid tiles with any iteration over TilingData keys. > > The old interest rect still contains those tiles. It does, but when you change TilingData's total size to something smaller you will never get those tile indices that are outside the bounds of that total size. These tiles are unreachable via the iterators below. https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... cc/resources/picture_layer_tiling.cc:75: gfx::Rect paint_rect = tiling_data_.TileBoundsWithBorder(i, j); On 2013/03/27 17:42:02, whunt wrote: > On 2013/03/27 16:16:27, enne wrote: > > I don't understand this paint_rect code at all. Within a given tiling, no > tiles > > overlap, except on the borders. It is guaranteed that no two tiles at > different > > indices will contain the other, considering either the bounds or bounds with > > border. > > > > A tile at the same index in a different tiling will have the exact same tile > > bounds with border. This seems like something you shouldn't need to store and > > certainly something you should never need to compare against in this code. > > The paint rect has to do with layers that change size. When we create a tile, > we give store the portion of that tile that's covered by the layers bounds. If > we shrink the layers bounds this tile is still good but if we grow the layers > bounds this tile is no longer good (assuming it was only partially covered by > the layer to begin with). When we create tiles for areas in the difference of > two interest rects we may be able to re-use the old tile. paint_rect is used to > determine if this is possible. Ah, I see. Storing a paint rect on every tile still seems like overkill to me. I think a more elegant way to handle this might be to add synthetic invalidations to the invalidation region when the layer bounds grows. Then a difference of layer bounds sizes between layer twins would be handled by the general invalidation case.
This sounds awesome. Just one drive by comment. 5) Added a ManageTiles function that creates all of the tiles required for a layer from its interest rect. It's interfact takes both the new and old rects, which isn't really required and could be changed if desired. This function deletes tiles outside of an old interest rect and adds tiles from the new interest rect. Just a thought: Could the fact that a tile fails to acquire memory be used to assist in removing tiles? If feels like culling all painted content outside of an interest rect might be prematurely culling content that we could easily cache. The memory budget (and if really needed a much larger culling rect), will always keep the tile count low.
On 2013/03/27 20:27:45, epenner wrote: > This sounds awesome. Just one drive by comment. > > 5) Added a ManageTiles function that creates all of the tiles required for a > layer from its interest rect. It's interfact takes both the new and old rects, > which isn't really required and could be changed if desired. This function > deletes tiles outside of an old interest rect and adds tiles from the new > interest rect. > > Just a thought: Could the fact that a tile fails to acquire memory be used to > assist in removing tiles? If feels like culling all painted content outside of > an interest rect might be prematurely culling content that we could easily > cache. The memory budget (and if really needed a much larger culling rect), will > always keep the tile count low. I think that this is the same argument that ccameron has made before, in that we should be more stateful about keeping tiles around that are outside of the interest rect if they still have content and then only deleting them when they no longer are allowed to have gpu memory. I don't disagree that this could save some some unnecessary raster work, but I also think it creates extra complexity. I'd rather do one step at a time and measure how often we fall down that path before we commit to that optimization. https://codereview.chromium.org/12865017/diff/1/cc/resources/tile.h File cc/resources/tile.h (right): https://codereview.chromium.org/12865017/diff/1/cc/resources/tile.h#newcode94 cc/resources/tile.h:94: gfx::Rect paint_rect_; As a follow-up to whunt's offline question about storing paint_rect instead of content_rect here, I don't think that's possible. Two tilings can share the same tile using the same picture pile but have different paint rects (because the layer size shrank, for example). Between paint rect and content rect, the content rect of the tile is the constant shared piece of information between both tilings using that tile.
https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:854: pending_layer->tilings_->Invalidate(gfx::Rect(bounds())); This is incorrect to remove wholesale. Invalidate both deletes and creates new tiles. Your patch defers creation, but I think you still need to somehow do the deletion here because there now a bunch of tiles that need to be rerastered. https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:585: if (layer_tree_impl()->IsPendingTree()) This if statement can be simplified. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (left): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:387: DCHECK(!priority.is_live); Can you also remove is_live from tile priority and the tile manager in this patch, as all tiles will be marked live with your change? https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:25: PictureLayerTiling* out = new PictureLayerTiling(contents_scale_); style nit: please don't ever use unwrapped raw pointers with no owner. You could instead say: scoped_ptr<PictureLayerTiling> clone(new PictureLayerTiling(contents_scale_)); // ... return clone.Pass(); https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:79: if (candidate_tile && tiling_data_.max_texture_size() == These two conditions aren't really tied together. Maybe you should check the max texture size up front before you even get the twin tiling? https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:82: // Loop over the invalidation region explicitly because we have As a simplification suggestion, what about scaling the tile bounds to layer space and then just using Region::Intersects? https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:136: interest_rect_ = gfx::Rect(); https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:141: for (TilingData::DifferenceIterator iter(&tiling_data_, I think this loop will be a no-op because you have already set the tiling data's total size above to something smaller than old_content_bounds. I think if you write a unit test for this and you will see what I am trying to get at. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:457: iter; ++iter) { style nit: Could you put "iter" and "++iter" on different lines if your for loop doesn't fit on one line, here and elsewhere? I find that easier to read, and it's what clang-format does automatically. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:461: !new_interest_rect.Intersects(found->second->content_rect())) What are you trying to get at with this condition? https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:171: gfx::Rect interest_rect_; Can you call this last_interest_rect_ to match the other two variables? https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:178: DISALLOW_ASSIGN(PictureLayerTiling); style nit: DISALLOW_etc goes last in a class declaration. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_set.cc (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_set.cc:48: const PictureLayerTiling* tiling) { style nit: this can fit on one line now.
https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:854: pending_layer->tilings_->Invalidate(gfx::Rect(bounds())); On 2013/03/29 20:50:58, enne wrote: > This is incorrect to remove wholesale. Invalidate both deletes and creates new > tiles. Your patch defers creation, but I think you still need to somehow do the > deletion here because there now a bunch of tiles that need to be rerastered. Good call, I'll investigate the best way to keep the correct behavior. https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:585: if (layer_tree_impl()->IsPendingTree()) On 2013/03/29 20:50:58, enne wrote: > This if statement can be simplified. Clearly. I'll go do that. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (left): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:387: DCHECK(!priority.is_live); On 2013/03/29 20:50:58, enne wrote: > Can you also remove is_live from tile priority and the tile manager in this > patch, as all tiles will be marked live with your change? From my understanding, all tiles should be live (we no longer have a loop that sets them to dead either). Would you like me to roll that into this patch? I talked to Nat and decided that removing the live_or_allocated_tiles from tile-manager was outside the scope of this cl but is_live seems closer. It's your call. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:25: PictureLayerTiling* out = new PictureLayerTiling(contents_scale_); On 2013/03/29 20:50:58, enne wrote: > style nit: please don't ever use unwrapped raw pointers with no owner. > > You could instead say: > scoped_ptr<PictureLayerTiling> clone(new PictureLayerTiling(contents_scale_)); > // ... > return clone.Pass(); Okay, this is a style nit through right? Due to out being local to the stack before the return and no exceptions occurring the current code cannot ever lose the pointer before calling make_scoped_ptr, corret? https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:79: if (candidate_tile && tiling_data_.max_texture_size() == On 2013/03/29 20:50:58, enne wrote: > These two conditions aren't really tied together. Maybe you should check the > max texture size up front before you even get the twin tiling? done. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:82: // Loop over the invalidation region explicitly because we have On 2013/03/29 20:50:58, enne wrote: > As a simplification suggestion, what about scaling the tile bounds to layer > space and then just using Region::Intersects? Is that safe? We did some rounding when generated these tile bounds in the first place. I'm not sure if the inverse scaling would be conservative given we push the points of the tiles around to avoid cracks and overlap? I guess we're comparing scaled values and scales *shouldn't* affect the comparison. I was following what was done before (which looked like this), but you know this better than I do. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:136: interest_rect_ = gfx::Rect(); On 2013/03/29 20:50:58, enne wrote: > https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... I think my previous response was just incorrect. If we shrink the tile slightly and let the old tile remain the lazy walk will discover no new area and generate no new tiles. We need to set the interest_rect_ to null if we clear the tiles, that way they get re-created correctly. This can be tested for. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:141: for (TilingData::DifferenceIterator iter(&tiling_data_, On 2013/03/29 20:50:58, enne wrote: > I think this loop will be a no-op because you have already set the tiling data's > total size above to something smaller than old_content_bounds. I think if you > write a unit test for this and you will see what I am trying to get at. I don't understand. If the new content bounds are bigger than the old content bounds, we need to invalidate the partially pained edge tiles. This difference iterator should cover those tiles. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:457: iter; ++iter) { On 2013/03/29 20:50:58, enne wrote: > style nit: Could you put "iter" and "++iter" on different lines if your for loop > doesn't fit on one line, here and elsewhere? I find that easier to read, and > it's what clang-format does automatically. Sure. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:461: !new_interest_rect.Intersects(found->second->content_rect())) On 2013/03/29 20:50:58, enne wrote: > What are you trying to get at with this condition? we're trying to find tiles that are entirely outside of our new interest rect. The difference iterator between the new and old interest rects can find border tiles that are not outside the new interest rect and do not need to be erased. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:171: gfx::Rect interest_rect_; On 2013/03/29 20:50:58, enne wrote: > Can you call this last_interest_rect_ to match the other two variables? I *can*, it doesn't quite follow the same semantics though. The two "last_" variables are used for finite differences for the sake of calculating motion. The "interest_rect_" could really be called "live_tiles_rect", it's used in a variety of places and set by functions other than UpdateTileProperties. I don't think it's appropriate to lump it in with the others. To be fair, I left it grouped with them in the definition here and I should move it. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:178: DISALLOW_ASSIGN(PictureLayerTiling); On 2013/03/29 20:50:58, enne wrote: > style nit: DISALLOW_etc goes last in a class declaration. Ahh, okay, this is where it merged in, I'll move it. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_set.cc (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_set.cc:48: const PictureLayerTiling* tiling) { On 2013/03/29 20:50:58, enne wrote: > style nit: this can fit on one line now. Done.
https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/12865017/diff/15001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:854: pending_layer->tilings_->Invalidate(gfx::Rect(bounds())); On 2013/03/29 21:52:04, whunt wrote: > On 2013/03/29 20:50:58, enne wrote: > > This is incorrect to remove wholesale. Invalidate both deletes and creates > new > > tiles. Your patch defers creation, but I think you still need to somehow do > the > > deletion here because there now a bunch of tiles that need to be rerastered. > > Good call, I'll investigate the best way to keep the correct behavior. On second thought, I'm not actually sure, we should discuss.
https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (left): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:387: DCHECK(!priority.is_live); On 2013/03/29 21:52:04, whunt wrote: > On 2013/03/29 20:50:58, enne wrote: > > Can you also remove is_live from tile priority and the tile manager in this > > patch, as all tiles will be marked live with your change? > > From my understanding, all tiles should be live (we no longer have a loop that > sets them to dead either). Would you like me to roll that into this patch? I > talked to Nat and decided that removing the live_or_allocated_tiles from > tile-manager was outside the scope of this cl but is_live seems closer. It's > your call. That's fine to leave it out and follow up later. I just wasn't privy to that conversation where that was decided and just wanted to know what your plan was. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:25: PictureLayerTiling* out = new PictureLayerTiling(contents_scale_); On 2013/03/29 21:52:04, whunt wrote: > On 2013/03/29 20:50:58, enne wrote: > > style nit: please don't ever use unwrapped raw pointers with no owner. > > > > You could instead say: > > scoped_ptr<PictureLayerTiling> clone(new PictureLayerTiling(contents_scale_)); > > // ... > > return clone.Pass(); > > Okay, this is a style nit through right? Due to out being local to the stack > before the return and no exceptions occurring the current code cannot ever lose > the pointer before calling make_scoped_ptr, corret? Yes. Your code as written is logically correct. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:82: // Loop over the invalidation region explicitly because we have On 2013/03/29 21:52:04, whunt wrote: > On 2013/03/29 20:50:58, enne wrote: > > As a simplification suggestion, what about scaling the tile bounds to layer > > space and then just using Region::Intersects? > > Is that safe? We did some rounding when generated these tile bounds in the > first place. I'm not sure if the inverse scaling would be conservative given we > push the points of the tiles around to avoid cracks and overlap? I guess we're > comparing scaled values and scales *shouldn't* affect the comparison. I was > following what was done before (which looked like this), but you know this > better than I do. I think this is safe to do in both directions if you do it conservatively using ToEnclosingRect. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:136: interest_rect_ = gfx::Rect(); On 2013/03/29 21:52:04, whunt wrote: > On 2013/03/29 20:50:58, enne wrote: > > > https://codereview.chromium.org/12865017/diff/1/cc/resources/picture_layer_ti... > > I think my previous response was just incorrect. If we shrink the tile slightly > and let the old tile remain the lazy walk will discover no new area and generate > no new tiles. We need to set the interest_rect_ to null if we clear the tiles, > that way they get re-created correctly. This can be tested for. Ah, that makes a lot of sense. Thanks for the explanation. https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:141: for (TilingData::DifferenceIterator iter(&tiling_data_, On 2013/03/29 21:52:04, whunt wrote: > On 2013/03/29 20:50:58, enne wrote: > > I think this loop will be a no-op because you have already set the tiling > data's > > total size above to something smaller than old_content_bounds. I think if you > > write a unit test for this and you will see what I am trying to get at. > > I don't understand. If the new content bounds are bigger than the old content > bounds, we need to invalidate the partially pained edge tiles. This difference > iterator should cover those tiles. Put another way, what is the maximum index that TilingData::Iterator or TilingData::DifferenceIterator will consider? https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:171: gfx::Rect interest_rect_; On 2013/03/29 21:52:04, whunt wrote: > On 2013/03/29 20:50:58, enne wrote: > > Can you call this last_interest_rect_ to match the other two variables? > > I *can*, it doesn't quite follow the same semantics though. The two "last_" > variables are used for finite differences for the sake of calculating motion. > The "interest_rect_" could really be called "live_tiles_rect", it's used in a > variety of places and set by functions other than UpdateTileProperties. I don't > think it's appropriate to lump it in with the others. To be fair, I left it > grouped with them in the definition here and I should move it. By "a variety of places" you mean "can be cleared in SetLayerBounds"? I think the tile map and the interest rect are tied together, where they both need to be updated together. It's still semantically "the last time you updated the tiles" for the sake of generating a diff.
https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (left): https://codereview.chromium.org/12865017/diff/15001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:387: DCHECK(!priority.is_live); On 2013/03/29 22:34:20, enne wrote: > On 2013/03/29 21:52:04, whunt wrote: > > On 2013/03/29 20:50:58, enne wrote: > > > Can you also remove is_live from tile priority and the tile manager in this > > > patch, as all tiles will be marked live with your change? > > > > From my understanding, all tiles should be live (we no longer have a loop that > > sets them to dead either). Would you like me to roll that into this patch? I > > talked to Nat and decided that removing the live_or_allocated_tiles from > > tile-manager was outside the scope of this cl but is_live seems closer. It's > > your call. > > That's fine to leave it out and follow up later. I just wasn't privy to that > conversation where that was decided and just wanted to know what your plan was. It was just an informal discussion in the cubes like 3 hours ago. I offered to pull in more to this patch and his suggestion was to keep it more manageable for the reviewers (which is mostly you, so is you're call =) )
https://codereview.chromium.org/12865017/diff/32001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/12865017/diff/32001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:854: pending_layer->tilings_->Invalidate(gfx::Rect(bounds())); Where do tiles get recreated here because of this new invalidation?
https://codereview.chromium.org/12865017/diff/32001/cc/layers/picture_layer_i... File cc/layers/picture_layer_impl.cc (left): https://codereview.chromium.org/12865017/diff/32001/cc/layers/picture_layer_i... cc/layers/picture_layer_impl.cc:854: pending_layer->tilings_->Invalidate(gfx::Rect(bounds())); On 2013/04/05 21:30:02, enne wrote: > Where do tiles get recreated here because of this new invalidation? Whoops, forgot to handle this. I'll fix it.
On 2013/04/05 21:33:25, whunt wrote: > https://codereview.chromium.org/12865017/diff/32001/cc/layers/picture_layer_i... > File cc/layers/picture_layer_impl.cc (left): > > https://codereview.chromium.org/12865017/diff/32001/cc/layers/picture_layer_i... > cc/layers/picture_layer_impl.cc:854: > pending_layer->tilings_->Invalidate(gfx::Rect(bounds())); > On 2013/04/05 21:30:02, enne wrote: > > Where do tiles get recreated here because of this new invalidation? > > Whoops, forgot to handle this. I'll fix it. On a related note: https://codereview.chromium.org/13726013/
The code looks pretty reasonable. Do you have any measurements on performance change here? Does the TileManager get faster due to not having to deal with is_live=false tiles? Is there a performance decrease anywhere due to more indirection through the tiling client?
On 2013/04/08 21:11:48, enne wrote: > The code looks pretty reasonable. Do you have any measurements on performance > change here? Does the TileManager get faster due to not having to deal with > is_live=false tiles? Is there a performance decrease anywhere due to more > indirection through the tiling client? I have no measured performance deltas. I can look into that.
On 2013/04/09 18:27:13, whunt wrote: > On 2013/04/08 21:11:48, enne wrote: > > The code looks pretty reasonable. Do you have any measurements on performance > > change here? Does the TileManager get faster due to not having to deal with > > is_live=false tiles? Is there a performance decrease anywhere due to more > > indirection through the tiling client? > > I have no measured performance deltas. I can look into that. Traces for bulbgarden show that time in UpdateDrawProperties increases by about 50% and time in ManageTiles decreases by 50%. Because we spend about 10x as much time in ManageTiles as UpdateDrawProperties it's a significant net win. It also removes the jank associated with creating and destroying large numbers of tiles at the same time.
Could you share the absolute numbers here too, just for reference? I'm assuming that ManageTiles is faster because it only knows about the live tiles and UpdateDrawProperties is slower because UpdateTileProperties is slower? Do you have any more detailed sense of where that slow down comes from? We've certainly had issues with UpdateTileProperties being slow in the past (as you well know), so I'd be curious to know more precisely what the costly work is here.
On 2013/04/09 21:36:37, enne wrote: > Could you share the absolute numbers here too, just for reference? > > I'm assuming that ManageTiles is faster because it only knows about the live > tiles and UpdateDrawProperties is slower because UpdateTileProperties is slower? > > Do you have any more detailed sense of where that slow down comes from? We've > certainly had issues with UpdateTileProperties being slow in the past (as you > well know), so I'd be curious to know more precisely what the costly work is > here. Measured average times over lots of frames on a Z620: Prepatch: UpdateDrawProperties: 0.09ms ManageTiles: 1ms Postpatch: UpdateDrawProperties: 0.13ms ManageTiles: 0.55ms The most major performance problem I was aware of with UpdateTileProperties had to do with lots of non-inlined math being called on lots of tiles. All costs that used to be in SetLayerBounds have moved into UpdateTilePriorities, so we're allocating tiles inside UpdateTileProperties now, which certainly has cost. Additionally, I scroll up and down the page many times during my traces so tiles get destroyed and recreated multiple times as the interest rect moves around. This is a penalty for making live tiles track the interest rect, we destroy a few and create a few very frequently during scrolling. There's no jank but a little bit of extra cost. Of course if I hadn't scrolled around as much we could have created fewer tiles than when they were created eagerly, but that's not what I tested.
Ok, thanks for all the detailed info. lgtm
Are you looking for another review here? What were you fixing?
On 2013/04/10 19:27:07, enne wrote: > Are you looking for another review here? What were you fixing? I changed two DCHECKS to code that enforces the behavior. One is in CreateTile that erases an already existing tile if we try to create a new one over it and one in SetLiveTilesRect that clips the interest rect to the content rect. Previously this was assumed but I made the method public for the sake of testing and now that it's public we need to enforce the assumption. I also added a unit test that for a variety of configurations makes sure that all of the tiles created actually touch the interest rect. There will be one more patch to rebase to tip of tree (coming in about 10 minutes).
https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:84: tiles_.erase(key); I liked the dcheck better. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:438: const gfx::Rect& new_live_tiles_rect_input) { This function should take a gfx::Rect by value. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:440: gfx::Rect content_rect = I'd really rather just dcheck this than intersect twice. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:455: if (found != tiles_.end()) { This change seems unnecessary. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:160: void SetLiveTilesRect(const gfx::Rect& live_tiles_rect); I don't like making all of these functions public for testing. It gives the impression that these are functions that external callers could call. I'd be happier with leaving them protected and having a TestPictureLayerTiling class that implemented VerifyTilesExistInLiveRect, or some such. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:161: void CreateTile(int i, int j); This doesn't look like it needs to be exposed. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:29: void VerifyTilesExistOnlyInLiveRect(gfx::Rect live_tiles_rect) { This function is named oddly. Maybe SetLiveRectAndVerifyTiles would be a better name. The name as-is makes it sound like it should be const. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:32: for (TilingData::Iterator iter(&tiling_->tiling_data(), Shouldn't you iterate over all tiles and not just the live tiles rect?
https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:84: tiles_.erase(key); On 2013/04/10 20:25:30, enne wrote: > I liked the dcheck better. The DCHECK is wrong given the way that SetLiveTilesRect works. If we grow slightly nothing gets deleted because there are no regions that were previously exposed that aren't any more. Because the grown regions need to be repainted this code handles that during the second loop that creates tiles for newly exposed regions. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:440: gfx::Rect content_rect = On 2013/04/10 20:25:30, enne wrote: > I'd really rather just dcheck this than intersect twice. If SetLiveTilesRect can be made private we can use the DCHECK, otherwise it needs to stay the way it is. I'll look into it. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:455: if (found != tiles_.end()) { On 2013/04/10 20:25:30, enne wrote: > This change seems unnecessary. It was introduced during debugging and can be deleted. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.h:160: void SetLiveTilesRect(const gfx::Rect& live_tiles_rect); On 2013/04/10 20:25:30, enne wrote: > I don't like making all of these functions public for testing. It gives the > impression that these are functions that external callers could call. > > I'd be happier with leaving them protected and having a TestPictureLayerTiling > class that implemented VerifyTilesExistInLiveRect, or some such. It's not unreasonable to add SetLiveTilesRect to the interface. I can see what I can do to hide it. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:29: void VerifyTilesExistOnlyInLiveRect(gfx::Rect live_tiles_rect) { On 2013/04/10 20:25:30, enne wrote: > This function is named oddly. Maybe SetLiveRectAndVerifyTiles would be a better > name. The name as-is makes it sound like it should be const. sure. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:32: for (TilingData::Iterator iter(&tiling_->tiling_data(), On 2013/04/10 20:25:30, enne wrote: > Shouldn't you iterate over all tiles and not just the live tiles rect? Yes, it should be iterating over the content rect, I thought it was. hmmm...
https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:84: tiles_.erase(key); On 2013/04/10 20:52:30, whunt wrote: > On 2013/04/10 20:25:30, enne wrote: > > I liked the dcheck better. > > The DCHECK is wrong given the way that SetLiveTilesRect works. If we grow > slightly nothing gets deleted because there are no regions that were previously > exposed that aren't any more. Because the grown regions need to be repainted > this code handles that during the second loop that creates tiles for newly > exposed regions. What do you mean by grow slightly? I thought all the layer size changes were handled by passing the layer size to the constructor. https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:32: for (TilingData::Iterator iter(&tiling_->tiling_data(), On 2013/04/10 20:52:30, whunt wrote: > On 2013/04/10 20:25:30, enne wrote: > > Shouldn't you iterate over all tiles and not just the live tiles rect? > > Yes, it should be iterating over the content rect, I thought it was. hmmm... No, it's more than that. I'm saying that you need to iterate over the tile map and not over a rect.
https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/61001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:84: tiles_.erase(key); On 2013/04/10 21:21:24, enne wrote: > On 2013/04/10 20:52:30, whunt wrote: > > On 2013/04/10 20:25:30, enne wrote: > > > I liked the dcheck better. > > > > The DCHECK is wrong given the way that SetLiveTilesRect works. If we grow > > slightly nothing gets deleted because there are no regions that were > previously > > exposed that aren't any more. Because the grown regions need to be repainted > > this code handles that during the second loop that creates tiles for newly > > exposed regions. > > What do you mean by grow slightly? I thought all the layer size changes were > handled by passing the layer size to the constructor. I mean grow the interest rect. We use difference iterators over the new and old interest rects, we delete things in the difference between old - new and add things in new - old. However, some of the tiles in the new - old region already exist but are not fully painted... oohh they're all always painted right because we don't have a SetLayerBounds call anymore?
Yeah, that's exactly what I was trying to say. They should all be all entirely painted.
https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_laye... cc/resources/picture_layer_tiling.cc:441: DCHECK(content_rect.Contains(new_live_tiles_rect)); s/content_rect/ContentRect()/ https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_laye... File cc/resources/picture_layer_tiling_unittest.cc (right): https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:52: tiling_->SetLiveTilesRect(live_tiles_rect); // max tiles in tile manager I don't think this comment is true, and could just be removed. https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:56: iter != tiles.end(); style nit: don't indent 5 spaces after a for loop https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_laye... cc/resources/picture_layer_tiling_unittest.cc:58: EXPECT_EQ(*iter != NULL, Now that the loop has changed, this is a little bit weird. I think you should only be checking the second condition. Every tile in the map should be true. If you're worried about NULL tiles, then AllTilesForTesting would be a better place to check for that. https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_pile... File cc/resources/picture_pile_base.h (right): https://codereview.chromium.org/12865017/diff/79001/cc/resources/picture_pile... cc/resources/picture_pile_base.h:42: TilingData& tiling() { return tiling_; } I don't think this needs to be exposed.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/whunt@chromium.org/12865017/91001
Message was sent while issue was closed.
Change committed as 193762 |
