Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2771713004:
Adds UMA for screen rotation animation smoothness. (Closed)
DescriptionAdds UMA for screen rotation animation smoothness.
Adding histgram of screen rotation animation smoothness.
BUG=678763
TEST=manual, created histogram locally.
Review-Url: https://codereview.chromium.org/2771713004
Cr-Commit-Position: refs/heads/master@{#459635}
Committed: https://chromium.googlesource.com/chromium/src/+/3f08f81806674e0e9882a09c6f97852b184628b5
Patch Set 1 #
Total comments: 11
Patch Set 2 : Respond to reviewers. #
Total comments: 8
Patch Set 3 : Adds histograms.xml. #
Total comments: 10
Patch Set 4 : Fixed the description of histogram metric. #
Total comments: 4
Patch Set 5 : Merged and Moves the ScreenRotationAnimationMetricsReporter to .cc. #
Messages
Total messages: 34 (14 generated)
wutao@chromium.org changed reviewers: + bruthig@chromium.org, oshima@chromium.org, varkha@chromium.org
Hi Valery and Oshima, I am adding a UMA metric for screen rotation animation smoothness based on the reporting framework developed by Valery. Could you please have a look of this cl. Thanks, Tao
https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:16: #include "base/lazy_instance.h" Not necessary? https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:190: "Ash.Rotator.ScreenRotationAnimationSmoothness", kPercentage_MIN, Suggestion: how about "Ash.Rotation.AnimationSmoothness" to be somewhat similar to the overview UMA names? https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:193: ->Add(value); nit: should the line above be indented? https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:203: base::MakeUnique<ScreenRotationAnimationMetricsReporter>(); This could be handled in the initializer list. Also does it have the right lifetime, i.e. could the animation sequence outlive the ScreenRotationAnimator?
https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:203: base::MakeUnique<ScreenRotationAnimationMetricsReporter>(); On 2017/03/24 02:50:25, varkha wrote: > This could be handled in the initializer list. > Also does it have the right lifetime, i.e. could the animation sequence outlive > the ScreenRotationAnimator? One maybe possible scenario to consider is when you shut down the OS shell that owns |this| while animation is ongoing. You can try making animation duration long and closing the Chrome OS simulation window.
Hi Valery, Could you please have a look again. Thanks. Tao https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:16: #include "base/lazy_instance.h" On 2017/03/24 02:50:25, varkha wrote: > Not necessary? Removed. https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:190: "Ash.Rotator.ScreenRotationAnimationSmoothness", kPercentage_MIN, On 2017/03/24 02:50:25, varkha wrote: > Suggestion: how about "Ash.Rotation.AnimationSmoothness" to be somewhat similar > to the overview UMA names? Done. https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:193: ->Add(value); On 2017/03/24 02:50:25, varkha wrote: > nit: should the line above be indented? git cl format made this. https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:203: base::MakeUnique<ScreenRotationAnimationMetricsReporter>(); On 2017/03/24 03:01:33, varkha wrote: > On 2017/03/24 02:50:25, varkha wrote: > > This could be handled in the initializer list. > > Also does it have the right lifetime, i.e. could the animation sequence > outlive > > the ScreenRotationAnimator? > > One maybe possible scenario to consider is when you shut down the OS shell that > owns |this| while animation is ongoing. You can try making animation duration > long and closing the Chrome OS simulation window. Tried to increase the animation duration and closed the Chrome OS simulation window. No exception. https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:203: base::MakeUnique<ScreenRotationAnimationMetricsReporter>(); On 2017/03/24 02:50:25, varkha wrote: > This could be handled in the initializer list. > Also does it have the right lifetime, i.e. could the animation sequence outlive > the ScreenRotationAnimator? Moved to initializer list. Good observation. |animator| should outlive the animation sequence of |old_layer_tree|. The animated |old_layer_tree| is a unique_ptr member of |animator|. When destroying |animator|, |old_layer_tree| is destroyed and the animation sequence is aborted. That is possible during destroying |animator|, the |metrics_reporter| is destroyed first and then animation sequence finished. I will reorder the member |metrics_reporter_| before |old_layer_tree_owner_|.
https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:189: "Ash.Rotation.AnimationSmoothness", kPercentage_MIN, kPercentage_MAX, FYI You will need to add an entry to histograms.xml so that the UMA dashboards surface this metric. https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:262: std::unique_ptr<ui::LayerAnimationSequence> animation_sequence = Why don't we add the metric for this sequence? https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:296: if (disable_animation_timers_for_test_) Not related to this change but shouldn't we be disabling the timer for the |animation_sequence| above?
https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:262: std::unique_ptr<ui::LayerAnimationSequence> animation_sequence = On 2017/03/24 14:25:39, bruthig wrote: > Why don't we add the metric for this sequence? I think it would effectively be redundant given that those animations use the same duration.
https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:203: base::MakeUnique<ScreenRotationAnimationMetricsReporter>(); On 2017/03/24 06:12:38, wutao wrote: > On 2017/03/24 02:50:25, varkha wrote: > > This could be handled in the initializer list. > > Also does it have the right lifetime, i.e. could the animation sequence > outlive > > the ScreenRotationAnimator? > > Moved to initializer list. > > Good observation. > |animator| should outlive the animation sequence of |old_layer_tree|. > The animated |old_layer_tree| is a unique_ptr member of |animator|. When > destroying |animator|, |old_layer_tree| is destroyed and the animation sequence > is aborted. > That is possible during destroying |animator|, the |metrics_reporter| is > destroyed first and then animation sequence finished. I will reorder the member > |metrics_reporter_| before |old_layer_tree_owner_|. The lifetime part is subtle. Can you please add a comment about this in the header or tie the lifetimes tighter in some way. Would be also good to have some test coverage for this scenario.
https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:262: std::unique_ptr<ui::LayerAnimationSequence> animation_sequence = On 2017/03/24 15:04:59, varkha wrote: > On 2017/03/24 14:25:39, bruthig wrote: > > Why don't we add the metric for this sequence? > > I think it would effectively be redundant given that those animations use the > same duration. Also, perhaps more importantly, the |child_layer|s here can outlive the Shell > DisplayConfigurationController > ScreenRotationAnimator ownership chain and so you would have a lifetime problem unless |metrics_reporter_| lifetime is beyond all of the |child_layer|s and |screen_rotation| elements. So I don't recommend adding the metric to those |child_layer|s.
On 2017/03/24 15:08:47, varkha wrote: > https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... > File ash/rotator/screen_rotation_animator.cc (right): > > https://codereview.chromium.org/2771713004/diff/1/ash/rotator/screen_rotation... > ash/rotator/screen_rotation_animator.cc:203: > base::MakeUnique<ScreenRotationAnimationMetricsReporter>(); > On 2017/03/24 06:12:38, wutao wrote: > > On 2017/03/24 02:50:25, varkha wrote: > > > This could be handled in the initializer list. > > > Also does it have the right lifetime, i.e. could the animation sequence > > outlive > > > the ScreenRotationAnimator? > > > > Moved to initializer list. > > > > Good observation. > > |animator| should outlive the animation sequence of |old_layer_tree|. > > The animated |old_layer_tree| is a unique_ptr member of |animator|. When > > destroying |animator|, |old_layer_tree| is destroyed and the animation > sequence > > is aborted. > > That is possible during destroying |animator|, the |metrics_reporter| is > > destroyed first and then animation sequence finished. I will reorder the > member > > |metrics_reporter_| before |old_layer_tree_owner_|. > > The lifetime part is subtle. Can you please add a comment about this in the > header or tie the lifetimes tighter in some way. Would be also good to have some > test coverage for this scenario. Discussed offline. Add doc and explicitly reset of |old_layer_tree_owner_| and |metrics_reporter_| in the d'tor.
wutao@chromium.org changed reviewers: + mpearson@chromium.org
Hi Valery and Ben, Please have another look. Hi Mark, Would you please look at the file: tools/metrics/histograms/histograms.xml Hi Oshima, Please review as the owner. Thanks, Tao https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:189: "Ash.Rotation.AnimationSmoothness", kPercentage_MIN, kPercentage_MAX, On 2017/03/24 14:25:39, bruthig wrote: > FYI You will need to add an entry to histograms.xml so that the UMA dashboards > surface this metric. Done. https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:262: std::unique_ptr<ui::LayerAnimationSequence> animation_sequence = On 2017/03/24 16:49:32, varkha wrote: > On 2017/03/24 15:04:59, varkha wrote: > > On 2017/03/24 14:25:39, bruthig wrote: > > > Why don't we add the metric for this sequence? > > > > I think it would effectively be redundant given that those animations use the > > same duration. > > Also, perhaps more importantly, the |child_layer|s here can outlive the Shell > > DisplayConfigurationController > ScreenRotationAnimator ownership chain and so > you would have a lifetime problem unless |metrics_reporter_| lifetime is beyond > all of the |child_layer|s and |screen_rotation| elements. > So I don't recommend adding the metric to those |child_layer|s. Thank you to address this for me! https://codereview.chromium.org/2771713004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:296: if (disable_animation_timers_for_test_) On 2017/03/24 14:25:39, bruthig wrote: > Not related to this change but shouldn't we be disabling the timer for the > |animation_sequence| above? Ideally yes, but it will not affect the test result. Due to that the above block animation will be replaced by the animation of |new_layer_tree|, a screenshot after screen rotation, I would like to add the timer flag and add the animator of the |new_layer_tree| to the testapi at that time.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:188: base::LinearHistogram::FactoryGet( Maybe I haven't thought about this enough, but why can't you use the UMA_HISTOGRAM_PERCENTAGE macro? https://codereview.chromium.org/2771713004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2771713004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1884: + ideally smooth 60 frames per second. Please state when this is recorded. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Also, what does 0% or 50% represent. Is 50%, say, smooth at 30 frames per second, or half the frames are unsmooth, or half the pixels are unsmooth, or ...
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/2771713004/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:53: const int kPercentage_MAX = 101; Don't need those if you use UMA_HISTOGRAM_PERCENTAGE. https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:188: base::LinearHistogram::FactoryGet( On 2017/03/24 20:32:41, Mark P wrote: > Maybe I haven't thought about this enough, but why can't you use the > UMA_HISTOGRAM_PERCENTAGE macro? UMA_HISTOGRAM_PERCENTAGE("Ash.Rotation.AnimationSmoothness", value); should indeed do the same thing. https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:54: // Otherwise it will notify |screen_rotation_animator_observer_|. Change to declarative as in "When screen rotation animation is ended or aborted, calls Rotate() with the pending rotation request if the request queue is not empty. Otherwise notifies |screen_rotation_animator_observer_|."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Mark and Valery, I made changes according to your comments. Please have a look again. Thank you, Tao https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:53: const int kPercentage_MAX = 101; On 2017/03/24 20:38:21, varkha wrote: > Don't need those if you use UMA_HISTOGRAM_PERCENTAGE. Will remove. https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:188: base::LinearHistogram::FactoryGet( On 2017/03/24 20:32:41, Mark P wrote: > Maybe I haven't thought about this enough, but why can't you use the > UMA_HISTOGRAM_PERCENTAGE macro? I was confused by the use case of UMA_HISTOGRAM_PERCENTAGE and FactoryGet. Changed to UMA_HISTOGRAM_PERCENTAGE. https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:188: base::LinearHistogram::FactoryGet( On 2017/03/24 20:38:21, varkha wrote: > On 2017/03/24 20:32:41, Mark P wrote: > > Maybe I haven't thought about this enough, but why can't you use the > > UMA_HISTOGRAM_PERCENTAGE macro? > > UMA_HISTOGRAM_PERCENTAGE("Ash.Rotation.AnimationSmoothness", value); > should indeed do the same thing. Thanks. https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2771713004/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:54: // Otherwise it will notify |screen_rotation_animator_observer_|. On 2017/03/24 20:38:22, varkha wrote: > Change to declarative as in "When screen rotation animation is ended or aborted, > calls Rotate() with the pending rotation request if the request queue is not > empty. Otherwise notifies |screen_rotation_animator_observer_|." Thanks. Done. https://codereview.chromium.org/2771713004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2771713004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1884: + ideally smooth 60 frames per second. On 2017/03/24 20:32:41, Mark P wrote: > Please state when this is recorded. > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > Also, what does 0% or 50% represent. Is 50%, say, smooth at 30 frames per > second, or half the frames are unsmooth, or half the pixels are unsmooth, or ... Added when it is recorded and added more descriptions.
PS4 lgtm. Thanks for adding this!
histograms.xml lgtm
lgtm with nits https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:183: void Report(int value) override { nit: define public ctor/dtor, then DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:76: std::unique_ptr<ScreenRotationAnimationMetricsReporter> metrics_reporter_; nit: you can just use ui::AnimationMetricsReporter, then hide the impl in .cc
lgtm
Merged and ready to commit. https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:183: void Report(int value) override { On 2017/03/24 22:46:00, oshima wrote: > nit: define public ctor/dtor, then DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2771713004/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:76: std::unique_ptr<ScreenRotationAnimationMetricsReporter> metrics_reporter_; On 2017/03/24 22:46:00, oshima wrote: > nit: you can just use ui::AnimationMetricsReporter, then hide the impl in .cc Good suggestion. Done and Thanks.
The CQ bit was checked by wutao@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.
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, varkha@chromium.org, mpearson@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2771713004/#ps80001 (title: "Merged and Moves the ScreenRotationAnimationMetricsReporter to .cc.")
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": 80001, "attempt_start_ts": 1490410826898170,
"parent_rev": "7844edf8040581364b3651ac6e0f4ddbc27efe9a", "commit_rev":
"3f08f81806674e0e9882a09c6f97852b184628b5"}
Message was sent while issue was closed.
Description was changed from ========== Adds UMA for screen rotation animation smoothness. Adding histgram of screen rotation animation smoothness. BUG=678763 TEST=manual, created histogram locally. ========== to ========== Adds UMA for screen rotation animation smoothness. Adding histgram of screen rotation animation smoothness. BUG=678763 TEST=manual, created histogram locally. Review-Url: https://codereview.chromium.org/2771713004 Cr-Commit-Position: refs/heads/master@{#459635} Committed: https://chromium.googlesource.com/chromium/src/+/3f08f81806674e0e9882a09c6f97... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3f08f81806674e0e9882a09c6f97... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
