Removing the animate step from DoSwapInternal as it happens in the ScheduleAnimate method now.

Removing the animate step from DoSwapInternal as it happens in the ScheduleAnimate method now.

R=skyostil,brianderson
BUG=346230

[mithro-old, 2014-06-18 05:44:22]

Hi,

Any idea why DoSwapInternal calls animate? Should the animate method already
have been called? Is there some interaction with the SingleThreadProxy or others
that I'm missing?

cc_unittests seems to pass, waiting for results from other tests.

Tim 'mithro' Ansell

[ajuma, 2014-06-18 14:08:18]

On 2014/06/18 05:44:22, mithro wrote:
> Hi,
> 
> Any idea why DoSwapInternal calls animate? Should the animate method already
> have been called? Is there some interaction with the SingleThreadProxy or
others
> that I'm missing?
> 
> cc_unittests seems to pass, waiting for results from other tests.
> 
> Tim 'mithro' Ansell

If there's a commit after the start of a frame (which is where we animate), we
need to tick animations again so that newly added animations get ticked. See
https://codereview.chromium.org/206793003/#msg25 for more context.

[Sami, 2014-06-18 16:32:23]

On 2014/06/18 14:08:18, ajuma wrote:
> On 2014/06/18 05:44:22, mithro wrote:
> > Hi,
> > 
> > Any idea why DoSwapInternal calls animate? Should the animate method already
> > have been called? Is there some interaction with the SingleThreadProxy or
> others
> > that I'm missing?
> > 
> > cc_unittests seems to pass, waiting for results from other tests.
> > 
> > Tim 'mithro' Ansell
> 
> If there's a commit after the start of a frame (which is where we animate), we
> need to tick animations again so that newly added animations get ticked. See
> https://codereview.chromium.org/206793003/#msg25 for more context.

Right. LayerTreeHostAnimationTestAddAnimationAfterAnimating should fail if you
remove that call -- does it not?

[mithro-old, 2014-06-18 22:24:12]

On 2014/06/18 16:32:23, Sami wrote:
> On 2014/06/18 14:08:18, ajuma wrote:
> > On 2014/06/18 05:44:22, mithro wrote:
> > > Hi,
> > > 
> > > Any idea why DoSwapInternal calls animate? Should the animate method
already
> > > have been called? Is there some interaction with the SingleThreadProxy or
> > others
> > > that I'm missing?
> > > 
> > > cc_unittests seems to pass, waiting for results from other tests.
> > > 
> > > Tim 'mithro' Ansell
> > 
> > If there's a commit after the start of a frame (which is where we animate),
we
> > need to tick animations again so that newly added animations get ticked. See
> > https://codereview.chromium.org/206793003/#msg25 for more context.
> 
> Right. LayerTreeHostAnimationTestAddAnimationAfterAnimating should fail if you
> remove that call -- does it not?

That test does not fail when I remove these lines, see the output below.

