Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
377793003:
Consider occluded tiles during eviction with occluded as Tile property. (Closed)
|
Created:
6 years, 5 months ago by jbedley Modified:
6 years, 5 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionConsider occluded tiles during eviction with occluded as Tile property.
This modifies the eviction order to consider occlusion so that the
LayerEvictionTileIterator and TileManager::EvictionTileIterator will
evict occluded tiles before unoccluded tiles in the same priority bin.
The current eviction order is to evict tiles that are:
1. In a lower priority bin
2. Not required for activation
3. At worse resolution
4. Farther away from visible
This patch adds occlusion to the order to evict tiles that are:
1. In a lower priority bin
2. Not required for activation
3. At worse resolution
4. Occluded
5. Farther away from visible
This patch depends on https://codereview.chromium.org/374653003/ which
tracks occlusion per tree and stores the info on the Tile.
BUG=178971
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283842
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address feedback #
Total comments: 12
Patch Set 3 : Address feedback #Patch Set 4 : Add TileManager unit test #
Total comments: 22
Patch Set 5 : Address feedback #Patch Set 6 : Rebase #
Messages
Total messages: 22 (0 generated)
PTAL
https://codereview.chromium.org/377793003/diff/1/cc/layers/picture_layer_impl... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/377793003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl_unittest.cc:3258: pending_layer_, NEW_CONTENT_TAKES_PRIORITY); what about the SAME_PRIORITY_FOR_BOTH_TREES case? if a tile is not shared would it just default to being unoccluded always? should we be testing all 3 priorities? https://codereview.chromium.org/377793003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl_unittest.cc:3297: if (tile->is_occluded(PENDING_TREE)) { Hm, this verifies that we don't evict an occluded tile after a non-occluded one, but it's pretty indirect in verifying that we do evict all occluded tiles first (as opposed to not evicting any of them and then this loop passes, but maybe the 43 check doesn't). Can you change this test a bit to be more direct, like.. My first thought is 1. Verify that at scale T there are N occluded tiles. 2. Verify that for each tile I we evict at scale T, the tile is occluded iff I < N. Then you can hard-code the N if you like but it's more obvious where it's coming from and the test verifies it's what you think it is by doing step 1. wdyt? https://codereview.chromium.org/377793003/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/377793003/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling.cc:48: bool a_is_occluded; initialize these to false
I'm not sure that the tile manager will do the right thing with this patch. It needs to determine, given two or more layers, which layer is next to evict a tile. That means it also has to deal with occlusion. Can you modify TileManagerTileIteratorTest.EvictionTileIterator test or add a new one that verifies it will do the right thing? https://codereview.chromium.org/377793003/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/377793003/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling.cc:51: switch (tree_priority_) { Maybe it's worth having something like is_occluded_for_tree_priority simimlar to priority_for_tree_priority?
The changes made to the OcclusionTrackingPictureLayerImpl unit test caused it to get pretty long. Also, right now the same number of tiles are occluded on the pending and active layers because it seemed cleaner that way. Let me know if you think I should make any changes there. vmpstr, I think you are right about the TileManager. I've made some changes to the EvictionOrderComparator so that the TileManager considers occlusion as well. I am working on writing a test that will exercise this. https://codereview.chromium.org/377793003/diff/1/cc/layers/picture_layer_impl... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/377793003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl_unittest.cc:3258: pending_layer_, NEW_CONTENT_TAKES_PRIORITY); On 2014/07/09 17:38:40, danakj wrote: > what about the SAME_PRIORITY_FOR_BOTH_TREES case? if a tile is not shared would > it just default to being unoccluded always? > > should we be testing all 3 priorities? I added all 6 cases to the test (pending or active layer for 3 priorities). https://codereview.chromium.org/377793003/diff/1/cc/layers/picture_layer_impl... cc/layers/picture_layer_impl_unittest.cc:3297: if (tile->is_occluded(PENDING_TREE)) { On 2014/07/09 17:38:40, danakj wrote: > Hm, this verifies that we don't evict an occluded tile after a non-occluded one, > but it's pretty indirect in verifying that we do evict all occluded tiles first > (as opposed to not evicting any of them and then this loop passes, but maybe the > 43 check doesn't). > > Can you change this test a bit to be more direct, like.. My first thought is > > 1. Verify that at scale T there are N occluded tiles. Done. > 2. Verify that for each tile I we evict at scale T, the tile is occluded iff I < > N. It won't necessarily be that the occluded tiles at a certain scale will be the first N tiles iterated over at that scale. For example, an EVENTUALLY tile would be evicted before an occluded NOW tile. I modified it to verify that occlusion is considered at the proper time (after bin, requirement for activation, and resolution). Another way could be to only count the NOW tiles that aren't required for activation and make sure that the occluded tiles are the first N of those. Do you have a preference? https://codereview.chromium.org/377793003/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/377793003/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling.cc:51: switch (tree_priority_) { On 2014/07/09 23:23:06, vmpstr wrote: > Maybe it's worth having something like is_occluded_for_tree_priority simimlar to > priority_for_tree_priority? Done.
I think the tile manager changes look correct. Thanks. Just a few test comments below https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3359: (tile->priority_for_tree_priority(tree_priority).priority_bin != Can you get the priority first, then check the bin in an effort to clean up this statement a bit? Or get all three parts into a bool first, something like "bin_differs, activation_differs, scale_differs" and then have EXPECT_TRUE(bin_differs || activation_differs || scale_differs); https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3362: (tile->required_for_activation() != Why is activation a part of this condition? That is, what is the configuration where we return an unoccluded tile after an occluded one with the reason that their activation flag differs? (IIUC occluded tiles aren't marked required for activation) https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3364: (tile->contents_scale() != last_tile->contents_scale())); I would pull this check up and just bypass the whole EXPECT_TRUE thing https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3402: tree_priority = SMOOTHNESS_TAKES_PRIORITY; Can you compress the test a bit by making a loop: for (int priority_count = 0; priority_count < 3; ++priority_count) { tree_priority = static_cast<TreePriority>(priority_count); ... } Otherwise, it's hard to tell if the tree priority is the only thing that differs or there's a subtle difference. Likewise, pending/active layer thing can be a helper function that takes a layer and does the loop. (all this assuming that nothing differs... If something is different that precludes this, then you should put a comment explaining the differences between each block) https://codereview.chromium.org/377793003/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/377793003/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:1566: // Otherwise if the distance to visible differs, b is lower priorty if it is nit: you don't need the "if the distance to visible differs" part in the comment
https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3359: (tile->priority_for_tree_priority(tree_priority).priority_bin != On 2014/07/11 00:44:31, vmpstr wrote: > Can you get the priority first, then check the bin in an effort to clean up this > statement a bit? Or get all three parts into a bool first, something like > "bin_differs, activation_differs, scale_differs" and then have > EXPECT_TRUE(bin_differs || activation_differs || scale_differs); How does it look now? https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3362: (tile->required_for_activation() != On 2014/07/11 00:44:31, vmpstr wrote: > Why is activation a part of this condition? That is, what is the configuration > where we return an unoccluded tile after an occluded one with the reason that > their activation flag differs? (IIUC occluded tiles aren't marked required for > activation) Tiles that are occluded on the pending tree cannot be marked as required for activation, but if the tile is only occluded on the active tree then it can be required for activation. So if the TreePriority is SMOOTHNESS_TAKES_PRIORITY then we could see a tile that is occluded (on the active tree) and required for activation, in which case it could be evicted after an unoccluded tile that is not required for activation since requirement for activation takes precedence. I think it is ok for a tile that is only occluded on the active tree to be marked as required for activation. I also think it is right for the TileEvictionOrder to consider requirement for activation regardless of TreePriority to make sure we don't evict shared tiles that are required for activation. If either of these assumptions are wrong, please let me know and I will change the behaviour. https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3364: (tile->contents_scale() != last_tile->contents_scale())); On 2014/07/11 00:44:32, vmpstr wrote: > I would pull this check up and just bypass the whole EXPECT_TRUE thing Sorry, could you explain what you mean by this? https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3402: tree_priority = SMOOTHNESS_TAKES_PRIORITY; On 2014/07/11 00:44:32, vmpstr wrote: > Can you compress the test a bit by making a loop: > > for (int priority_count = 0; priority_count < 3; ++priority_count) { > tree_priority = static_cast<TreePriority>(priority_count); > ... > } > > Otherwise, it's hard to tell if the tree priority is the only thing that differs > or there's a subtle difference. Likewise, pending/active layer thing can be a > helper function that takes a layer and does the loop. > > (all this assuming that nothing differs... If something is different that > precludes this, then you should put a comment explaining the differences between > each block) Done. https://codereview.chromium.org/377793003/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/377793003/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:1566: // Otherwise if the distance to visible differs, b is lower priorty if it is On 2014/07/11 00:44:32, vmpstr wrote: > nit: you don't need the "if the distance to visible differs" part in the comment Done.
This LG, just the tile manager unittests left, I think. https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3362: (tile->required_for_activation() != On 2014/07/11 18:00:30, jbedley wrote: > On 2014/07/11 00:44:31, vmpstr wrote: > > Why is activation a part of this condition? That is, what is the configuration > > where we return an unoccluded tile after an occluded one with the reason that > > their activation flag differs? (IIUC occluded tiles aren't marked required for > > activation) > > Tiles that are occluded on the pending tree cannot be marked as required for > activation, but if the tile is only occluded on the active tree then it can be > required for activation. So if the TreePriority is SMOOTHNESS_TAKES_PRIORITY > then we could see a tile that is occluded (on the active tree) and required for > activation, in which case it could be evicted after an unoccluded tile that is > not required for activation since requirement for activation takes precedence. > > I think it is ok for a tile that is only occluded on the active tree to be > marked as required for activation. I also think it is right for the > TileEvictionOrder to consider requirement for activation regardless of > TreePriority to make sure we don't evict shared tiles that are required for > activation. If either of these assumptions are wrong, please let me know and I > will change the behaviour. Ah, I forgot that we have occlusion for both trees separately. Thanks for the explanation! https://codereview.chromium.org/377793003/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3364: (tile->contents_scale() != last_tile->contents_scale())); On 2014/07/11 18:00:30, jbedley wrote: > On 2014/07/11 00:44:32, vmpstr wrote: > > I would pull this check up and just bypass the whole EXPECT_TRUE thing > > Sorry, could you explain what you mean by this? I just meant something like if (one_scale != other_scale) continue; EXPECT_TRUE(...) or something along those lines, but don't worry about it now. I just wanted EXPECT_TRUE statement to look clean. The current version looks nice! Thanks.
I've added a test to make sure TileManager considers occlusion as well. PTAL.
Mostly LG https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager_unittest.cc:1067: TreePriority tree_priority = NEW_CONTENT_TAKES_PRIORITY; From my understanding, this mode would normally force active tree tiles to be evicted first, but now we get pending tree ones because of occlusion. Is that correct? What I'm asking I guess is without the tile manager changes, this test fails, right?
https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager_unittest.cc:1067: TreePriority tree_priority = NEW_CONTENT_TAKES_PRIORITY; On 2014/07/14 18:42:20, vmpstr wrote: > From my understanding, this mode would normally force active tree tiles to be > evicted first, but now we get pending tree ones because of occlusion. Is that > correct? By using NEW_CONTENT_TAKES_PRIORITY, only the pending tree priorities are considered during eviction. So if there were active-tree-only tiles then they would have the default low pending tree priorities (EVENTUALLY bin with infinite distance from visible) and they would be evicted first. In this test, there are two layers on the pending tree and the child layer's tiles have all been marked as occluded on the pending tree. > What I'm asking I guess is without the tile manager changes, this test fails, > right? Yes, without the tile manager changes this test fails. The EvictionTileIterator alternates between the two layers so it returns alternating occluded and unoccluded tiles. With the tile manager changes, all the occluded tiles are returned first, as they should be.
lgtm; back to danakj for final review. https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager_unittest.cc:1067: TreePriority tree_priority = NEW_CONTENT_TAKES_PRIORITY; On 2014/07/14 19:31:33, jbedley wrote: > On 2014/07/14 18:42:20, vmpstr wrote: > > From my understanding, this mode would normally force active tree tiles to be > > evicted first, but now we get pending tree ones because of occlusion. Is that > > correct? > > By using NEW_CONTENT_TAKES_PRIORITY, only the pending tree priorities are > considered during eviction. So if there were active-tree-only tiles then they > would have the default low pending tree priorities (EVENTUALLY bin with infinite > distance from visible) and they would be evicted first. > > In this test, there are two layers on the pending tree and the child layer's > tiles have all been marked as occluded on the pending tree. > > > What I'm asking I guess is without the tile manager changes, this test fails, > > right? > > Yes, without the tile manager changes this test fails. The EvictionTileIterator > alternates between the two layers so it returns alternating occluded and > unoccluded tiles. With the tile manager changes, all the occluded tiles are > returned first, as they should be. > Awesome, thanks.
https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2827: size_t expected_occluded_tile_count[]) { can this be size_t expected_occluded_tile_count[NUM_TREE_PRIORITIES]? https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2828: for (int priority_count = 0; priority_count < 3; ++priority_count) { can this 3 be NUM_TREE_PRIORITIES (and any other hard-coded 3s) and can you add that to the enum TreePriority? https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3310: size_t expected_occluded_on_both_count[] = {9u, 1u, 1u, 1u, 1u}; Can you add a comment saying why there's 5 values in these arrays https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3313: size_t expected_occluded_sum[] = {13u, 43u, 43u}; And why there's 3 in this one, and what this is a sum of? You could also ASSERT_EQ(arraysize(expected_occluded_sum), NUM_TREE_PRIORITIES) to guard against weird crashes or whatevers. https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3338: EXPECT_EQ(expected_occluded_on_pending_count[i], occluded_on_pending_count); can you add "<< i" to the end of each of these EXPECTs so if they fail we can see what tiling it failed on? https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3366: EXPECT_EQ(expected_occluded_on_pending_count[i], occluded_on_pending_count); << i here also https://codereview.chromium.org/377793003/diff/60001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/picture_lay... cc/resources/picture_layer_tiling.cc:50: nit: drop extra whitespace https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:1562: nit: drop extra whitespace https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager_unittest.cc:1067: TreePriority tree_priority = NEW_CONTENT_TAKES_PRIORITY; On 2014/07/14 19:31:33, jbedley wrote: > On 2014/07/14 18:42:20, vmpstr wrote: > > From my understanding, this mode would normally force active tree tiles to be > > evicted first, but now we get pending tree ones because of occlusion. Is that > > correct? > > By using NEW_CONTENT_TAKES_PRIORITY, only the pending tree priorities are > considered during eviction. So if there were active-tree-only tiles then they > would have the default low pending tree priorities (EVENTUALLY bin with infinite > distance from visible) and they would be evicted first. What happens for unshared tiles in the SAME_PRIORITY_FOR_BOTH_TREES case? It seems like they'll only be occluded if they are considered occluded in both trees, but they are not shared, so can they ever be occluded?
PTAL https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2827: size_t expected_occluded_tile_count[]) { On 2014/07/15 21:26:50, danakj wrote: > can this be size_t expected_occluded_tile_count[NUM_TREE_PRIORITIES]? Done. https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:2828: for (int priority_count = 0; priority_count < 3; ++priority_count) { On 2014/07/15 21:26:50, danakj wrote: > can this 3 be NUM_TREE_PRIORITIES (and any other hard-coded 3s) and can you add > that to the enum TreePriority? Done. https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3310: size_t expected_occluded_on_both_count[] = {9u, 1u, 1u, 1u, 1u}; On 2014/07/15 21:26:50, danakj wrote: > Can you add a comment saying why there's 5 values in these arrays Done. https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3313: size_t expected_occluded_sum[] = {13u, 43u, 43u}; On 2014/07/15 21:26:50, danakj wrote: > And why there's 3 in this one, and what this is a sum of? > > You could also ASSERT_EQ(arraysize(expected_occluded_sum), NUM_TREE_PRIORITIES) > to guard against weird crashes or whatevers. Done. https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3338: EXPECT_EQ(expected_occluded_on_pending_count[i], occluded_on_pending_count); On 2014/07/15 21:26:50, danakj wrote: > can you add "<< i" to the end of each of these EXPECTs so if they fail we can > see what tiling it failed on? Done. https://codereview.chromium.org/377793003/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl_unittest.cc:3366: EXPECT_EQ(expected_occluded_on_pending_count[i], occluded_on_pending_count); On 2014/07/15 21:26:50, danakj wrote: > << i here also Done. https://codereview.chromium.org/377793003/diff/60001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/picture_lay... cc/resources/picture_layer_tiling.cc:50: On 2014/07/15 21:26:50, danakj wrote: > nit: drop extra whitespace Done. https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:1562: On 2014/07/15 21:26:51, danakj wrote: > nit: drop extra whitespace Done. https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager_unittest.cc:1067: TreePriority tree_priority = NEW_CONTENT_TAKES_PRIORITY; On 2014/07/15 21:26:51, danakj wrote: > On 2014/07/14 19:31:33, jbedley wrote: > > On 2014/07/14 18:42:20, vmpstr wrote: > > > From my understanding, this mode would normally force active tree tiles to > be > > > evicted first, but now we get pending tree ones because of occlusion. Is > that > > > correct? > > > > By using NEW_CONTENT_TAKES_PRIORITY, only the pending tree priorities are > > considered during eviction. So if there were active-tree-only tiles then they > > would have the default low pending tree priorities (EVENTUALLY bin with > infinite > > distance from visible) and they would be evicted first. > > What happens for unshared tiles in the SAME_PRIORITY_FOR_BOTH_TREES case? It > seems like they'll only be occluded if they are considered occluded in both > trees, but they are not shared, so can they ever be occluded? That is correct. Unshared tiles will be considered unoccluded on whatever tree they aren't on so they will be unoccluded for SAME_PRIORITY_FOR_BOTH_TREES.
https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager_unittest.cc (right): https://codereview.chromium.org/377793003/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager_unittest.cc:1067: TreePriority tree_priority = NEW_CONTENT_TAKES_PRIORITY; On 2014/07/16 16:09:31, jbedley wrote: > On 2014/07/15 21:26:51, danakj wrote: > > On 2014/07/14 19:31:33, jbedley wrote: > > > On 2014/07/14 18:42:20, vmpstr wrote: > > > > From my understanding, this mode would normally force active tree tiles to > > be > > > > evicted first, but now we get pending tree ones because of occlusion. Is > > that > > > > correct? > > > > > > By using NEW_CONTENT_TAKES_PRIORITY, only the pending tree priorities are > > > considered during eviction. So if there were active-tree-only tiles then > they > > > would have the default low pending tree priorities (EVENTUALLY bin with > > infinite > > > distance from visible) and they would be evicted first. > > > > What happens for unshared tiles in the SAME_PRIORITY_FOR_BOTH_TREES case? It > > seems like they'll only be occluded if they are considered occluded in both > > trees, but they are not shared, so can they ever be occluded? > > That is correct. Unshared tiles will be considered unoccluded on whatever tree > they aren't on so they will be unoccluded for SAME_PRIORITY_FOR_BOTH_TREES. Hm, I think there's 2 problems with this SAME_PRIORITY situation. 1. If the tile is not shared, we can never evict it, which isn't right. Because we && with an unset occlusion which means we're never occluded. 2. If the tile is shared but the tile is not visible on the other tree, we won't have set the occlusion, so we won't be able to evict it based on occlusion in our tree. If you like you could file a bug about this and follow up on this in a separate CL instead of continuing on this CL. My thoughts on this after talking with vlad some are: 1. We could do this by being able to distinguish "unset occlusion" from "not occluded" and "is occluded", making the occluded state an enum instead of a bool. but.. that's not quite enough.. 2. Right now we decide to evict by doing like - should we evict for reason A? and we && the answer from pending and active - should we evict for reason B? and we && the answer from pending and active. But then "should we evict for occlusion" means it has to be unoccluded on both trees. But that's not quite the right answer. Instead we could do - Should we evict from the pending tree's perspective? (it goes off and thinks about reason A B and C). - Should we evict from the active tree's perspective? (it goes off and thinks about reason A B and C). - Then for SAME_PRIO we && the two final answers, and for !SAME_PRIO we just use one of them. At that point maybe the pending tree has it occluded, but the active tree has the tile outside the viewport, with unknown occlusion, but it's far enough away that it wants to evict too. Then we would. WDYT?
LGTM on this as an incremental step if you like, but I think we need to consider how SAME_PRIO works still
On 2014/07/16 22:31:14, danakj wrote: > LGTM on this as an incremental step if you like, but I think we need to consider > how SAME_PRIO works still Ok I will address the SAME_PRIO issue in a separate CL.
The CQ bit was checked by jbedley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbedley@chromium.org/377793003/80001
The CQ bit was unchecked by jbedley@chromium.org
The CQ bit was checked by jbedley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbedley@chromium.org/377793003/100001
Message was sent while issue was closed.
Change committed as 283842 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
