Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790583004:
Add second copy request after screen rotation to flatten the layers in animation. (Closed)
DescriptionAdd a copy request after screen rotation to flatten the layers in animation.
Currently we use all the layers after rotation to do animation during screen
rotation. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient.
BUG=678763
TEST=Unittest and Manual
Review-Url: https://codereview.chromium.org/2790583004
Cr-Commit-Position: refs/heads/master@{#465864}
Committed: https://chromium.googlesource.com/chromium/src/+/6d394112c70e0edb2e8f1464bcb9f29dfa54bbb7
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add a copy request after screen rotation to flatten the layers in animation. #
Total comments: 16
Patch Set 3 : Refactor to get old layer transformer. #Patch Set 4 : Move some functions to ash/utility/transformer_util. #
Total comments: 12
Patch Set 5 : Fix for comments in patch 4. #Patch Set 6 : Rebased on all the changes previously. #
Total comments: 6
Patch Set 7 : Fix nits in patch 6. #Patch Set 8 : Rebased and fixed a bug. #
Total comments: 6
Patch Set 9 : Add new tests and add doc to handle new rotation request in |Rotate()|. #
Total comments: 10
Patch Set 10 : Fix nits and add docs. #
Total comments: 2
Patch Set 11 : Add one more state for rotating and fix corner case. #
Total comments: 2
Patch Set 12 : Do not observe screen rotation animator in display_configuration_controller. #
Total comments: 2
Patch Set 13 : Separate the two test sets for slow/smooth animation. #Messages
Total messages: 62 (31 generated)
wutao@chromium.org changed reviewers: + oshima@chromium.org
Hi Oshima, When this cl landed, it will bring us to 15 fps for the animation. Please look at the ash/rotator/screen_rotation_animator.cc. Other files were already submitted for review in other cls. When they are approved, I will rebase the code. Thanks, Tao
danakj@chromium.org changed reviewers: + danakj@chromium.org
Thanks it looks like you're using copy requests correctly, but you could make things less synchronous, left a comment. https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:265: if (result->IsEmpty() || result->size().IsEmpty() || !result->HasTexture()) { I don't think you need to check size() or HasTexture. chromeos only works with GPU compositing, results will always be texture or empty. The size will always be the size of the surface you're requesting a copy of. https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:276: RequestCopyRootLayerByName(kDelegateRootLayerName, callback); Why not do both of these copies at the same time instead of waiting for one to complete before starting the other? https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:283: if (!result->IsEmpty() && !result->size().IsEmpty() && result->HasTexture()) same about size and HasTexture
Description was changed from ========== [WIP] Uses copy request to flattern the layers to do screen rotation animation. This cl is based on the following cls: 1) https://codereview.chromium.org/2780823002/ 2) https://codereview.chromium.org/2780823002/ and the related resize_lock changes. Please focus on the file ash/rotator/screen_rotation_animator.cc. Other changes will be rebased again when cls 1) and 2) are approved. 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 ========== Add a copy request after screen rotation to flatten the layers in animation. Currently we use all the layers after rotation to do animation during screen rotation. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient. BUG=678763 TEST=Unittest and Manual ==========
Hi Oshima, Could you please have a look. This will be the last major CL for the screen rotation animation. Adding the second copy request after screen rotation. Thank you, Tao https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:265: if (result->IsEmpty() || result->size().IsEmpty() || !result->HasTexture()) { On 2017/03/31 19:32:30, danakj wrote: > I don't think you need to check size() or HasTexture. > > chromeos only works with GPU compositing, results will always be texture or > empty. The size will always be the size of the surface you're requesting a copy > of. Acknowledged. https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:276: RequestCopyRootLayerByName(kDelegateRootLayerName, callback); On 2017/03/31 19:32:30, danakj wrote: > Why not do both of these copies at the same time instead of waiting for one to > complete before starting the other? Talked offline, The two requests are on the same layer before/after setting screen rotation. So need to wait the first request finishes and attach it in front of old layers to hide the screen rotation. And the second request need to be taken after the screen rotated and the content are ready. https://codereview.chromium.org/2790583004/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:283: if (!result->IsEmpty() && !result->size().IsEmpty() && result->HasTexture()) On 2017/03/31 19:32:30, danakj wrote: > same about size and HasTexture Acknowledged.
can you also test Internal Secondary + External Primary start rotation unplug external while capturing a copy ? https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:160: layer->SetTransform(transform); Can you refactor and reuse CreateRotationTransform in root_window_transformers.cc instead of copying? https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:202: switches::kAshEnableSmoothScreenRotation)) { you may cache this value. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:232: ui::Layer* layer = GetScreenRotationContainer(display_id_)->layer(); since the container will never change, you should get it once and use it in this instance. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:267: // compositor. b) The compositor is shutdown. do we have to continue under these conditions? Can't we simply abort? https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:291: // animation. ditto https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:68: ScreenRotationRequest(const display::Display::Rotation& from_rotation, it's enum, so no need to pass by reference. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:110: std::unique_ptr<cc::CopyOutputResult> result); new line after this https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:120: // Creates a new layer from |CopyOutputResult|. new layer and its layer tree owner
Hi Oshima, I added a new test as you suggested. For the refactoring of CreateRotationTransform in root_window_transformers.cc, please see my comments below. Would you please suggest a good way to implement it. Thanks, Tao https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:160: layer->SetTransform(transform); On 2017/04/07 20:48:56, oshima wrote: > Can you refactor and reuse CreateRotationTransform in > root_window_transformers.cc instead of copying? It is not exactly the same. It is an inverse transform from new_rotation to old_rotation. I tried to refactor this, to create a transformers for old layer in the screen rotation animation, but due to the include rule "-ash/host", I cannot get the transform from the transformer. So I just return the transform. Please advice a correct way. Thanks. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:202: switches::kAshEnableSmoothScreenRotation)) { On 2017/04/07 20:48:56, oshima wrote: > you may cache this value. Done. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:232: ui::Layer* layer = GetScreenRotationContainer(display_id_)->layer(); On 2017/04/07 20:48:56, oshima wrote: > since the container will never change, you should get it once and use it in this > instance. Done. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:267: // compositor. b) The compositor is shutdown. On 2017/04/07 20:48:56, oshima wrote: > do we have to continue under these conditions? Can't we simply abort? Yes, we can https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:291: // animation. On 2017/04/07 20:48:56, oshima wrote: > ditto Abort here. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:68: ScreenRotationRequest(const display::Display::Rotation& from_rotation, On 2017/04/07 20:48:56, oshima wrote: > it's enum, so no need to pass by reference. Done. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:110: std::unique_ptr<cc::CopyOutputResult> result); On 2017/04/07 20:48:57, oshima wrote: > new line after this Done. https://codereview.chromium.org/2790583004/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:120: // Creates a new layer from |CopyOutputResult|. On 2017/04/07 20:48:57, oshima wrote: > new layer and its layer tree owner Done.
Hi Oshima, Friendly ping. Tao
Hi Oshima, I moved some helper files to ash/utility/transformer_util. I only changed the following two functions, other functions are not touched. CreateRotationTransform and CreateRootWindowRotationTransform Thank you, Tao
https://codereview.chromium.org/2790583004/diff/60001/ash/display/root_window... File ash/display/root_window_transformers.h (right): https://codereview.chromium.org/2790583004/diff/60001/ash/display/root_window... ash/display/root_window_transformers.h:22: class Transform; do you need these changes? https://codereview.chromium.org/2790583004/diff/60001/ash/public/cpp/shell_wi... File ash/public/cpp/shell_window_ids.h (right): https://codereview.chromium.org/2790583004/diff/60001/ash/public/cpp/shell_wi... ash/public/cpp/shell_window_ids.h:110: const int32_t kShellWindowId_ScreenRotationContainer = 25; these are from another CL? https://codereview.chromium.org/2790583004/diff/60001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2790583004/diff/60001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:13: #include "ash/utility/transformer_util.h" is this used? https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... File ash/utility/transformer_util.cc (right): https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... ash/utility/transformer_util.cc:26: #endif you can remove OS_WIN part as ash no longer supports and compiles on windows. please cleanup includes as well. https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... ash/utility/transformer_util.cc:95: } do you have to move these other than CreateRotationTransform ? https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... File ash/utility/transformer_util.h (right): https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... ash/utility/transformer_util.h:35: can you document them?
Hi Oshima, I cleaned it up. Thank you. Tao https://codereview.chromium.org/2790583004/diff/60001/ash/display/root_window... File ash/display/root_window_transformers.h (right): https://codereview.chromium.org/2790583004/diff/60001/ash/display/root_window... ash/display/root_window_transformers.h:22: class Transform; On 2017/04/13 23:51:45, oshima wrote: > do you need these changes? Will remove. https://codereview.chromium.org/2790583004/diff/60001/ash/public/cpp/shell_wi... File ash/public/cpp/shell_window_ids.h (right): https://codereview.chromium.org/2790583004/diff/60001/ash/public/cpp/shell_wi... ash/public/cpp/shell_window_ids.h:110: const int32_t kShellWindowId_ScreenRotationContainer = 25; On 2017/04/13 23:51:45, oshima wrote: > these are from another CL? It is from other cl, which has not committed. So I have not rebased here. https://codereview.chromium.org/2790583004/diff/60001/ash/utility/screenshot_... File ash/utility/screenshot_controller.cc (right): https://codereview.chromium.org/2790583004/diff/60001/ash/utility/screenshot_... ash/utility/screenshot_controller.cc:13: #include "ash/utility/transformer_util.h" On 2017/04/13 23:51:45, oshima wrote: > is this used? Ha, will remove. https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... File ash/utility/transformer_util.cc (right): https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... ash/utility/transformer_util.cc:26: #endif On 2017/04/13 23:51:45, oshima wrote: > you can remove OS_WIN part as ash no longer supports and compiles on windows. > please cleanup includes as well. Done. https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... ash/utility/transformer_util.cc:95: } On 2017/04/13 23:51:45, oshima wrote: > do you have to move these other than CreateRotationTransform ? I do not. I only need CreateRotationTransform. I can put others back. https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... File ash/utility/transformer_util.h (right): https://codereview.chromium.org/2790583004/diff/60001/ash/utility/transformer... ash/utility/transformer_util.h:35: On 2017/04/13 23:51:45, oshima wrote: > can you document them? Done.
Hi Oshima, I rebased on all the changes landed and not landed (in review). Please let me know if you have any questions. Thank you, Tao
still half way. i'll review the rest tomorrow morning https://codereview.chromium.org/2790583004/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2790583004/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:181: return ShellPort::Get()->GetPrimaryRootWindow(); this isn't your change, is it? https://codereview.chromium.org/2790583004/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:165: GetScreenRotationContainer(display_id_)->layer()), use root_window_ to get the rotation container. https://codereview.chromium.org/2790583004/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:392: base::CommandLine::ForCurrentProcess()->HasSwitch( initialize it in ctor
On 2017/04/14 04:54:26, oshima wrote: > still half way. i'll review the rest tomorrow morning Thank you. > > https://codereview.chromium.org/2790583004/diff/100001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_unittest.cc:181: return > ShellPort::Get()->GetPrimaryRootWindow(); > this isn't your change, is it? > The old version is in my another cl (not landed yet): https://codereview.chromium.org/2786563003/ There is a change at ToT. It got renamed recently from WmShell to ShellPort.
On 2017/04/14 05:53:09, wutao wrote: > On 2017/04/14 04:54:26, oshima wrote: > > still half way. i'll review the rest tomorrow morning > Thank you. > > > > > https://codereview.chromium.org/2790583004/diff/100001/ash/devtools/ash_devto... > > ash/devtools/ash_devtools_unittest.cc:181: return > > ShellPort::Get()->GetPrimaryRootWindow(); > > this isn't your change, is it? > > > The old version is in my another cl (not landed yet): > https://codereview.chromium.org/2786563003/ > There is a change at ToT. It got renamed recently from WmShell to ShellPort. Yes, I know it. You can upload only diffs that you made. I can tell you tomorrow.
Hi Oshima, Upload some changes for patch 6. Please review. Thank you, Tao https://codereview.chromium.org/2790583004/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2790583004/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:181: return ShellPort::Get()->GetPrimaryRootWindow(); On 2017/04/14 04:54:26, oshima wrote: > this isn't your change, is it? The old version is in my another cl (not landed yet): https://codereview.chromium.org/2786563003/ There is a change at ToT. It got renamed recently from WmShell to ShellPort. I will change WmShell to ShellPort in the cl 2786563003 when I commit it. https://codereview.chromium.org/2790583004/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:165: GetScreenRotationContainer(display_id_)->layer()), On 2017/04/14 04:54:26, oshima wrote: > use root_window_ to get the rotation container. Done. https://codereview.chromium.org/2790583004/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:392: base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/04/14 04:54:26, oshima wrote: > initialize it in ctor Done. I also need to add a setter for test so that I can enable the ash flag.
Hi Oshima, I merged and uploaded the changes. Fixed a bug in previous patch that ::wm::RecreateLayers() will generate a new screen_rotation_container_layer_, which requires update the cached layer. Thanks, Tao
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_...)
https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:327: screen_rotation_container_layer_->SetOpacity(0.0f); Do you need this? Putting the fully opaque layer on top isn't enough? https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:402: return; is this correct if there is a rotation request? https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:92: std::unique_ptr<ScreenRotationRequest> rotation_request); looks to me that you're not really using both in the test? Either cover both cases, or just define one you really need.
fyi: looks like mash test are failing.
wutao@chromium.org changed reviewers: - danakj@chromium.org
Hi Oshima, I fixed several things in this patch: 1. Add two tests to remove external display in the second copy request callback. 2. Fixed mash unit test. 3. Fixed the handling of new rotation request in |Rotate()|. 4. Added black mask layer, instead of setting opacity to 0. Thanks, Tao https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:327: screen_rotation_container_layer_->SetOpacity(0.0f); On 2017/04/17 15:25:23, oshima wrote: > Do you need this? Putting the fully opaque layer on top isn't enough? When rotating, at the four corners we can see this layer. I can add another black layer on top of this and remove the black layer after animation. https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:402: return; On 2017/04/17 15:25:23, oshima wrote: > is this correct if there is a rotation request? I need to increase the |rotation_request_id_| first and then check current_rotation == new_rotation. Add doc for all the cases. https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/140001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:92: std::unique_ptr<ScreenRotationRequest> rotation_request); On 2017/04/17 15:25:23, oshima wrote: > looks to me that you're not really using both in the test? > Either cover both cases, or just define one you really need. Added two more tests to remove display before the second copy request finishes.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
close. once you fixes the double increase for pending request, i'll approve. https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:299: void ScreenRotationAnimator::CreateOldLayerTree() { can you rename this to CreateOldLayerTreeForSlowAnimation() ? https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:314: gfx::Rect rect(screen_rotation_container_layer_->size()); nit: const https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:452: // skipped. We should call |StartRotationAnimation()|. You don't have to explain everything that the code does. Just explain how request id works to skip slate requests. https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:501: last_pending_request_.reset(); Looking more closely, looks like this increases the id then recreate the request. Can you refactor so that it uses the same rotation request? https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:103: // The code path to start slow animation. nit: can you explain the difference in logic? (not just slow)
Hi Oshima, Please take another look. Thank you, Tao https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:299: void ScreenRotationAnimator::CreateOldLayerTree() { On 2017/04/18 18:32:25, oshima wrote: > can you rename this to CreateOldLayerTreeForSlowAnimation() ? Done. https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:314: gfx::Rect rect(screen_rotation_container_layer_->size()); On 2017/04/18 18:32:25, oshima wrote: > nit: const Done. https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:452: // skipped. We should call |StartRotationAnimation()|. On 2017/04/18 18:32:25, oshima wrote: > You don't have to explain everything that the code does. > > Just explain how request id works to skip slate requests. Done. https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:501: last_pending_request_.reset(); On 2017/04/18 18:32:25, oshima wrote: > Looking more closely, looks like this increases the id then recreate the > request. Can you refactor so that it uses the same rotation request? Thanks to catch the double increasing request id. Since in ProcessAnimationQueue(), the screen_rotation_state_ is set to IDLE, when we call Rotate() at this time, it will call StartRotationAnimation() if the target rotation is different. I will call StartRotationAnimation() in ProcessAnimationQueue() directly after checking the target rotation. https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/160001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:103: // The code path to start slow animation. On 2017/04/18 18:32:25, oshima wrote: > nit: can you explain the difference in logic? (not just slow) 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.
https://codereview.chromium.org/2790583004/diff/180001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/180001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:466: StartRotationAnimation(std::move(last_pending_request_)); This will not stop the currently running animation, right? how about: 1) move "if (current_rotation == new_rotation) {" check to StartRotationAnimation 2) the state is either idle or copy (thus no animation), it's safe to just skip. 3) ProcessAnimationQueue may be called before or after animation starts, so call the observer only if the previous state is ROTATING.
Hi Oshima, ptal. Thanks, Tao https://codereview.chromium.org/2790583004/diff/180001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2790583004/diff/180001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:466: StartRotationAnimation(std::move(last_pending_request_)); On 2017/04/19 00:59:01, oshima wrote: > This will not stop the currently running animation, right? > > how about: > > 1) move "if (current_rotation == new_rotation) {" check to > StartRotationAnimation > > 2) the state is either idle or copy (thus no animation), it's > safe to just skip. > > 3) ProcessAnimationQueue may be called before or after animation starts, so call > the observer only if the previous state is ROTATING. Good suggestion. We only get here after calling StopAnimating(). The question is that, is layer->GetAnimator()->StopAnimating() running at another thread or main thread? If main thread, then when reaching this code, all animations are finished. If running at another thread, then there is a chance the animation not finished at this point. By doing 1), we should avoid this situation at all. Adding another state STOPPING, in case we enter StopAnimating() multiple times if layer->GetAnimator()->StopAnimating() running at another thread. 1), 2) done, ptal. 3) The observer is not the animation observer, it is the animator observer to reset the animator from display_configuration_controller.
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.
let's discuss offline today https://codereview.chromium.org/2790583004/diff/200001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (left): https://codereview.chromium.org/2790583004/diff/200001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:408: screen_rotation_state_ = IDLE; looks like we never set to IDLE anymore?
Hi Oshima, ptal. Thanks, Tao https://codereview.chromium.org/2790583004/diff/200001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (left): https://codereview.chromium.org/2790583004/diff/200001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:408: screen_rotation_state_ = IDLE; On 2017/04/19 15:07:41, oshima wrote: > looks like we never set to IDLE anymore? Right. There are only two code path from here: 1) no more request. Call the animator observer and destroying the animator. 2) has more request, call StartRotationAnimation(). If the current rotation is not target rotation, then state will be set to ROTATINGor COPY_REQUESTED. If the current rotation is target rotation, this function will be called again, then no more request. Will repeat 1). Talked offline. The layer->GetAnimator()->StopAnimating() will run at the UI thread. Then we do not need STOPPING. Again, call the animator observer to destroy the animator is not important since only one animator instance for each display. So we will remove it.
Hi Oshima, ptal. Thanks, Tao
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_...)
On 2017/04/19 17:48:34, wutao wrote: > Hi Oshima, > > ptal. > > Thanks, > Tao A test failed ash_mus_unittest because I renamed a test and need to add it to testing/buildbot/filters/ash_mus_unittests.filter: DisplayConfigurationControllerTest.KeepsAnimatorOnAnimationEnded. I will add this in the next patch. Please advise any other changes need to make. Thanks.
https://codereview.chromium.org/2790583004/diff/220001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/220001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:153: } last one. instead, just append switch in the SetUp in the test. You can have test base class for slow (no switch), and fast (with swtich).
Hi Oshima, Please take another look. Thank you, Tao https://codereview.chromium.org/2790583004/diff/220001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2790583004/diff/220001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:153: } On 2017/04/19 22:56:52, oshima wrote: > last one. instead, just append switch in the SetUp in the test. > You can have test base class for slow (no switch), and fast (with swtich). Separate the two test sets for slow/smooth animation.
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.
lgtm thanks for your patience!
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...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1492654308263580,
"parent_rev": "29ea5c72b53e6155f6be77abcb7c65ed5187e122", "commit_rev":
"6d394112c70e0edb2e8f1464bcb9f29dfa54bbb7"}
Message was sent while issue was closed.
Description was changed from ========== Add a copy request after screen rotation to flatten the layers in animation. Currently we use all the layers after rotation to do animation during screen rotation. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient. BUG=678763 TEST=Unittest and Manual ========== to ========== Add a copy request after screen rotation to flatten the layers in animation. Currently we use all the layers after rotation to do animation during screen rotation. The solution is using compositor copy request to flatten the layer hierarchy and make animations more efficient. BUG=678763 TEST=Unittest and Manual Review-Url: https://codereview.chromium.org/2790583004 Cr-Commit-Position: refs/heads/master@{#465864} Committed: https://chromium.googlesource.com/chromium/src/+/6d394112c70e0edb2e8f1464bcb9... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/6d394112c70e0edb2e8f1464bcb9... |
