Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905533002:
cc : Store surface layer ids on LayerTreeHost and push them at commit (Closed)
Descriptioncc : Store surface layer ids on LayerTreeHost and push them at commit
This CL stores surface layer ids on LayerTreeHost which is pushed onto
LayerTreeImpl on commit and activation. These ids are used make the
compositor frame metadata instead of LayerTreeImpl::surface_layers and
so this patch also deletes LayerTreeImpl::surface_layers.
This CL is required to stop pushing hidden subtrees at commit.
(See comments in crrev.com/2846653002)
BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2905533002
Cr-Commit-Position: refs/heads/master@{#474907}
Committed: https://chromium.googlesource.com/chromium/src/+/10be211f21100f7e9a3069cdefff580ddd0ef544
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : . #Patch Set 4 : comments #
Total comments: 10
Patch Set 5 : comments #
Total comments: 4
Patch Set 6 : comments #
Total comments: 2
Patch Set 7 : comments #Patch Set 8 : rebase #Patch Set 9 : initialize #
Messages
Total messages: 54 (38 generated)
Description was changed from ========== cc : Store surface layer ids on LayerTreeHost and push them at commit This CL stores surface layer ids on LayerTreeHost which is pushed onto LayerTreeImpl on commit and activation. These ids are used make the compositor frame metadata instead of LayerTreeImpl::surface_layers and so this patch also deletes LayerTreeImpl::surface_layers. This CL is required to stop pushing hidden subtrees at commit. (See comments in crrev.com//2846653002) BUG=595843 ========== to ========== cc : Store surface layer ids on LayerTreeHost and push them at commit This CL stores surface layer ids on LayerTreeHost which is pushed onto LayerTreeImpl on commit and activation. These ids are used make the compositor frame metadata instead of LayerTreeImpl::surface_layers and so this patch also deletes LayerTreeImpl::surface_layers. This CL is required to stop pushing hidden subtrees at commit. (See comments in crrev.com//2846653002) BUG=595843 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc : Store surface layer ids on LayerTreeHost and push them at commit This CL stores surface layer ids on LayerTreeHost which is pushed onto LayerTreeImpl on commit and activation. These ids are used make the compositor frame metadata instead of LayerTreeImpl::surface_layers and so this patch also deletes LayerTreeImpl::surface_layers. This CL is required to stop pushing hidden subtrees at commit. (See comments in crrev.com//2846653002) BUG=595843 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Store surface layer ids on LayerTreeHost and push them at commit This CL stores surface layer ids on LayerTreeHost which is pushed onto LayerTreeImpl on commit and activation. These ids are used make the compositor frame metadata instead of LayerTreeImpl::surface_layers and so this patch also deletes LayerTreeImpl::surface_layers. This CL is required to stop pushing hidden subtrees at commit. (See comments in crrev.com/2846653002) BUG=595843 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
jaydasika@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org, fsamuel@chromium.org
PTAL
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2905533002/diff/1/cc/layers/surface_layer.h File cc/layers/surface_layer.h (right): https://codereview.chromium.org/2905533002/diff/1/cc/layers/surface_layer.h#n... cc/layers/surface_layer.h:56: const SurfaceId* GetSurfaceId(LayerTreeHost* new_host = nullptr); nit: GetReferencedSurfaceId
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2905533002/diff/1/cc/layers/surface_layer.h File cc/layers/surface_layer.h (right): https://codereview.chromium.org/2905533002/diff/1/cc/layers/surface_layer.h#n... cc/layers/surface_layer.h:56: const SurfaceId* GetSurfaceId(LayerTreeHost* new_host = nullptr); On 2017/05/24 01:12:52, Fady Samuel wrote: > nit: GetReferencedSurfaceId Done.
I think the idea here is reasonable, but I'm hoping maybe there's some opportunity for simplifying this a good bit. https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer... cc/layers/surface_layer.cc:118: const SurfaceId* SurfaceLayer::GetReferencedSurfaceId(LayerTreeHost* new_host) { This is a lot of logic in SurfaceLayer to try to avoid sending too many referenced surfaces. Is there anything wrong with just sending all primary and fallback surface ids regardless of surface synchronization settings? Alternatively, could all of this move to the compositor thread? In other words, SurfaceLayerImpl::SetPrimary/FallbackSurfaceInfo could call some new LayerTreeImpl::Add/RemoveSurfaceLayerId instead of having SurfaceLayer do it, and have to handle changing LayerTreeHosts as well? https://codereview.chromium.org/2905533002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:1105: auto pos = std::find(surface_layer_ids_.begin(), surface_layer_ids_.end(), What about flat_set over vector, so you don't have to find before insert/erase? https://codereview.chromium.org/2905533002/diff/60001/cc/trees/tree_synchroni... File cc/trees/tree_synchronizer.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/trees/tree_synchroni... cc/trees/tree_synchronizer.cc:135: if (pending_tree->needs_surface_ids_sync()) { Should this be in LayerTreeImpl::PushPropertiesTo and not TreeSynchronizer? https://codereview.chromium.org/2905533002/diff/60001/cc/trees/tree_synchroni... cc/trees/tree_synchronizer.cc:147: if (host_tree->needs_surface_ids_sync()) { Similarly, should this be in LayerTreeHost::FinishCommitOnImplThread and not TreeSynchronizer?
https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer... cc/layers/surface_layer.cc:118: const SurfaceId* SurfaceLayer::GetReferencedSurfaceId(LayerTreeHost* new_host) { On 2017/05/24 18:44:54, enne wrote: > This is a lot of logic in SurfaceLayer to try to avoid sending too many > referenced surfaces. Is there anything wrong with just sending all primary and > fallback surface ids regardless of surface synchronization settings? > > Alternatively, could all of this move to the compositor thread? In other words, > SurfaceLayerImpl::SetPrimary/FallbackSurfaceInfo could call some new > LayerTreeImpl::Add/RemoveSurfaceLayerId instead of having SurfaceLayer do it, > and have to handle changing LayerTreeHosts as well? Done (sending all primary and fallback surface ids). If we are not going to push hidden layers, we won't have SurfaceLayerImpl and so we can't move this to compositor thread. https://codereview.chromium.org/2905533002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host.cc:1105: auto pos = std::find(surface_layer_ids_.begin(), surface_layer_ids_.end(), On 2017/05/24 18:44:54, enne wrote: > What about flat_set over vector, so you don't have to find before insert/erase? Done. https://codereview.chromium.org/2905533002/diff/60001/cc/trees/tree_synchroni... File cc/trees/tree_synchronizer.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/trees/tree_synchroni... cc/trees/tree_synchronizer.cc:135: if (pending_tree->needs_surface_ids_sync()) { On 2017/05/24 18:44:54, enne wrote: > Should this be in LayerTreeImpl::PushPropertiesTo and not TreeSynchronizer? Done. https://codereview.chromium.org/2905533002/diff/60001/cc/trees/tree_synchroni... cc/trees/tree_synchronizer.cc:147: if (host_tree->needs_surface_ids_sync()) { On 2017/05/24 18:44:54, enne wrote: > Similarly, should this be in LayerTreeHost::FinishCommitOnImplThread and not > TreeSynchronizer? Done.
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2905533002/diff/80001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/80001/cc/layers/surface_layer... cc/layers/surface_layer.cc:81: if (old_surface_id != new_surface_id) { Hmm. This check is comparing pointers? Can you have the same surface id by value but different pointers? That said, it looks like callers of this function already don't call it if the surface id hasn't changed, so I'm not sure it's worth having the conditional. I feel like all this code could be simplified too if you had a AddSurfaceId(SurfaceInfo&) and RemoveSurfaceId(SurfaceInfo&) that had the conditional inside of it, and then you don't need the const SurfaceId* everywhere. You'd just need RemoveSurfaceId(primary_surface_info_) and then AddSurfaceId(primary_surface_info) here and elsewhere. Does that sound reasonable? https://codereview.chromium.org/2905533002/diff/80001/cc/layers/surface_layer... cc/layers/surface_layer.cc:158: layer_tree_host()->RemoveSurfaceLayerId(*fallback_surface_id); add?
https://codereview.chromium.org/2905533002/diff/80001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/80001/cc/layers/surface_layer... cc/layers/surface_layer.cc:81: if (old_surface_id != new_surface_id) { On 2017/05/24 21:57:42, enne wrote: > Hmm. This check is comparing pointers? Can you have the same surface id by > value but different pointers? That said, it looks like callers of this function > already don't call it if the surface id hasn't changed, so I'm not sure it's > worth having the conditional. > > I feel like all this code could be simplified too if you had a > AddSurfaceId(SurfaceInfo&) and RemoveSurfaceId(SurfaceInfo&) that had the > conditional inside of it, and then you don't need the const SurfaceId* > everywhere. You'd just need RemoveSurfaceId(primary_surface_info_) and then > AddSurfaceId(primary_surface_info) here and elsewhere. > > Does that sound reasonable? Done. https://codereview.chromium.org/2905533002/diff/80001/cc/layers/surface_layer... cc/layers/surface_layer.cc:158: layer_tree_host()->RemoveSurfaceLayerId(*fallback_surface_id); On 2017/05/24 21:57:42, enne wrote: > add? Done.
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
https://codereview.chromium.org/2905533002/diff/100001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer.cc:128: RemoveSurfaceId(fallback_surface_info_); This breaks surface synchronization and surface references because |referenced_surfaces| and |activation_dependencies| are expected to be disjointed sets. |activation_dependencies| don't necessarily yet exist.
https://codereview.chromium.org/2905533002/diff/100001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer.cc:128: RemoveSurfaceId(fallback_surface_info_); On 2017/05/24 22:31:14, Fady Samuel wrote: > This breaks surface synchronization and surface references because > |referenced_surfaces| and |activation_dependencies| are expected to be > disjointed sets. |activation_dependencies| don't necessarily yet exist. I did not understand how removing surface ids from the old LayerTreeHost breaks that. Can you elaborate ?
https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer... cc/layers/surface_layer.cc:118: const SurfaceId* SurfaceLayer::GetReferencedSurfaceId(LayerTreeHost* new_host) { On 2017/05/24 21:39:54, jaydasika wrote: > On 2017/05/24 18:44:54, enne wrote: > > This is a lot of logic in SurfaceLayer to try to avoid sending too many > > referenced surfaces. Is there anything wrong with just sending all primary > and > > fallback surface ids regardless of surface synchronization settings? > > > > Alternatively, could all of this move to the compositor thread? In other > words, > > SurfaceLayerImpl::SetPrimary/FallbackSurfaceInfo could call some new > > LayerTreeImpl::Add/RemoveSurfaceLayerId instead of having SurfaceLayer do it, > > and have to handle changing LayerTreeHosts as well? > > Done (sending all primary and fallback surface ids). > If we are not going to push hidden layers, we won't have SurfaceLayerImpl and so > we can't move this to compositor thread. I meant to reply here: > This breaks surface synchronization and surface references because > |referenced_surfaces| and |activation_dependencies| are expected to be > disjointed sets. |activation_dependencies| don't necessarily yet exist.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer.cc File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2905533002/diff/60001/cc/layers/surface_layer... cc/layers/surface_layer.cc:118: const SurfaceId* SurfaceLayer::GetReferencedSurfaceId(LayerTreeHost* new_host) { On 2017/05/24 22:46:24, Fady Samuel wrote: > On 2017/05/24 21:39:54, jaydasika wrote: > > On 2017/05/24 18:44:54, enne wrote: > > > This is a lot of logic in SurfaceLayer to try to avoid sending too many > > > referenced surfaces. Is there anything wrong with just sending all primary > > and > > > fallback surface ids regardless of surface synchronization settings? > > > > > > Alternatively, could all of this move to the compositor thread? In other > > words, > > > SurfaceLayerImpl::SetPrimary/FallbackSurfaceInfo could call some new > > > LayerTreeImpl::Add/RemoveSurfaceLayerId instead of having SurfaceLayer do > it, > > > and have to handle changing LayerTreeHosts as well? > > > > Done (sending all primary and fallback surface ids). > > If we are not going to push hidden layers, we won't have SurfaceLayerImpl and > so > > we can't move this to compositor thread. > > I meant to reply here: > > This breaks surface synchronization and surface references because > > |referenced_surfaces| and |activation_dependencies| are expected to be > > disjointed sets. |activation_dependencies| don't necessarily yet exist. Ok. I have changed the CL again to record only one of the surface ids, primary or fallback based on surface synchronization settings.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Looks good once the bots are happy.
The CQ bit was checked by jaydasika@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jbauman@chromium.org changed reviewers: + jbauman@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jaydasika@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/2905533002/#ps160001 (title: "initialize")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495772991441970,
"parent_rev": "0595f762984952c40eded20338dd1710a57a1dd7", "commit_rev":
"10be211f21100f7e9a3069cdefff580ddd0ef544"}
Message was sent while issue was closed.
Description was changed from ========== cc : Store surface layer ids on LayerTreeHost and push them at commit This CL stores surface layer ids on LayerTreeHost which is pushed onto LayerTreeImpl on commit and activation. These ids are used make the compositor frame metadata instead of LayerTreeImpl::surface_layers and so this patch also deletes LayerTreeImpl::surface_layers. This CL is required to stop pushing hidden subtrees at commit. (See comments in crrev.com/2846653002) BUG=595843 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Store surface layer ids on LayerTreeHost and push them at commit This CL stores surface layer ids on LayerTreeHost which is pushed onto LayerTreeImpl on commit and activation. These ids are used make the compositor frame metadata instead of LayerTreeImpl::surface_layers and so this patch also deletes LayerTreeImpl::surface_layers. This CL is required to stop pushing hidden subtrees at commit. (See comments in crrev.com/2846653002) BUG=595843 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2905533002 Cr-Commit-Position: refs/heads/master@{#474907} Committed: https://chromium.googlesource.com/chromium/src/+/10be211f21100f7e9a3069cdefff... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/10be211f21100f7e9a3069cdefff... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
