Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780823002:
Uses copy request to flatten the layers to do screen rotation animation. (Closed)
DescriptionUses copy request to flatten the layers to do screen rotation animation.
Currently we clone all the layers to do animation during screen
rotation, which is slow on some of the devices. The solution is using
compositor copy request to flatten the layer hierarchy and make
animations more efficient.
BUG=678763
R=oshima@chromium.org, bruthig@chromium.org
TEST=Unittest with ScreenRotationAnimatorTest and Manual
Review-Url: https://codereview.chromium.org/2780823002
Cr-Commit-Position: refs/heads/master@{#462658}
Committed: https://chromium.googlesource.com/chromium/src/+/2c0ca180a3b0db00edd152ed5b2a703942d84163
Patch Set 1 #
Total comments: 10
Patch Set 2 : Rebase and fix nits. #Patch Set 3 : Remove duplicated entry in flags. #
Total comments: 16
Patch Set 4 : Document corner cases. #Patch Set 5 : Add tests for enable_smooth_screen_rotation code path. #
Total comments: 6
Patch Set 6 : Change the test. #
Total comments: 12
Patch Set 7 : Fix nits in patch 6. #
Total comments: 2
Patch Set 8 : Fix nit in patch 7 and rebase. #Patch Set 9 : Updated the value in histogram.xml. #Patch Set 10 : Resolve merge conflicts. #
Messages
Total messages: 61 (32 generated)
wutao@chromium.org changed reviewers: + bruthig@chromium.org
Hi Ben, Could you please have a look first? Not much new code, most are refactoring current functions. Will upload the histograms.xml file in the next patch. Thanks, Tao
Overall looks good. I've left some suggestions but feel free to add OWNERs now. It might be a good idea to add someone more familiar with the CopyOutputRequest mechanism to make sure that all looks good too. https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:243: // If copy request is not succeeded, fall back to recreate layers solution. nit: 'is not succeeded' - > 'does not succeed' https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:69: void RequestCopyRootLayerAndAnimateRotation( Some docs on these functions would be helpful. Especially mentioning the ones that are asynchronous. https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:87: void AnimateRotation(std::unique_ptr<ScreenRotationRequest> rotation_request); It is a very subtle detail that AnimateRotation() now requires |old_layer_tree_owner_| to be set up properly prior to being called. I see 2 ways to make this better: 1) Document it here. 2) Add a 'std::unique_ptr<ui::LayerTreeOwner> old_layer_tree_owner' parameter to AnimateRotation() and pass it along from the CopyOldLayerTree() and CreateOldLayerTree() functions. [CopyOldLayerTree() and CreateOldLayerTree() could probably be moved to anonymous helper functions with this approach] I have a slight preference for #2 but I'm ok with 1 too. https://codereview.chromium.org/2780823002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2780823002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5749: + <message name="IDS_FLAGS_ASH_ENABLE_SMOOTH_SCREEN_ROTATION_ANIMATION_NAME" desc="Title for the flag to enable a smoother screen rotation animation." translateable="false"> It appears vabr@ moved a lot of Ash command line flags to chrome/browser/flag_descriptions.cc (See https://codereview.chromium.org/2774203002). Might want to double check with them as to where these descriptions should be added. https://codereview.chromium.org/2780823002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5750: + Enable a smoother screen rotation animation. nit: This is rather long for a name. Consider the slightly shorter: 'Enable smooth rotation animations'
lgtm
wutao@chromium.org changed reviewers: + jam@chromium.org, mpearson@chromium.org, oshima@chromium.org
Hi jam@: Please review changes in chrome/browser/about_flags.cc chrome/browser/flag_descriptions.cc chrome/browser/flag_descriptions.h Hi mpearson@: Please review changes in tools/metrics/histograms/histograms.xml Hi oshima@: Please review changes in other files. Thank you, Tao https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:243: // If copy request is not succeeded, fall back to recreate layers solution. On 2017/03/30 18:28:24, bruthig wrote: > nit: 'is not succeeded' - > 'does not succeed' Done. https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:69: void RequestCopyRootLayerAndAnimateRotation( On 2017/03/30 18:28:24, bruthig wrote: > Some docs on these functions would be helpful. Especially mentioning the ones > that are asynchronous. Done. https://codereview.chromium.org/2780823002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:87: void AnimateRotation(std::unique_ptr<ScreenRotationRequest> rotation_request); On 2017/03/30 18:28:24, bruthig wrote: > It is a very subtle detail that AnimateRotation() now requires > |old_layer_tree_owner_| to be set up properly prior to being called. I see 2 > ways to make this better: > > 1) Document it here. > 2) Add a 'std::unique_ptr<ui::LayerTreeOwner> old_layer_tree_owner' parameter to > AnimateRotation() and pass it along from the CopyOldLayerTree() and > CreateOldLayerTree() functions. [CopyOldLayerTree() and CreateOldLayerTree() > could probably be moved to anonymous helper functions with this approach] > > I have a slight preference for #2 but I'm ok with 1 too. I prefer document it here first. In the further edition, when we add the second copy output request, the function signature will change again. And at that time, will consider to move some of the functions to anonymous namespace as well. https://codereview.chromium.org/2780823002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2780823002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5749: + <message name="IDS_FLAGS_ASH_ENABLE_SMOOTH_SCREEN_ROTATION_ANIMATION_NAME" desc="Title for the flag to enable a smoother screen rotation animation." translateable="false"> On 2017/03/30 18:28:24, bruthig wrote: > It appears vabr@ moved a lot of Ash command line flags to > chrome/browser/flag_descriptions.cc (See > https://codereview.chromium.org/2774203002). Might want to double check with > them as to where these descriptions should be added. You are right. https://codereview.chromium.org/2780823002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5750: + Enable a smoother screen rotation animation. On 2017/03/30 18:28:24, bruthig wrote: > nit: This is rather long for a name. Consider the slightly shorter: 'Enable > smooth rotation animations' Done.
wutao@chromium.org changed reviewers: + thestig@chromium.org - jam@chromium.org
Hi Lei, Please review changes in: chrome/browser/about_flags.cc chrome/browser/flag_descriptions.cc chrome/browser/flag_descriptions.h Thanks, Tao
chrome/ lgtm
https://codereview.chromium.org/2780823002/diff/40001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/common/ash_switches... ash/common/ash_switches.cc:66: "ash-enable-smooth-screen-rotation-animation"; you may omit "Animation/-animation" as it's obvious. up to you. https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:235: weak_factory_.GetWeakPtr(), base::Passed(&rotation_request))); do we still need to use ::Passed? Didn't std::move work? https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:243: // If copy request does not succeeded, fall back to recreate layers solution. can you mention when and how it can fail? https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:264: root_window->layer()->size().height()); gfx::Rect rect(..->size()); https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:266: copy_layer->SetBounds(rect); what happens if a) the display bounds changed during animation b) the display gets removed during animation (can happen when switching to docked mode) https://codereview.chromium.org/2780823002/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2780823002/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.h:789: // Title for the flag to enable smooth rotation animations. can you file a bug to cleanup the flag and add reference to it?
histograms.xml lgtm
wutao@chromium.org changed reviewers: + danakj@chromium.org
Hi Dana, Could you please help to answer this question so that I can document it: can you mention when and how it (layer copy request) can fail? https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... Thanks, Tao https://codereview.chromium.org/2780823002/diff/40001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/common/ash_switches... ash/common/ash_switches.cc:66: "ash-enable-smooth-screen-rotation-animation"; On 2017/03/31 00:14:34, oshima wrote: > you may omit "Animation/-animation" as it's obvious. up to you. Removed as suggested. https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:235: weak_factory_.GetWeakPtr(), base::Passed(&rotation_request))); On 2017/03/31 00:14:34, oshima wrote: > do we still need to use ::Passed? Didn't std::move work? Cannot, it prompts: 'unique_ptr' has been explicitly marked deleted here. https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:243: // If copy request does not succeeded, fall back to recreate layers solution. On 2017/03/31 00:14:34, oshima wrote: > can you mention when and how it can fail? One situation is that the user cancel the request. I will ask Dana to add more cases it will fail. https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:264: root_window->layer()->size().height()); On 2017/03/31 00:14:34, oshima wrote: > gfx::Rect rect(..->size()); Done. https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:266: copy_layer->SetBounds(rect); On 2017/03/31 00:14:34, oshima wrote: > what happens if > a) the display bounds changed during animation > b) the display gets removed during animation (can happen when switching to > docked mode) a) The display bounds changed during animation will not affect here, because the animation starts after this command. But it may be reliable to pass a rect to the callback? b) If we remove the display, we should not continue to rotate. We can clean up the |animator| and notify the displayConfigurationController. https://codereview.chromium.org/2780823002/diff/40001/chrome/browser/flag_des... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2780823002/diff/40001/chrome/browser/flag_des... chrome/browser/flag_descriptions.h:789: // Title for the flag to enable smooth rotation animations. On 2017/03/31 00:14:34, oshima wrote: > can you file a bug to cleanup the flag and add reference to it? Created a http://crbug.com/707800 and added it to the main bug.
https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:243: // If copy request does not succeeded, fall back to recreate layers solution. On 2017/04/03 16:16:04, wutao wrote: > On 2017/03/31 00:14:34, oshima wrote: > > can you mention when and how it can fail? > > One situation is that the user cancel the request. I will ask Dana to add more > cases it will fail. It would fail if, for example.. - The layer is removed from the compositor and destroyed before committing the request to the compositor. - The compositor is shutdown. Anything that would destroy the copy request prematurely in this way.
https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:235: weak_factory_.GetWeakPtr(), base::Passed(&rotation_request))); On 2017/03/31 00:14:34, oshima wrote: > do we still need to use ::Passed? Didn't std::move work? Passed() changes behaviour of how the variable is passed when the callback is run. move() would only change how it's passed/stored inside the callback. Passed(std::move(t)) or Passed(&t) are equivalent though and both work.
https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:266: copy_layer->SetBounds(rect); On 2017/04/03 16:16:04, wutao wrote: > On 2017/03/31 00:14:34, oshima wrote: > > what happens if > > a) the display bounds changed during animation > > b) the display gets removed during animation (can happen when switching to > > docked mode) > > a) The display bounds changed during animation will not affect here, because the > animation starts after this command. But it may be reliable to pass a rect to > the callback? > b) If we remove the display, we should not continue to rotate. We can clean up > the |animator| and notify the displayConfigurationController. My concerns are: 1) it shouldn't crash or leave chrome in bad state. 2) it shouldn't look corrupted. I think it's better to simply abort animation and move to the target rotation for both cases.
Hi Oshima, Could you please have a look again. Thank you. Tao https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:266: copy_layer->SetBounds(rect); On 2017/04/03 16:36:11, oshima wrote: > On 2017/04/03 16:16:04, wutao wrote: > > On 2017/03/31 00:14:34, oshima wrote: > > > what happens if > > > a) the display bounds changed during animation > > > b) the display gets removed during animation (can happen when switching to > > > docked mode) > > > > a) The display bounds changed during animation will not affect here, because > the > > animation starts after this command. But it may be reliable to pass a rect to > > the callback? > > b) If we remove the display, we should not continue to rotate. We can clean up > > the |animator| and notify the displayConfigurationController. > > My concerns are: > > 1) it shouldn't crash or leave chrome in bad state. > 2) it shouldn't look corrupted. > > I think it's better to simply abort animation and move to the target rotation > for both cases. I archive your comments and add TODO. We can add code in DisplayConfigurationController to get notifications when a) and b) happens, and abort the animation to the target rotation when any of the bounds changes or display removed.
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hi Oshima, Tests are added to simulate the situation of removing the rotating display in the enable_smooth_screen_rotation code path. Please have a look. Thank you, Tao
https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:216: enable_smooth_rotation_for_test_) { you can just add the switch in the test. https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:70: }; can you move the definition to .cc? https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:75: std::unique_ptr<ScreenRotationRequest> rotation_request); you should be able to just call OnceClosure CreateAfterCopyCallback() then in the test subclass OnceClosure callback = ScreenRotationAnimator::CreateAfterCopyCallback(request); return Bind(&YourCallback, new_callback);
Hi Oshima, Changed the why to call test as you suggested. Thank you, Tao https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:216: enable_smooth_rotation_for_test_) { On 2017/04/04 20:23:13, oshima wrote: > you can just add the switch in the test. Done. https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:70: }; On 2017/04/04 20:23:13, oshima wrote: > can you move the definition to .cc? Tested, but have error: invalid application of 'sizeof' to an incomplete type 'ash::ScreenRotationAnimator::ScreenRotationRequest' https://codereview.chromium.org/2780823002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:75: std::unique_ptr<ScreenRotationRequest> rotation_request); On 2017/04/04 20:23:13, oshima wrote: > you should be able to just call > > > OnceClosure CreateAfterCopyCallback() > > then in the test subclass > > OnceClosure callback = ScreenRotationAnimator::CreateAfterCopyCallback(request); > > return Bind(&YourCallback, new_callback); Done. But I need to specify the callback type, cannot use Closure.
https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:74: virtual base::Callback<void(std::unique_ptr<cc::CopyOutputResult> result)> using CopyCallback = OnceCallback<....> https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:58: class ScreenRotationTestAnimator : public ScreenRotationAnimator { nit: TestScreenRotationAnimator just to make it easier to distinguish from ScreenRotationAnimator https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:123: void SetSecreenRotatioAnimator(int64_t display_id); SetScreen... optional: you can pass the closure to here -> ctor. https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:152: run_loop_->Quit(); QuitWhenIdle https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:324: &ScreenRotationAnimatorTest::DoNothingCallback, base::Unretained(this))); run_loop->QuitWhenIdleClosure() https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:345: AddSecondaryDisplay("640x480,800x600"); just call UpdateDisplay("640x480,800x600"); no need to define extra method.
Hi Oshima, Please look again the new changes. Thank you. Tao https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:74: virtual base::Callback<void(std::unique_ptr<cc::CopyOutputResult> result)> On 2017/04/05 01:55:42, oshima wrote: > using CopyCallback = OnceCallback<....> Changed to: using CopyCallback = base::Callback<>. When using OnceCallback, got error: static_assert failed "OnceCallback::Run() may only be invoked on a non-const rvalue, i.e. std::move(callback).Run()." https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:58: class ScreenRotationTestAnimator : public ScreenRotationAnimator { On 2017/04/05 01:55:43, oshima wrote: > nit: TestScreenRotationAnimator > > just to make it easier to distinguish from ScreenRotationAnimator Done. https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:123: void SetSecreenRotatioAnimator(int64_t display_id); On 2017/04/05 01:55:43, oshima wrote: > SetScreen... > > optional: you can pass the closure to here -> ctor. Done. https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:152: run_loop_->Quit(); On 2017/04/05 01:55:42, oshima wrote: > QuitWhenIdle Done. https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:324: &ScreenRotationAnimatorTest::DoNothingCallback, base::Unretained(this))); On 2017/04/05 01:55:43, oshima wrote: > run_loop->QuitWhenIdleClosure() Done. https://codereview.chromium.org/2780823002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:345: AddSecondaryDisplay("640x480,800x600"); On 2017/04/05 01:55:42, oshima wrote: > just call > > UpdateDisplay("640x480,800x600"); > > no need to define extra method. Done.
The CQ bit was checked by oshima@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...
ash lgtm Please make sure the test fails without display check in OnRootLayerCopiedBeforeRotation. https://codereview.chromium.org/2780823002/diff/120001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/120001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:82: std::unique_ptr<cc::CopyOutputResult> result); nit: this can be private now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
On 2017/04/05 19:02:03, oshima wrote: > ash lgtm > > Please make sure the test fails without display check in > OnRootLayerCopiedBeforeRotation. It fails if remove the display check.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rebased and ready to commit. https://codereview.chromium.org/2780823002/diff/120001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2780823002/diff/120001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:82: std::unique_ptr<cc::CopyOutputResult> result); On 2017/04/05 19:02:03, oshima wrote: > nit: this can be private now? Right. Done.
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 bruthig@chromium.org, thestig@chromium.org, mpearson@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2780823002/#ps160001 (title: "Updated the value in histogram.xml.")
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
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...)
Hi Dana, Please check if I can add '+cc/output' to the DEPS. A work around is to add new APIs in ui::Layer, to send copy request and popular a layer with the mailbox. Thank you, Tao
DEPS LGTM
The CQ bit was checked by wutao@chromium.org
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
Failed to apply patch for ash/rotator/screen_rotation_animator.cc:
While running git apply --index -3 -p1;
error: patch failed: ash/rotator/screen_rotation_animator.cc:230
Falling back to three-way merge...
Applied patch to 'ash/rotator/screen_rotation_animator.cc' with conflicts.
U ash/rotator/screen_rotation_animator.cc
Patch: ash/rotator/screen_rotation_animator.cc
Index: ash/rotator/screen_rotation_animator.cc
diff --git a/ash/rotator/screen_rotation_animator.cc
b/ash/rotator/screen_rotation_animator.cc
index
102b8d0e7b3082f7ff25656d2f1243ecad206e22..6fa4bf235dbc902ac7caf57c4382a72be74234f5
100644
--- a/ash/rotator/screen_rotation_animator.cc
+++ b/ash/rotator/screen_rotation_animator.cc
@@ -8,6 +8,7 @@
#include <utility>
#include <vector>
+#include "ash/common/ash_switches.h"
#include "ash/display/window_tree_host_manager.h"
#include "ash/rotator/screen_rotation_animation.h"
#include "ash/rotator/screen_rotation_animator_observer.h"
@@ -16,6 +17,8 @@
#include "base/memory/ptr_util.h"
#include "base/metrics/histogram_macros.h"
#include "base/time/time.h"
+#include "cc/output/copy_output_request.h"
+#include "cc/output/copy_output_result.h"
#include "ui/aura/window.h"
#include "ui/compositor/layer.h"
#include "ui/compositor/layer_animation_element.h"
@@ -185,14 +188,6 @@ class ScreenRotationAnimationMetricsReporter
} // namespace
-struct ScreenRotationAnimator::ScreenRotationRequest {
- ScreenRotationRequest(display::Display::Rotation to_rotation,
- display::Display::RotationSource from_source)
- : new_rotation(to_rotation), source(from_source) {}
- display::Display::Rotation new_rotation;
- display::Display::RotationSource source;
-};
-
ScreenRotationAnimator::ScreenRotationAnimator(int64_t display_id)
: display_id_(display_id),
is_rotating_(false),
@@ -213,9 +208,86 @@ ScreenRotationAnimator::~ScreenRotationAnimator() {
metrics_reporter_.reset();
}
+void ScreenRotationAnimator::StartRotationAnimation(
+ std::unique_ptr<ScreenRotationRequest> rotation_request) {
+ if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+ switches::kAshEnableSmoothScreenRotation)) {
+ RequestCopyRootLayerAndAnimateRotation(std::move(rotation_request));
+ } else {
+ CreateOldLayerTree();
+ AnimateRotation(std::move(rotation_request));
+ }
+}
+
+void ScreenRotationAnimator::RequestCopyRootLayerAndAnimateRotation(
+ std::unique_ptr<ScreenRotationRequest> rotation_request) {
+ std::unique_ptr<cc::CopyOutputRequest> copy_output_request =
+ cc::CopyOutputRequest::CreateRequest(
+ CreateAfterCopyCallback(std::move(rotation_request)));
+ ui::Layer* layer = GetRootWindow(display_id_)->layer();
+ copy_output_request->set_area(gfx::Rect(layer->size()));
+ layer->RequestCopyOfOutput(std::move(copy_output_request));
+}
+
+ScreenRotationAnimator::CopyCallback
+ScreenRotationAnimator::CreateAfterCopyCallback(
+ std::unique_ptr<ScreenRotationRequest> rotation_request) {
+ return base::Bind(&ScreenRotationAnimator::OnRootLayerCopiedBeforeRotation,
+ weak_factory_.GetWeakPtr(),
+ base::Passed(&rotation_request));
+}
+
+void ScreenRotationAnimator::OnRootLayerCopiedBeforeRotation(
+ std::unique_ptr<ScreenRotationRequest> rotation_request,
+ std::unique_ptr<cc::CopyOutputResult> result) {
+ // If display was removed, should abort rotation.
+ if (!IsDisplayIdValid(display_id_)) {
+ ProcessAnimationQueue();
+ return;
+ }
+
+ // Fall back to recreate layers solution when the copy request has been
+ // canceled or failed. It would fail if, for examples: a) The layer is
removed
+ // from the compositor and destroyed before committing the request to the
+ // compositor. b) The compositor is shutdown.
+ if (result->IsEmpty())
+ CreateOldLayerTree();
+ else
+ CopyOldLayerTree(std::move(result));
+ AnimateRotation(std::move(rotation_request));
+}
+
+void ScreenRotationAnimator::CreateOldLayerTree() {
+ old_layer_tree_owner_ = ::wm::RecreateLayers(GetRootWindow(display_id_));
+}
+
+void ScreenRotationAnimator::CopyOldLayerTree(
+ std::unique_ptr<cc::CopyOutputResult> result) {
+ cc::TextureMailbox texture_mailbox;
+ std::unique_ptr<cc::SingleReleaseCallback> release_callback;
+ result->TakeTexture(&texture_mailbox, &release_callback);
+ DCHECK(texture_mailbox.IsTexture());
+
+ aura::Window* root_window = GetRootWindow(display_id_);
+ gfx::Rect rect(root_window->layer()->size());
+ std::unique_ptr<ui::Layer> copy_layer = base::MakeUnique<ui::Layer>();
+ copy_layer->SetBounds(rect);
+ copy_layer->SetTextureMailbox(texture_mailbox, std::move(release_callback),
+ rect.size());
+ old_layer_tree_owner_ =
+ base::MakeUnique<ui::LayerTreeOwner>(std::move(copy_layer));
+}
+
void ScreenRotationAnimator::AnimateRotation(
std::unique_ptr<ScreenRotationRequest> rotation_request) {
aura::Window* root_window = GetRootWindow(display_id_);
+ std::unique_ptr<LayerCleanupObserver> old_layer_cleanup_observer(
+ new LayerCleanupObserver(weak_factory_.GetWeakPtr()));
+ ui::Layer* old_root_layer = old_layer_tree_owner_->root();
+ old_root_layer->set_name("ScreenRotationAnimator:old_layer_tree");
+ // Add the cloned layer tree in to the root, so it will be rendered.
+ root_window->layer()->Add(old_root_layer);
+ root_window->layer()->StackAtTop(old_root_layer);
const gfx::Rect original_screen_bounds = root_window->GetTargetBounds();
@@ -230,18 +302,6 @@ void ScreenRotationAnimator::AnimateRotation(
const gfx::Tween::Type tween_type = gfx::Tween::FAST_OUT_LINEAR_IN;
- std::unique_ptr<ui::LayerTreeOwner> old_layer_tree =
- ::wm::RecreateLayers(root_window);
- old_layer_tree->root()->set_name("ScreenRotationAnimator:old_layer_tree");
-
- // Add the cloned layer tree in to the root, so it will be rendered.
- root_window->layer()->Add(old_layer_tree->root());
- root_window->layer()->StackAtTop(old_layer_tree->root());
-
- old_layer_tree_owner_ = std::move(old_layer_tree);
- std::unique_ptr<LayerCleanupObserver> old_layer_cleanup_observer(
- new LayerCleanupObserver(weak_factory_.GetWeakPtr()));
-
Shell::GetInstance()->display_manager()->SetDisplayRotation(
display_id_, rotation_request->new_rotation, rotation_request->source);
@@ -249,7 +309,6 @@ void ScreenRotationAnimator::AnimateRotation(
const gfx::Point pivot = gfx::Point(rotated_screen_bounds.width() / 2,
rotated_screen_bounds.height() / 2);
- ui::Layer* old_root_layer = old_layer_tree_owner_->root();
// We must animate each non-cloned child layer individually because the
cloned
// layer was added as a child to |root_window|'s layer so that it will be
// rendered.
@@ -328,7 +387,7 @@ void
ScreenRotationAnimator::Rotate(display::Display::Rotation new_rotation,
StopAnimating();
} else {
is_rotating_ = true;
- AnimateRotation(std::move(rotation_request));
+ StartRotationAnimation(std::move(rotation_request));
}
}
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...
Description was changed from ========== Uses copy request to flattern the layers to do screen rotation animation. Currently we clone all the layers to do animation during screen rotation, which is slow on some of the devices. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient. BUG=678763 ========== to ========== Uses copy request to flatten the layers to do screen rotation animation. Currently we clone all the layers to do animation during screen rotation, which is slow on some of the devices. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient. BUG=678763 R=oshima@chromium.org, bruthig@chromium.org TEST=Unittest with ScreenRotationAnimatorTest and Manual ==========
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 thestig@chromium.org, oshima@chromium.org, bruthig@chromium.org, mpearson@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2780823002/#ps180001 (title: "Resolve merge conflicts.")
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": 180001, "attempt_start_ts": 1491511091554100,
"parent_rev": "2d7e2d37fded9c962ba2f3a708d10bba1237d373", "commit_rev":
"2c0ca180a3b0db00edd152ed5b2a703942d84163"}
Message was sent while issue was closed.
Description was changed from ========== Uses copy request to flatten the layers to do screen rotation animation. Currently we clone all the layers to do animation during screen rotation, which is slow on some of the devices. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient. BUG=678763 R=oshima@chromium.org, bruthig@chromium.org TEST=Unittest with ScreenRotationAnimatorTest and Manual ========== to ========== Uses copy request to flatten the layers to do screen rotation animation. Currently we clone all the layers to do animation during screen rotation, which is slow on some of the devices. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient. BUG=678763 R=oshima@chromium.org, bruthig@chromium.org TEST=Unittest with ScreenRotationAnimatorTest and Manual Review-Url: https://codereview.chromium.org/2780823002 Cr-Commit-Position: refs/heads/master@{#462658} Committed: https://chromium.googlesource.com/chromium/src/+/2c0ca180a3b0db00edd152ed5b2a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/2c0ca180a3b0db00edd152ed5b2a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
