Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
819643004:
Cache viewport rect for tile priority in UpdateTiles (Closed)
|
Created:
5 years, 11 months ago by hush (inactive) Modified:
5 years, 11 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache viewport rect for tile priority in UpdateTiles
PictureLayerImpls in a tree should continue using the same viewport rect
for tile priority until UpdateTiles() is called with the new value of
viewport rect for tile priority. Otherwise, a PictureLayerImpl could be
in an inconsistent state that could lead to a wrong decision about
AllTilesRequiredForActivationAreReadyToDraw.
This CL caches the viewport rect for tile priority in PictureLayerImpl.
BUG=449008
Committed: https://crrev.com/9178c77a0167ee7c73364b21a6f3a633029b7a44
Cr-Commit-Position: refs/heads/master@{#312523}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : test #
Total comments: 2
Patch Set 4 : review #Patch Set 5 : #
Total comments: 5
Patch Set 6 : cache in pictureLayerImpl #Patch Set 7 : cache the return value of GetViewportForTilePriorityInContentSpace #
Total comments: 6
Patch Set 8 : review #
Total comments: 6
Patch Set 9 : review #Patch Set 10 : rebase #
Messages
Total messages: 30 (7 generated)
hush@chromium.org changed reviewers: + boliu@chromium.org
Hi Bo! Could you take a look before I sent it to cc owners? Thanks!
minor comment https://codereview.chromium.org/819643004/diff/40001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/819643004/diff/40001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:566: layer_tree_host_impl_->ViewportRectForTilePriority(); I guess this is ok now, but probably safer to move this above the CalculateDrawProperties? There is no need to check resourceless_software_draw, this is valid even inside a software draw.
https://codereview.chromium.org/819643004/diff/40001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/819643004/diff/40001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:566: layer_tree_host_impl_->ViewportRectForTilePriority(); On 2015/01/15 01:36:05, boliu wrote: > I guess this is ok now, but probably safer to move this above the > CalculateDrawProperties? There is no need to check resourceless_software_draw, > this is valid even inside a software draw. Yes. Done.
hush@chromium.org changed reviewers: + danakj@chromium.org
Add Dana for cc/ review
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
(some drive-by comments) The function to get this rect is only being used in one spot in PictureLayerImpl::GetViewportForTilePriorityInContentSpace. Does it make sense to cache that one instead? On top of ensuring it doesn't change before UpdateTiles, you also would be able to save some computations. Or can other settings like screen_space_transform() change? https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... File cc/trees/layer_tree_impl_unittest.cc (right): https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2099: gfx::Transform transform, transform_for_tile_priority; One variable per statement, please. https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2103: gfx::Rect viewport = gfx::Rect(0, 0, 200, 200), Same here. https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl_unittest.cc:2104: rect1 = gfx::Rect(0, 0, 100, 100), If you name these something like "viewport_rect_for_tile_priority" and "different_viewport_rect_for_tile_priority", then you don't need the comments below to describe what rect1 and rect2 are :)
On 2015/01/16 19:14:01, vmpstr wrote: > (some drive-by comments) > > The function to get this rect is only being used in one spot in > PictureLayerImpl::GetViewportForTilePriorityInContentSpace. Does it make sense > to cache that one instead? On top of ensuring it doesn't change before > UpdateTiles, you also would be able to save some computations. Or can other > settings like screen_space_transform() change? I think that should work as well. It would live in this if-block: https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_... But I think when this was first reviewed with Dana, we preferred to have less code in that if-block since it's totally unclear to others what that if block does. This way saves a rect per layer as well I guess :p > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > File cc/trees/layer_tree_impl_unittest.cc (right): > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > cc/trees/layer_tree_impl_unittest.cc:2099: gfx::Transform transform, > transform_for_tile_priority; > One variable per statement, please. > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > cc/trees/layer_tree_impl_unittest.cc:2103: gfx::Rect viewport = gfx::Rect(0, 0, > 200, 200), > Same here. > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > cc/trees/layer_tree_impl_unittest.cc:2104: rect1 = gfx::Rect(0, 0, 100, 100), > If you name these something like "viewport_rect_for_tile_priority" and > "different_viewport_rect_for_tile_priority", then you don't need the comments > below to describe what rect1 and rect2 are :)
On 2015/01/16 19:28:27, boliu wrote: > On 2015/01/16 19:14:01, vmpstr wrote: > > (some drive-by comments) > > > > The function to get this rect is only being used in one spot in > > PictureLayerImpl::GetViewportForTilePriorityInContentSpace. Does it make sense > > to cache that one instead? On top of ensuring it doesn't change before > > UpdateTiles, you also would be able to save some computations. Or can other > > settings like screen_space_transform() change? > > I think that should work as well. It would live in this if-block: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_... > > But I think when this was first reviewed with Dana, we preferred to have less > code in that if-block since it's totally unclear to others what that if block > does. > > This way saves a rect per layer as well I guess :p We can always have something like UpdateViewportForTilePriority() outside of the if (right after)... The opinion I have here isn't a strong one, so feel free to keep the caching the way it is :) > > > > > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > > File cc/trees/layer_tree_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > > cc/trees/layer_tree_impl_unittest.cc:2099: gfx::Transform transform, > > transform_for_tile_priority; > > One variable per statement, please. > > > > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > > cc/trees/layer_tree_impl_unittest.cc:2103: gfx::Rect viewport = gfx::Rect(0, > 0, > > 200, 200), > > Same here. > > > > > https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... > > cc/trees/layer_tree_impl_unittest.cc:2104: rect1 = gfx::Rect(0, 0, 100, 100), > > If you name these something like "viewport_rect_for_tile_priority" and > > "different_viewport_rect_for_tile_priority", then you don't need the comments > > below to describe what rect1 and rect2 are :)
On 2015/01/16 19:28:27, boliu wrote: > On 2015/01/16 19:14:01, vmpstr wrote: > > (some drive-by comments) > > > > The function to get this rect is only being used in one spot in > > PictureLayerImpl::GetViewportForTilePriorityInContentSpace. Does it make sense > > to cache that one instead? On top of ensuring it doesn't change before > > UpdateTiles, you also would be able to save some computations. Or can other > > settings like screen_space_transform() change? > > I think that should work as well. It would live in this if-block: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_... Why would it be in an if-block as opposed to always cacheing it there for all codepaths?
https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:515: layer_tree_host_impl_->ViewportRectForTilePriority(); I find it awkward that LTI mirrors the LTHI method name, but it's a cached value instead of just calling through to the LTHI like other methods in LTI do.
On 2015/01/20 22:42:35, danakj wrote: > On 2015/01/16 19:28:27, boliu wrote: > > On 2015/01/16 19:14:01, vmpstr wrote: > > > (some drive-by comments) > > > > > > The function to get this rect is only being used in one spot in > > > PictureLayerImpl::GetViewportForTilePriorityInContentSpace. Does it make > sense > > > to cache that one instead? On top of ensuring it doesn't change before > > > UpdateTiles, you also would be able to save some computations. Or can other > > > settings like screen_space_transform() change? > > > > I think that should work as well. It would live in this if-block: > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_... > > Why would it be in an if-block as opposed to always cacheing it there for all > codepaths? My bad. As Vlad pointed out, always caching GetViewportForTilePriorityInContentSpace immediately after that if-block is fine.
On 2015/01/20 23:11:40, boliu wrote: > On 2015/01/20 22:42:35, danakj wrote: > > On 2015/01/16 19:28:27, boliu wrote: > > > On 2015/01/16 19:14:01, vmpstr wrote: > > > > (some drive-by comments) > > > > > > > > The function to get this rect is only being used in one spot in > > > > PictureLayerImpl::GetViewportForTilePriorityInContentSpace. Does it make > > sense > > > > to cache that one instead? On top of ensuring it doesn't change before > > > > UpdateTiles, you also would be able to save some computations. Or can > other > > > > settings like screen_space_transform() change? > > > > > > I think that should work as well. It would live in this if-block: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_... > > > > Why would it be in an if-block as opposed to always cacheing it there for all > > codepaths? > > My bad. As Vlad pointed out, always caching > GetViewportForTilePriorityInContentSpace immediately after that if-block is > fine. Doing it in the layer there seems like it will be easier to follow probably. So I'd vote for trying out how that looks.
Patchset #6 (id:100001) has been deleted
PTAL PS6 https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/819643004/diff/80001/cc/trees/layer_tree_impl... cc/trees/layer_tree_impl.cc:515: layer_tree_host_impl_->ViewportRectForTilePriority(); On 2015/01/20 22:42:46, danakj wrote: > I find it awkward that LTI mirrors the LTHI method name, but it's a cached value > instead of just calling through to the LTHI like other methods in LTI do. > good point... it is inconsistent.. I will cache it in PictureLayerImpl when updateTiles is called.
https://codereview.chromium.org/819643004/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/819643004/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:286: viewport_rect_for_tile_priority_in_content_space_, max_contents_scale); Is this right? We'll be drawing with the new DeviceViewport() and DeviceClip() but the old ViewportRectForTilePriority? https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... File cc/test/fake_picture_layer_impl.cc (right): https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... cc/test/fake_picture_layer_impl.cc:204: gfx::Rect rect = GetViewportForTilePriorityInContentSpaceForTesting(); use the viewport_rect_for_tile_priority_in_content_space_ the same way PLI does (and the same way it uses visible_rect_for_tile_priority_ above) https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... File cc/test/fake_picture_layer_impl.h (right): https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... cc/test/fake_picture_layer_impl.h:80: using PictureLayerImpl::GetViewportForTilePriorityInContentSpaceForTesting; instead of exposing a ForTesting method, you can just add an accessor method to this class. like invalidation()
https://codereview.chromium.org/819643004/diff/140001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/819643004/diff/140001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:286: viewport_rect_for_tile_priority_in_content_space_, max_contents_scale); On 2015/01/21 18:47:40, danakj wrote: > Is this right? We'll be drawing with the new DeviceViewport() and DeviceClip() > but the old ViewportRectForTilePriority? If this is not Android WebView, DeviceViewport and DeviceClip will be updated in commit, and commit will also call set_needs_udpate_draw_properties, so all 3 should be in sync at "draw" stage. https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... If this is Android WebView, DeviceViewport, DeviceClip and ViewportRectForTilePriority could be changed by LTHI::SetExternalDrawProperties, and the active tree will update its draw properties when this happens. https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... So I think we always have the guarantee that DeviceViewport, DeviceClip and ViewportRectForTilePriority are in sync at draw time. https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... File cc/test/fake_picture_layer_impl.cc (right): https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... cc/test/fake_picture_layer_impl.cc:204: gfx::Rect rect = GetViewportForTilePriorityInContentSpaceForTesting(); On 2015/01/21 18:47:40, danakj wrote: > use the viewport_rect_for_tile_priority_in_content_space_ the same way PLI does > (and the same way it uses visible_rect_for_tile_priority_ above) Done. https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... File cc/test/fake_picture_layer_impl.h (right): https://codereview.chromium.org/819643004/diff/140001/cc/test/fake_picture_la... cc/test/fake_picture_layer_impl.h:80: using PictureLayerImpl::GetViewportForTilePriorityInContentSpaceForTesting; On 2015/01/21 18:47:40, danakj wrote: > instead of exposing a ForTesting method, you can just add an accessor method to > this class. like invalidation() Done.
https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:504: void PictureLayerImpl::CacheViewportForTilePriorityInContentSpace() { s/CacheViewportFor/UpdateViewportRectFor/ https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:510: don't add whitespace https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:122: gfx::Rect viewport_rect_for_tile_priority_in_content_space() const { I meant you can have this accessor on the FakePictureLayerImpl. You don't need to make it public here.
https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:504: void PictureLayerImpl::CacheViewportForTilePriorityInContentSpace() { On 2015/01/21 22:32:18, danakj wrote: > s/CacheViewportFor/UpdateViewportRectFor/ Done. https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:510: On 2015/01/21 22:32:18, danakj wrote: > don't add whitespace Done. https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/819643004/diff/160001/cc/layers/picture_layer... cc/layers/picture_layer_impl.h:122: gfx::Rect viewport_rect_for_tile_priority_in_content_space() const { On 2015/01/21 22:32:18, danakj wrote: > I meant you can have this accessor on the FakePictureLayerImpl. You don't need > to make it public here. Oh. I see. Done.
LGTM
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/819643004/180001
This might need a rebase on https://codereview.chromium.org/853393002/
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) win8_chromium_gn_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/819643004/200001
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9178c77a0167ee7c73364b21a6f3a633029b7a44 Cr-Commit-Position: refs/heads/master@{#312523} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