===============================================================================================================================================
tansell@tansell-z620-l2:/fast/chrome/src$ git diff HEAD~1
diff --git a/cc/trees/thread_proxy.cc b/cc/trees/thread_proxy.cc
index 1eed641..904ae60 100644
--- a/cc/trees/thread_proxy.cc
+++ b/cc/trees/thread_proxy.cc
@@ -1059,11 +1059,6 @@ DrawResult ThreadProxy::DrawSwapInternal(bool
forced_draw) {
   base::TimeDelta draw_duration_estimate = DrawDurationEstimate();
   base::AutoReset<bool> mark_inside(&impl().inside_draw, true);
 
-  if (impl().did_commit_after_animating) {
-    impl().layer_tree_host_impl->Animate(impl().animation_time);
-    impl().did_commit_after_animating = false;
-  }
-
   if (impl().layer_tree_host_impl->pending_tree())
     impl().layer_tree_host_impl->pending_tree()->UpdateDrawProperties();
 
tansell@tansell-z620-l2:/fast/chrome/src$ export TARGET=cc_unittests; ninja -C
out-fast/Debug $TARGET 2> /dev/null; ninja -C out-fast/Debug $
TARGET -k 1 -j 1                                                                
                                                            
ninja: Entering directory `out-fast/Debug'
ninja: no work to do.
ninja: Entering directory `out-fast/Debug'
ninja: no work to do.
tansell@tansell-z620-l2:/fast/chrome/src$ out-fast/Debug/cc_unittests
--single-process-tests --gtest_filter=*LayerTreeHostAnimationTestAddAni
mationAfterAnimating*                                                           
                                                            
Note: Google Test filter =
*LayerTreeHostAnimationTestAddAnimationAfterAnimating*
[==========] Running 6 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 6 tests from LayerTreeHostAnimationTestAddAnimationAfterAnimating
[ RUN      ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DirectRenderer
[       OK ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DirectRenderer
(106 ms)
[ RUN      ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_MainThreadPaint
[       OK ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_MainThreadPaint
(17 ms)
[ RUN      ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_ImplSidePaint
[       OK ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_ImplSidePaint
(20 ms)
[ RUN      ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DelegatingRenderer
[       OK ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DelegatingRenderer
(103 ms)
[ RUN      ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_MainThreadPaint
[       OK ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_MainThreadPaint
(16 ms)
[ RUN      ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_ImplSidePaint
[       OK ]
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_ImplSidePaint
(20 ms)
[----------] 6 tests from LayerTreeHostAnimationTestAddAnimationAfterAnimating
(283 ms total)

[----------] Global test environment tear-down
[==========] 6 tests from 1 test case ran. (285 ms total)
[  PASSED  ] 6 tests.

  YOU HAVE 6 DISABLED TESTS

tansell@tansell-z620-l2:/fast/chrome/src$ 
===============================================================================================================================================

Should it be failing?

[simonhong, 2014-06-19 00:51:02]

On 2014/06/18 22:24:12, mithro wrote:
> On 2014/06/18 16:32:23, Sami wrote:
> > On 2014/06/18 14:08:18, ajuma wrote:
> > > On 2014/06/18 05:44:22, mithro wrote:
> > > > Hi,
> > > > 
> > > > Any idea why DoSwapInternal calls animate? Should the animate method
> already
> > > > have been called? Is there some interaction with the SingleThreadProxy
or
> > > others
> > > > that I'm missing?
> > > > 
> > > > cc_unittests seems to pass, waiting for results from other tests.
> > > > 
> > > > Tim 'mithro' Ansell
> > > 
> > > If there's a commit after the start of a frame (which is where we
animate),
> we
> > > need to tick animations again so that newly added animations get ticked.
See
> > > https://codereview.chromium.org/206793003/#msg25 for more context.
> > 
> > Right. LayerTreeHostAnimationTestAddAnimationAfterAnimating should fail if
you
> > remove that call -- does it not?
> 
> That test does not fail when I remove these lines, see the output below.
> 
>
===============================================================================================================================================
> tansell@tansell-z620-l2:/fast/chrome/src$ git diff HEAD~1
> diff --git a/cc/trees/thread_proxy.cc b/cc/trees/thread_proxy.cc
> index 1eed641..904ae60 100644
> --- a/cc/trees/thread_proxy.cc
> +++ b/cc/trees/thread_proxy.cc
> @@ -1059,11 +1059,6 @@ DrawResult ThreadProxy::DrawSwapInternal(bool
> forced_draw) {
>    base::TimeDelta draw_duration_estimate = DrawDurationEstimate();
>    base::AutoReset<bool> mark_inside(&impl().inside_draw, true);
>  
> -  if (impl().did_commit_after_animating) {
> -    impl().layer_tree_host_impl->Animate(impl().animation_time);
> -    impl().did_commit_after_animating = false;
> -  }
> -
>    if (impl().layer_tree_host_impl->pending_tree())
>      impl().layer_tree_host_impl->pending_tree()->UpdateDrawProperties();
>  
> tansell@tansell-z620-l2:/fast/chrome/src$ export TARGET=cc_unittests; ninja -C
> out-fast/Debug $TARGET 2> /dev/null; ninja -C out-fast/Debug $
> TARGET -k 1 -j 1                                                              
 
>                                                             
> ninja: Entering directory `out-fast/Debug'
> ninja: no work to do.
> ninja: Entering directory `out-fast/Debug'
> ninja: no work to do.
> tansell@tansell-z620-l2:/fast/chrome/src$ out-fast/Debug/cc_unittests
> --single-process-tests --gtest_filter=*LayerTreeHostAnimationTestAddAni
> mationAfterAnimating*                                                         
 
>                                                             
> Note: Google Test filter =
> *LayerTreeHostAnimationTestAddAnimationAfterAnimating*
> [==========] Running 6 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 6 tests from LayerTreeHostAnimationTestAddAnimationAfterAnimating
> [ RUN      ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DirectRenderer
> [       OK ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DirectRenderer
> (106 ms)
> [ RUN      ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_MainThreadPaint
> [       OK ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_MainThreadPaint
> (17 ms)
> [ RUN      ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_ImplSidePaint
> [       OK ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DirectRenderer_ImplSidePaint
> (20 ms)
> [ RUN      ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DelegatingRenderer
> [       OK ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunSingleThread_DelegatingRenderer
> (103 ms)
> [ RUN      ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_MainThreadPaint
> [       OK ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_MainThreadPaint
> (16 ms)
> [ RUN      ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_ImplSidePaint
> [       OK ]
>
LayerTreeHostAnimationTestAddAnimationAfterAnimating.RunMultiThread_DelegatingRenderer_ImplSidePaint
> (20 ms)
> [----------] 6 tests from LayerTreeHostAnimationTestAddAnimationAfterAnimating
> (283 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 6 tests from 1 test case ran. (285 ms total)
> [  PASSED  ] 6 tests.
> 
>   YOU HAVE 6 DISABLED TESTS
> 
> tansell@tansell-z620-l2:/fast/chrome/src$ 
>
===============================================================================================================================================
> 
> Should it be failing?

I also think the code removed in this cl is not used anymore.
Animation is performed only once in the frame cycle.
When animation is set by SetNeedsAnimation() before starting the commit,
animation should be
triggered in the stage of BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING.
When animation is set in the middle of commit, animation also be triggered right
before DrawSwapInternal() is executed.
So, I think there is no chance to call LTHI->Animate() in the
DrawSwapInternal().

[mithro-old, 2014-06-19 04:52:46]

On 2014/06/19 00:51:02, simonhong wrote:
> I also think the code removed in this cl is not used anymore.
> Animation is performed only once in the frame cycle.
> When animation is set by SetNeedsAnimation() before starting the commit,
> animation should be
> triggered in the stage of BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING.
> When animation is set in the middle of commit, animation also be triggered
right
> before DrawSwapInternal() is executed.
> So, I think there is no chance to call LTHI->Animate() in the
> DrawSwapInternal().

After discussing with ajuma, brianderson and skyostil here is a summary.

In the *COMMIT* we get new animations from the main thread. We want to start
these animations before the DRAW otherwise there will be a frame delay between
the main thread creating the animations and the animation starting.

This wasn't a problem before skyostils change to add an explicit ANIMATE state
because all animation happened as part of the DRAW stage.

Long term myself, brianderson and skyostil agreed we should make the Scheduler
understand that there are cases where it needs to do an ANIMATE between COMMIT
and DRAW.

We also discovered that the LayerTreeHostAnimationTestAddAnimationAfterAnimating
test is broken and not actually testing the COMMIT->DRAW without ANIMATE steps.

To current plan is;
 1) Fix LayerTreeHostAnimationTestAddAnimationAfterAnimating (in a different CL)
to actually test the case we want it to.
 2) Confirm this CL is broken and then change it to call ScheduleActionAnimate
rather than Animate directly.
 3) Create a new CL which makes the Scheduler understand the need to call
ScheduleActionAnimate between COMMIT and DRAW (and remove the
ScheduleActionAnimate call added in 1.)

Tim

[simonhong, 2014-06-20 00:14:32]

On 2014/06/19 04:52:46, mithro wrote:
> On 2014/06/19 00:51:02, simonhong wrote:
> > I also think the code removed in this cl is not used anymore.
> > Animation is performed only once in the frame cycle.
> > When animation is set by SetNeedsAnimation() before starting the commit,
> > animation should be
> > triggered in the stage of BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING.
> > When animation is set in the middle of commit, animation also be triggered
> right
> > before DrawSwapInternal() is executed.
> > So, I think there is no chance to call LTHI->Animate() in the
> > DrawSwapInternal().
> 
> After discussing with ajuma, brianderson and skyostil here is a summary.
> 
> In the *COMMIT* we get new animations from the main thread. We want to start
> these animations before the DRAW otherwise there will be a frame delay between
> the main thread creating the animations and the animation starting.
> 
> This wasn't a problem before skyostils change to add an explicit ANIMATE state
> because all animation happened as part of the DRAW stage.
> 
> Long term myself, brianderson and skyostil agreed we should make the Scheduler
> understand that there are cases where it needs to do an ANIMATE between COMMIT
> and DRAW.
> 
> We also discovered that the
LayerTreeHostAnimationTestAddAnimationAfterAnimating
> test is broken and not actually testing the COMMIT->DRAW without ANIMATE
steps.
> 
> To current plan is;
>  1) Fix LayerTreeHostAnimationTestAddAnimationAfterAnimating (in a different
CL)
> to actually test the case we want it to.
>  2) Confirm this CL is broken and then change it to call ScheduleActionAnimate
> rather than Animate directly.
>  3) Create a new CL which makes the Scheduler understand the need to call
> ScheduleActionAnimate between COMMIT and DRAW (and remove the
> ScheduleActionAnimate call added in 1.)
> 
> Tim

Thanks for sharing this good summary!