Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
219963005:
cc: Add support for partial swaps when using impl-side painting. (Closed)
|
Created:
6 years, 8 months ago by sohanjg Modified:
5 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptioncc: Add support for partial swaps when using impl-side painting by only
damaging viewport area covered by newly initialized tiles.
In the current implementation, during frame draw
and visible tile update, LTHI was setting damage
to the entire viewport area, irrespective of the
initialized tiled content rect.
As part of the patch we are trying to ascertain newly
initialized tile rect from TileManager, notify LTHI about it,
and save it as damage rect in LayerImpl.
During frame draw, when the damage for surfaces and
layers are tracked, we query for these damaged layer rects
and unify with the DamageTracker's damage rect.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266852
Patch Set 1 #
Total comments: 1
Patch Set 2 : WIP - optimize damage rect in LTHI #
Total comments: 1
Patch Set 3 : WIP-transform tiled rect to screen space #Patch Set 4 : WIP - move damage tracking to DamageTracker #
Total comments: 17
Patch Set 5 : WIP - avoid std::map in TileManager + comments #
Total comments: 20
Patch Set 6 : WIP - comments + nits #
Total comments: 12
Patch Set 7 : WIP - renaming + comments #
Total comments: 6
Patch Set 8 : unit tests added + other comments #
Total comments: 3
Patch Set 9 : unit test comments updated #Patch Set 10 : use Contains() api instead of custom code #
Total comments: 2
Patch Set 11 : Unit tests updated #Patch Set 12 : resolve cc_unittests build error #
Messages
Total messages: 46 (1 generated)
This is just a rough draft to check whether the direction is correct. PTAL. Thanks. https://codereview.chromium.org/219963005/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/219963005/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:1200: DrawViewportSize().width() * DrawViewportSize().height()) we may optimize this check.
https://codereview.chromium.org/219963005/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/219963005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:1241: damage_rect_ = gfx::UnionRects(damage_rect_, tile->content_rect()); What space is the damage_rect_ in? If it's in screen space, you'd have to transform the tile's rect here. Atm it will be a combination of all possible layers' content spaces, which won't work when layers have transforms etc.
PTAL. Thanks.
Is this approach correct danakj@, reveman@ ?
On 2014/04/08 02:51:24, sohanjg wrote: > Is this approach correct danakj@, reveman@ ? I don't think this logic belongs in the tile manager and I don't think we can get away with doing this much work for each tile. Try integrating this with the existing damage tracker class instead.
On 2014/04/08 05:37:49, reveman wrote: > On 2014/04/08 02:51:24, sohanjg wrote: > > Is this approach correct danakj@, reveman@ ? > > I don't think this logic belongs in the tile manager and I don't think we can > get away with doing this much work for each tile. Try integrating this with the > existing damage tracker class instead. DamageTracker has no concept of tiles, it adds the layer's bounds and/or the layer's update_rect_. Can you explain what you're hoping for? Would a tile on a layer damage that entire layer instead of the entire viewport?
On 2014/04/08 14:31:59, danakj wrote: > On 2014/04/08 05:37:49, reveman wrote: > > On 2014/04/08 02:51:24, sohanjg wrote: > > > Is this approach correct danakj@, reveman@ ? > > > > I don't think this logic belongs in the tile manager and I don't think we can > > get away with doing this much work for each tile. Try integrating this with > the > > existing damage tracker class instead. > > DamageTracker has no concept of tiles, it adds the layer's bounds and/or the > layer's update_rect_. Can you explain what you're hoping for? Would a tile on a > layer damage that entire layer instead of the entire viewport? I was hoping that the update_rect_ could reflect damage from newly initialized tiles. Or if update_rect_ is not appropriate we can add something new. Damaging the entire layer could be a start but ideally we would have a bounding box of all damage from initialized tiles per layer.
Do you have an existing use case where this causes problems, or is this just a drive-by cleanup? I agree with reveman that this seems like a lot of complication and maps, and we'd do all of this for cases where partial swap isn't enabled. On cases where we initialize a ton of tiles, this seems like it could get needlessly inefficient.
On 2014/04/08 16:01:45, enne wrote: > Do you have an existing use case where this causes problems, or is this just a > drive-by cleanup? > > I agree with reveman that this seems like a lot of complication and maps, and > we'd do all of this for cases where partial swap isn't enabled. On cases where > we initialize a ton of tiles, this seems like it could get needlessly > inefficient. Well, it was originally a clean-up. But, when i saw the initialized tile's rect and what we are setting in LTHI::DidInitializeVisibleTile (i.e set damage_rect as entire viewport), there are times when we have initialized just 1 384X384 tile, but end up setting damge rect as entire viewport, which can be avoided. Regarding the implementation, ::onrastertaskcompleted, the operation is quite light, just add to the map. When DidInitializeVisibleTile, queries for the damaged rect, is when we do some calculations. I will take a trace and post it here maybe for time consumed during the operation. Regarding moving the implementation to damage tracker, we need to give the tile's content-rect and layer-id map to it, and it can traverse over its layer list to get the transform for the particular layer id, and apply to unioned rect. does that sound good ?
On Wed, Apr 9, 2014 at 6:11 AM, <sohan.jyoti@samsung.com> wrote: > On 2014/04/08 16:01:45, enne wrote: > >> Do you have an existing use case where this causes problems, or is this >> just a >> drive-by cleanup? >> > > I agree with reveman that this seems like a lot of complication and maps, >> and >> we'd do all of this for cases where partial swap isn't enabled. On cases >> > where > >> we initialize a ton of tiles, this seems like it could get needlessly >> inefficient. >> > > Well, it was originally a clean-up. > But, when i saw the initialized tile's rect and what we are setting in > LTHI::DidInitializeVisibleTile (i.e set damage_rect as entire viewport), > there > are times when we have initialized just 1 384X384 tile, but end up setting > damge > rect as entire viewport, which can be avoided. > > Regarding the implementation, ::onrastertaskcompleted, the operation is > quite > light, just add to the map. > When DidInitializeVisibleTile, queries for the damaged rect, is when we do > some > calculations. I will take a trace and post it here maybe for time consumed > during the operation. > > Regarding moving the implementation to damage tracker, we need to give the > tile's content-rect and layer-id map to it, and it can traverse over its > layer > list to get the transform for the particular layer id, and apply to unioned > rect. > does that sound good ? > I don't think this should talk to the damage tracker directly. Rather it could do what reveman suggested, and update the layer's update_rect_ which doesn't require any transform mapping either, incidentally. > > https://codereview.chromium.org/219963005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Apr 9, 2014 at 11:45 AM, Dana Jansens <danakj@chromium.org> wrote: > On Wed, Apr 9, 2014 at 6:11 AM, <sohan.jyoti@samsung.com> wrote: > >> On 2014/04/08 16:01:45, enne wrote: >> >>> Do you have an existing use case where this causes problems, or is this >>> just a >>> drive-by cleanup? >>> >> >> I agree with reveman that this seems like a lot of complication and >>> maps, and >>> we'd do all of this for cases where partial swap isn't enabled. On cases >>> >> where >> >>> we initialize a ton of tiles, this seems like it could get needlessly >>> inefficient. >>> >> >> Well, it was originally a clean-up. >> But, when i saw the initialized tile's rect and what we are setting in >> LTHI::DidInitializeVisibleTile (i.e set damage_rect as entire viewport), >> there >> are times when we have initialized just 1 384X384 tile, but end up >> setting damge >> rect as entire viewport, which can be avoided. >> >> Regarding the implementation, ::onrastertaskcompleted, the operation is >> quite >> light, just add to the map. >> When DidInitializeVisibleTile, queries for the damaged rect, is when we >> do some >> calculations. I will take a trace and post it here maybe for time consumed >> during the operation. >> >> Regarding moving the implementation to damage tracker, we need to give the >> tile's content-rect and layer-id map to it, and it can traverse over its >> layer >> list to get the transform for the particular layer id, and apply to >> unioned >> rect. >> does that sound good ? >> > > I don't think this should talk to the damage tracker directly. Rather it > could do what reveman suggested, and update the layer's update_rect_ which > doesn't require any transform mapping either, incidentally. > Of course, when I say that, I remember that this update_rect_ is used as the "paint rect" for debugging visualizations. Changing it will break those visualizations. So I think we'd need another rect beside it on the LayerImpl to do this right, and update DamageTracker to union those rects, add appropriate tests, etc. I also wonder if this is worth it, however. Do you see significant cpu savings are a result of this change in some real scenario? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/09 15:47:49, danakj wrote: > On Wed, Apr 9, 2014 at 11:45 AM, Dana Jansens <mailto:danakj@chromium.org> wrote: > > > On Wed, Apr 9, 2014 at 6:11 AM, <mailto:sohan.jyoti@samsung.com> wrote: > > > >> On 2014/04/08 16:01:45, enne wrote: > >> > >>> Do you have an existing use case where this causes problems, or is this > >>> just a > >>> drive-by cleanup? > >>> > >> > >> I agree with reveman that this seems like a lot of complication and > >>> maps, and > >>> we'd do all of this for cases where partial swap isn't enabled. On cases > >>> > >> where > >> > >>> we initialize a ton of tiles, this seems like it could get needlessly > >>> inefficient. > >>> > >> > >> Well, it was originally a clean-up. > >> But, when i saw the initialized tile's rect and what we are setting in > >> LTHI::DidInitializeVisibleTile (i.e set damage_rect as entire viewport), > >> there > >> are times when we have initialized just 1 384X384 tile, but end up > >> setting damge > >> rect as entire viewport, which can be avoided. > >> > >> Regarding the implementation, ::onrastertaskcompleted, the operation is > >> quite > >> light, just add to the map. > >> When DidInitializeVisibleTile, queries for the damaged rect, is when we > >> do some > >> calculations. I will take a trace and post it here maybe for time consumed > >> during the operation. > >> > >> Regarding moving the implementation to damage tracker, we need to give the > >> tile's content-rect and layer-id map to it, and it can traverse over its > >> layer > >> list to get the transform for the particular layer id, and apply to > >> unioned > >> rect. > >> does that sound good ? > >> > > > > I don't think this should talk to the damage tracker directly. Rather it > > could do what reveman suggested, and update the layer's update_rect_ which > > doesn't require any transform mapping either, incidentally. > > > > Of course, when I say that, I remember that this update_rect_ is used as > the "paint rect" for debugging visualizations. Changing it will break those > visualizations. So I think we'd need another rect beside it on the > LayerImpl to do this right, and update DamageTracker to union those rects, > add appropriate tests, etc. > > I also wonder if this is worth it, however. Do you see significant cpu > savings are a result of this change in some real scenario? I'd expect significant power savings with a UI compositor that supports partial swap. We're doing horribly in the "blinking cursor" case right now and when switching to impl-side painting for the UI compositor we'd do full screen redraw every frame for small animations in the UI without this.
https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:75: initialized_tile_rect_(gfx::Rect()) { no need for this. default ctor does the same. https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1021: void LayerImpl::SetInitializedTileRect(gfx::Rect initialized_tile_rect) { pass by const reference instead https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1022: if (!this) heh, nope. https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1025: gfx::UnionRects(initialized_tile_rect_, initialized_tile_rect); don't call this function "SetRect" and then do a union. https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:456: void SetInitializedTileRect(gfx::Rect initialized_tile_rect); I'm not a fan of "InitializedTile" name. This is the general layer class. The usage of tiles is picturelayer specific implementation detail. How about simply "SetDamageRect"? https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... cc/resources/tile_manager.cc:509: } all this tilemap stuff is confusing and inefficient. please notify the client as tiles get initialized instead. https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... cc/resources/tile_manager.h:302: gfx::Rect damage_rect_; unused https://codereview.chromium.org/219963005/diff/120001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/219963005/diff/120001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:312: layer->screen_space_transform(), layer->initialized_tile_rect()); I think we should unite this with update_rect() by not looking at update_rect above and instead required that damage to be added to the damage rect used for tile invalidation. https://codereview.chromium.org/219963005/diff/120001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:314: layer->SetInitializedTileRect(gfx::Rect()); I don't think this is the right place to reset the damage rect? what if this is called twice?
Can you please take a look. Thanks. https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:75: initialized_tile_rect_(gfx::Rect()) { On 2014/04/11 18:11:34, reveman wrote: > no need for this. default ctor does the same. Done. https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1022: if (!this) On 2014/04/11 18:11:34, reveman wrote: > heh, nope. Done. https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1025: gfx::UnionRects(initialized_tile_rect_, initialized_tile_rect); On 2014/04/11 18:11:34, reveman wrote: > don't call this function "SetRect" and then do a union. Done. https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/120001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:456: void SetInitializedTileRect(gfx::Rect initialized_tile_rect); On 2014/04/11 18:11:34, reveman wrote: > I'm not a fan of "InitializedTile" name. This is the general layer class. The > usage of tiles is picturelayer specific implementation detail. How about simply > "SetDamageRect"? Done. https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... cc/resources/tile_manager.cc:509: } On 2014/04/11 18:11:34, reveman wrote: > all this tilemap stuff is confusing and inefficient. please notify the client as > tiles get initialized instead. Done. https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/219963005/diff/120001/cc/resources/tile_manag... cc/resources/tile_manager.h:302: gfx::Rect damage_rect_; On 2014/04/11 18:11:34, reveman wrote: > unused Done. https://codereview.chromium.org/219963005/diff/120001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/219963005/diff/120001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:312: layer->screen_space_transform(), layer->initialized_tile_rect()); On 2014/04/11 18:11:34, reveman wrote: > I think we should unite this with update_rect() by not looking at update_rect > above and instead required that damage to be added to the damage rect used for > tile invalidation. Done. ptal, i am uniting with layers' update_rect before tracking the damage area, so that the existing logic is not affected. https://codereview.chromium.org/219963005/diff/120001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:314: layer->SetInitializedTileRect(gfx::Rect()); On 2014/04/11 18:11:34, reveman wrote: > I don't think this is the right place to reset the damage rect? what if this is > called twice? this will be called only when we calculate renderpass, once for each preparetodraw. Or its better to reset it after frame is drawn (DrawFrame()) ?
https://codereview.chromium.org/219963005/diff/180001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/180001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:456: void UnionDamageRect(gfx::Rect damage_rect); Can you name this function to be in the right space, e.g. UnionDamageLayerRect or UnionDamageContentRect? You are incorrectly unioning content space rects with layer rects.
https://codereview.chromium.org/219963005/diff/180001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/219963005/diff/180001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:298: layer->SetUpdateRect(gfx::UnionRects(rect, layer_rect)); instead of setting the UpdateRect on the layer, just do the union of the damage rect and the update rect here locally. Changing the UpdateRect here will mess with later debug visualizations that collect them. https://codereview.chromium.org/219963005/diff/180001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:299: layer->ResetDamageRect(); Instead of clearing the damage rect here, you can clear it in LayerImpl::ResetAllChangeTrackingForSubtree. https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1210: layer_impl->UnionDamageRect(tile->content_rect()); The tile's content rect is in the tiling's content space, which is not the same thing as the layer's content space. If you scale it by the tile's tiling's content scale, you'll get back to layer space. So something like tile->content_rect() * / tiling_content_scale_{x,y} would give you a rect in the layer's content space. You probably want this rect as floats, like update_rect_ since each pixel there may be many physical pixels.
> So something like tile->content_rect() * / tiling_content_scale_{x,y}
Should say: So something like tile->content_rect() / tiling_content_scale_{x,y}
https://codereview.chromium.org/219963005/diff/180001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/180001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:456: void UnionDamageRect(gfx::Rect damage_rect); Also, maybe more consistent to have this be SetDamageContentRect and leave it up to the caller to do a union with damage_content_rect() if necessary? https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... cc/resources/tile_manager.cc:1235: client_->NotifyInitializedTiles(tile); I think it makes more sense to call this below when all properties have been updated to reflect that the tile has actually been initialized. https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... cc/resources/tile_manager.h:34: virtual void NotifyInitializedTiles(Tile* tile) = 0; NotifyTileInitialized instead? Can it be "const Tile* tile" instead? https://codereview.chromium.org/219963005/diff/180001/cc/test/fake_tile_manag... File cc/test/fake_tile_manager_client.h (right): https://codereview.chromium.org/219963005/diff/180001/cc/test/fake_tile_manag... cc/test/fake_tile_manager_client.h:18: nit: no blank line here to make it clear that comment above refer to both these functions https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1206: LayerImpl* layer_impl = NULL; nit: move to where it's first used instead https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1207: if (active_tree_) { nit: maybe cleaner to early out instead of having nested if statements
PTAL. Thanks. https://codereview.chromium.org/219963005/diff/180001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/180001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:456: void UnionDamageRect(gfx::Rect damage_rect); On 2014/04/14 17:08:29, enne wrote: > Can you name this function to be in the right space, e.g. UnionDamageLayerRect > or UnionDamageContentRect? > > You are incorrectly unioning content space rects with layer rects. Done. https://codereview.chromium.org/219963005/diff/180001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:456: void UnionDamageRect(gfx::Rect damage_rect); On 2014/04/14 20:03:07, reveman wrote: > Also, maybe more consistent to have this be SetDamageContentRect and leave it up > to the caller to do a union with damage_content_rect() if necessary? In that case, the caller(LTHI) needs to maintain the unified damage rect for each layer id, again involving a map. https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... cc/resources/tile_manager.cc:1235: client_->NotifyInitializedTiles(tile); On 2014/04/14 20:03:07, reveman wrote: > I think it makes more sense to call this below when all properties have been > updated to reflect that the tile has actually been initialized. Done. https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/219963005/diff/180001/cc/resources/tile_manag... cc/resources/tile_manager.h:34: virtual void NotifyInitializedTiles(Tile* tile) = 0; On 2014/04/14 20:03:07, reveman wrote: > NotifyTileInitialized instead? Can it be "const Tile* tile" instead? Done. https://codereview.chromium.org/219963005/diff/180001/cc/test/fake_tile_manag... File cc/test/fake_tile_manager_client.h (right): https://codereview.chromium.org/219963005/diff/180001/cc/test/fake_tile_manag... cc/test/fake_tile_manager_client.h:18: On 2014/04/14 20:03:07, reveman wrote: > nit: no blank line here to make it clear that comment above refer to both these > functions Done. https://codereview.chromium.org/219963005/diff/180001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/219963005/diff/180001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:298: layer->SetUpdateRect(gfx::UnionRects(rect, layer_rect)); On 2014/04/14 17:17:07, danakj wrote: > instead of setting the UpdateRect on the layer, just do the union of the damage > rect and the update rect here locally. Changing the UpdateRect here will mess > with later debug visualizations that collect them. Done. https://codereview.chromium.org/219963005/diff/180001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:299: layer->ResetDamageRect(); On 2014/04/14 17:17:07, danakj wrote: > Instead of clearing the damage rect here, you can clear it in > LayerImpl::ResetAllChangeTrackingForSubtree. Done. https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1206: LayerImpl* layer_impl = NULL; On 2014/04/14 20:03:07, reveman wrote: > nit: move to where it's first used instead Done. https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1207: if (active_tree_) { On 2014/04/14 20:03:07, reveman wrote: > nit: maybe cleaner to early out instead of having nested if statements Done. https://codereview.chromium.org/219963005/diff/180001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1210: layer_impl->UnionDamageRect(tile->content_rect()); On 2014/04/14 17:17:07, danakj wrote: > The tile's content rect is in the tiling's content space, which is not the same > thing as the layer's content space. If you scale it by the tile's tiling's > content scale, you'll get back to layer space. > > So something like tile->content_rect() * / tiling_content_scale_{x,y} would give > you a rect in the layer's content space. You probably want this rect as floats, > like update_rect_ since each pixel there may be many physical pixels. > Done.
I think this change will need to include some tests. https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:457: void UnionDamageContentRect(gfx::RectF damage_content_rect); nit: pass by const reference instead https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:457: void UnionDamageContentRect(gfx::RectF damage_content_rect); I would prefer SetContentDamageRect() and have the caller responsible for union of rects. The code in NotifyTileInitialized would look something like this: layer_impl->SetContentDamageRect( gfx::UnionRects(layer_impl->content_damage_rect(), tile_content_rect_in_layer_space)) https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:459: const gfx::RectF& damage_content_rect() const { return damage_content_rect_; } content_damage_rect()? makes it sound more like a getter. https://codereview.chromium.org/219963005/diff/220001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/219963005/diff/220001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:290: // Union layer's update_rect with damage_rect nit: missing period at the end of the line. https://codereview.chromium.org/219963005/diff/220001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:291: gfx::RectF unified_update_and_damage_rect = The comment and variable names below are no longer properly named. I would just call this damage_rect, make it clear in the comment above that this is the union of update_/content_damage_rect and make the comment and variable names below consistent with that. https://codereview.chromium.org/219963005/diff/220001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/219963005/diff/220001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1230: gfx::RectF tile_in_layer_content_rect = is this supposed to be tile_content_rect_in_layer_space?
https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:457: void UnionDamageContentRect(gfx::RectF damage_content_rect); On 2014/04/15 16:25:59, reveman wrote: > I would prefer SetContentDamageRect() and have the caller responsible for union > of rects. The code in NotifyTileInitialized would look something like this: > > layer_impl->SetContentDamageRect( > gfx::UnionRects(layer_impl->content_damage_rect(), > tile_content_rect_in_layer_space)) (Personally I think what you suggest is a violation of demeter, since it makes TileManager responsible for the representation of damage on layers. It exposes that damage is kept as a RectF internally, rather than as a region or something. Not that we want to, but it's just API smell, in my opinion. I preferred the original version.) https://codereview.chromium.org/219963005/diff/220001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/219963005/diff/220001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1230: gfx::RectF tile_in_layer_content_rect = On 2014/04/15 16:25:59, reveman wrote: > is this supposed to be tile_content_rect_in_layer_space? I think this should just be called layer_rect or layer_damage_rect. https://codereview.chromium.org/219963005/diff/220001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1231: gfx::ScaleRect(tile->content_rect(), 1 / tile->contents_scale()); nit: 1 => 1.f https://codereview.chromium.org/219963005/diff/220001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:1232: layer_impl->UnionDamageContentRect(tile_in_layer_content_rect); This function seems misnamed. It should be UnionDamageLayerRect and not content.
https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:457: void UnionDamageContentRect(gfx::RectF damage_content_rect); On 2014/04/15 17:27:27, enne wrote: > On 2014/04/15 16:25:59, reveman wrote: > > I would prefer SetContentDamageRect() and have the caller responsible for > union > > of rects. The code in NotifyTileInitialized would look something like this: > > > > layer_impl->SetContentDamageRect( > > gfx::UnionRects(layer_impl->content_damage_rect(), > > tile_content_rect_in_layer_space)) > > (Personally I think what you suggest is a violation of demeter, since it makes > TileManager responsible for the representation of damage on layers. It exposes > that damage is kept as a RectF internally, rather than as a region or something. > Not that we want to, but it's just API smell, in my opinion. I preferred the > original version.) > Makes sense. I prefer AddContentDamageRect() in that case.
https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:457: void UnionDamageContentRect(gfx::RectF damage_content_rect); On 2014/04/15 17:50:48, reveman wrote: > On 2014/04/15 17:27:27, enne wrote: > > On 2014/04/15 16:25:59, reveman wrote: > > > I would prefer SetContentDamageRect() and have the caller responsible for > > union > > > of rects. The code in NotifyTileInitialized would look something like this: > > > > > > layer_impl->SetContentDamageRect( > > > gfx::UnionRects(layer_impl->content_damage_rect(), > > > tile_content_rect_in_layer_space)) > > > > (Personally I think what you suggest is a violation of demeter, since it makes > > TileManager responsible for the representation of damage on layers. It > exposes > > that damage is kept as a RectF internally, rather than as a region or > something. > > Not that we want to, but it's just API smell, in my opinion. I preferred the > > original version.) > > > > Makes sense. I prefer AddContentDamageRect() in that case. How about AddLayerDamageRect? Sorry to keep quibble, but it's awkward to keep content rects on PictureLayerImpl because it modifies its own content scale during pinch zoom. So, whenever you changed it, you'd have to rescale the content damage rect. It's better to keep damage in layer space to avoid potentially forgetting to do this.
On 2014/04/15 17:55:41, enne wrote: > https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h > File cc/layers/layer_impl.h (right): > > https://codereview.chromium.org/219963005/diff/220001/cc/layers/layer_impl.h#... > cc/layers/layer_impl.h:457: void UnionDamageContentRect(gfx::RectF > damage_content_rect); > On 2014/04/15 17:50:48, reveman wrote: > > On 2014/04/15 17:27:27, enne wrote: > > > On 2014/04/15 16:25:59, reveman wrote: > > > > I would prefer SetContentDamageRect() and have the caller responsible for > > > union > > > > of rects. The code in NotifyTileInitialized would look something like > this: > > > > > > > > layer_impl->SetContentDamageRect( > > > > gfx::UnionRects(layer_impl->content_damage_rect(), > > > > tile_content_rect_in_layer_space)) > > > > > > (Personally I think what you suggest is a violation of demeter, since it > makes > > > TileManager responsible for the representation of damage on layers. It > > exposes > > > that damage is kept as a RectF internally, rather than as a region or > > something. > > > Not that we want to, but it's just API smell, in my opinion. I preferred > the > > > original version.) > > > > > > > Makes sense. I prefer AddContentDamageRect() in that case. > > How about AddLayerDamageRect? > > Sorry to keep quibble, but it's awkward to keep content rects on > PictureLayerImpl because it modifies its own content scale during pinch zoom. > So, whenever you changed it, you'd have to rescale the content damage rect. > It's better to keep damage in layer space to avoid potentially forgetting to do > this. Sounds good. "Add" instead of "Union" and having the name read well was my main concerns.
PTAL. Thanks.
LGTM when the others are happy with it. Would like some tests for it please. :) https://codereview.chromium.org/219963005/diff/280001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/219963005/diff/280001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:290: // Unify layer's update_rect and damage_rect. drop this comment, it just says exactly what the code says now https://codereview.chromium.org/219963005/diff/280001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:305: gfx::Rect update_content_rect = layer->LayerRectToContentRect(damage_rect); rename this to |damage_content_rect| to match the |damage_rect|
https://codereview.chromium.org/219963005/diff/280001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/219963005/diff/280001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1021: void LayerImpl::AddLayerDamageRect(const gfx::RectF& damage_rect) { should this be AddDamageRect or should SetUpdateRect be renamed to SetLayerUpdateRect? They both take a rect in layer space. https://codereview.chromium.org/219963005/diff/280001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/219963005/diff/280001/cc/trees/damage_tracker... cc/trees/damage_tracker.cc:306: gfx::Rect update_rect_in_target_space = MathUtil::MapEnclosingClippedRect( and |damage_rect_in_target_space| here
https://codereview.chromium.org/219963005/diff/280001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/219963005/diff/280001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1021: void LayerImpl::AddLayerDamageRect(const gfx::RectF& damage_rect) { On 2014/04/17 05:18:34, reveman wrote: > should this be AddDamageRect or should SetUpdateRect be renamed to > SetLayerUpdateRect? They both take a rect in layer space. SetUpdateRect should be renamed, so should update_rect_ really. But I think that's outside the scope of this CL?
lgtm after addressing existing comments https://codereview.chromium.org/219963005/diff/280001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/219963005/diff/280001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1021: void LayerImpl::AddLayerDamageRect(const gfx::RectF& damage_rect) { On 2014/04/17 14:14:34, danakj wrote: > On 2014/04/17 05:18:34, reveman wrote: > > should this be AddDamageRect or should SetUpdateRect be renamed to > > SetLayerUpdateRect? They both take a rect in layer space. > > SetUpdateRect should be renamed, so should update_rect_ really. But I think > that's outside the scope of this CL? Ok, I would then name this AddDamageRect right now for consistency and rename everything in a follow up.
Can you please take a look. Thanks.
https://codereview.chromium.org/219963005/diff/320001/cc/trees/damage_tracker... File cc/trees/damage_tracker_unittest.cc (right): https://codereview.chromium.org/219963005/diff/320001/cc/trees/damage_tracker... cc/trees/damage_tracker_unittest.cc:275: // CASE 1: Setting the layer damage rect should cause the corresponding damage nit: you're not setting, you're adding a damage rect https://codereview.chromium.org/219963005/diff/320001/cc/trees/damage_tracker... cc/trees/damage_tracker_unittest.cc:286: root_damage_rect.ToString()); nit: as the api is AddDamageRect I think you should only test that the root damage includes the damage rect and not that it's exactly the same. It being exactly the same is an implementation detail. Same comment for all the tests below. https://codereview.chromium.org/219963005/diff/320001/cc/trees/damage_tracker... cc/trees/damage_tracker_unittest.cc:309: } could you include a test where you add more than one damage rect?
lgtm % reveman's comments
https://chromiumcodereview.appspot.com/219963005/diff/360001/cc/trees/damage_... File cc/trees/damage_tracker_unittest.cc (right): https://chromiumcodereview.appspot.com/219963005/diff/360001/cc/trees/damage_... cc/trees/damage_tracker_unittest.cc:319: EXPECT_EQ(true, root_damage_rect.Contains(gfx::Rect(120, 125, 3, 4))); EXPECT_EQ(true, root_damage_rect.Contains(gfx::Rect(120, 125, 1, 2))) here too?
https://chromiumcodereview.appspot.com/219963005/diff/360001/cc/trees/damage_... File cc/trees/damage_tracker_unittest.cc (right): https://chromiumcodereview.appspot.com/219963005/diff/360001/cc/trees/damage_... cc/trees/damage_tracker_unittest.cc:319: EXPECT_EQ(true, root_damage_rect.Contains(gfx::Rect(120, 125, 3, 4))); On 2014/04/28 16:33:46, reveman wrote: > EXPECT_EQ(true, root_damage_rect.Contains(gfx::Rect(120, 125, 1, 2))) here too? Sorry, I read this wrong. That check doesn't make sense. I think this test would be more useful if the second rect didn't contain the first one. As of now, you can remove the first AddDamageRect call and the result would be the same.
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/219963005/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/219963005/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/219963005/400001
Message was sent while issue was closed.
Change committed as 266852
Message was sent while issue was closed.
Description was changed from ========== cc: Add support for partial swaps when using impl-side painting by only damaging viewport area covered by newly initialized tiles. In the current implementation, during frame draw and visible tile update, LTHI was setting damage to the entire viewport area, irrespective of the initialized tiled content rect. As part of the patch we are trying to ascertain newly initialized tile rect from TileManager, notify LTHI about it, and save it as damage rect in LayerImpl. During frame draw, when the damage for surfaces and layers are tracked, we query for these damaged layer rects and unify with the DamageTracker's damage rect. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266852 ========== to ========== cc: Add support for partial swaps when using impl-side painting by only damaging viewport area covered by newly initialized tiles. In the current implementation, during frame draw and visible tile update, LTHI was setting damage to the entire viewport area, irrespective of the initialized tiled content rect. As part of the patch we are trying to ascertain newly initialized tile rect from TileManager, notify LTHI about it, and save it as damage rect in LayerImpl. During frame draw, when the damage for surfaces and layers are tracked, we query for these damaged layer rects and unify with the DamageTracker's damage rect. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266852 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
