New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 854256 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

cc and viz documentation is bad or missing

Project Member Reported by danakj@chromium.org, Jun 19 2018

Issue description

As per https://groups.google.com/a/google.com/d/topic/chrome-gpu/aAvGyw7xVY0/discussion our code complexity is bad, and documentation is considered a key factor in slowing down our productivity. Woops!
 
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc61f5a5812a18b77a3275791aa398b795684c6d

commit fc61f5a5812a18b77a3275791aa398b795684c6d
Author: Fady Samuel <fsamuel@chromium.org>
Date: Mon Jul 09 21:51:04 2018

Add class-level comments to FrameSinkId

Bug: 854256
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I83ff040664e008108e8265a6b21069e270dac981
Reviewed-on: https://chromium-review.googlesource.com/1128209
Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573464}
[modify] https://crrev.com/fc61f5a5812a18b77a3275791aa398b795684c6d/components/viz/common/surfaces/frame_sink_id.h

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ecad9dc531745cecc427ade1304c00a257db6b0e

commit ecad9dc531745cecc427ade1304c00a257db6b0e
Author: danakj <danakj@chromium.org>
Date: Wed Sep 05 15:35:43 2018

Add some comments on LayerTreeHost methods to explain them more.

R=enne@chromium.org

Bug: 854256
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ieeae7911be96a95f961062262d24a454ec10da26
Reviewed-on: https://chromium-review.googlesource.com/1204925
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588873}
[modify] https://crrev.com/ecad9dc531745cecc427ade1304c00a257db6b0e/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/ecad9dc531745cecc427ade1304c00a257db6b0e/cc/trees/layer_tree_host.h

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/956d1c82d6e8263fecbd7d2240774a7363aea2c4

commit 956d1c82d6e8263fecbd7d2240774a7363aea2c4
Author: danakj <danakj@chromium.org>
Date: Thu Sep 13 01:38:57 2018

Comment and clean up code for marking layers to PushPropertiesTo.

The LayersThatShouldPushProperties() returned a non-const reference but
did not need to, changed to const.

The LayerNeedsPushPropertiesForTesting() method is not needed since
tests can just check base::ContainsKey() against the already existing
LayersThatShouldPushProperties().

The PushLayerPropertiesInternal() function was making a copy of the
set of layers, because layers were removing them from the set,
changed to take a const-ref as the layers are cleared afterward.
Clarified variables inside the method for readability.

PictureLayerImpls always PushPropertiesTo. Instead of causing them to
mutate the set to reinsert themselves each frame (and preventing us from
just clear()ing the set after pushing), walk all PictureLayerImpls
separately from the set of layers that need PushPropertiesTo, and
which should be removed from the set after pushing.

Changed the set of layers from a std::unordered_set to a
base::flat_set now that we have a better data type for describing sets
of small numbers of small objects. In this case "large" sets are only
~100 and the objects are pointer-sized.

TODO: We could still improve performance by batching inserts to the set.

R=enne@chromium.org

Bug: 854256
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I9411aa99016b806e585ee9e89ac19c4832f586d2
Reviewed-on: https://chromium-review.googlesource.com/1208356
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590888}
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/layers/layer.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/layers/layer_impl.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/layers/layer_impl.h
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/layers/layer_impl_unittest.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/layers/layer_unittest.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/layers/picture_layer_impl.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/test/push_properties_counting_layer.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/test/push_properties_counting_layer.h
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/trees/layer_tree_host.h
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/trees/layer_tree_host_unittest_scroll.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/956d1c82d6e8263fecbd7d2240774a7363aea2c4/cc/trees/tree_synchronizer.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26099d5d348391dbdb59013e42d3cd437c23b904

commit 26099d5d348391dbdb59013e42d3cd437c23b904
Author: danakj <danakj@chromium.org>
Date: Thu Sep 13 17:08:56 2018

cc: Add comments, remove public APIs, clarify code around LayerTreeHost

R=bokan@chromium.org

Bug: 854256
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3eb9e5047a3ccf9deb364bbddec0346d75edeb4c
Reviewed-on: https://chromium-review.googlesource.com/1207912
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591051}
[modify] https://crrev.com/26099d5d348391dbdb59013e42d3cd437c23b904/cc/layers/layer.cc
[modify] https://crrev.com/26099d5d348391dbdb59013e42d3cd437c23b904/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/26099d5d348391dbdb59013e42d3cd437c23b904/cc/trees/layer_tree_host.h
[modify] https://crrev.com/26099d5d348391dbdb59013e42d3cd437c23b904/cc/trees/layer_tree_impl.h

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c185c1574b1711dc80d914d362e0d7c6db1797f

commit 4c185c1574b1711dc80d914d362e0d7c6db1797f
Author: Weiliang Chen <weiliangc@chromium.org>
Date: Fri Sep 21 19:42:04 2018

cc: Clean up LayerImpl::SetNeedsPushProperties call sites

There is no need to call SetNeedsPushProperties on each individual
setter on LayerImpl, so remove them.

LayerImpl's needs to push property status is only useful when they are
on pending tree and waiting to push its properties during activation.
With PropertyTrees, the change to trigger a LayerImpl's need to push
properties on pending trees come from only the main thread's commit.
Since Layer pushing properties to LayerImpl SetNeedsPushProperties on
LayerImpl, there is no need for each individual setter on LayerImpl to
set that status again.

R=enne

Bug: 854256
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3cfc0e3898deb6ae08a88c1f715d035dea624c9b
Reviewed-on: https://chromium-review.googlesource.com/1234999
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: weiliangc <weiliangc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593305}
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/layers/layer_impl.cc
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/layers/layer_impl.h
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/layers/layer_impl_unittest.cc
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/layers/picture_layer_impl_unittest.cc
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/layers/texture_layer_impl.cc
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/trees/tree_synchronizer.cc
[modify] https://crrev.com/4c185c1574b1711dc80d914d362e0d7c6db1797f/cc/trees/tree_synchronizer_unittest.cc

Sign in to add a comment