cc: Only send a BeginMainFrame inside an BeginImplFrame.

cc: Only send a BeginMainFrame inside an BeginImplFrame.

Currently we send a BeginMainFrame straight after surface creation. The problem
with this is that we don't know what time we should be using for rendering. If
we try and use Now() we break the monotonicity of the frame timestamps.

BUG=346230
Committed: https://crrev.com/428ecd106b95281ec67396cc82197c367142d3c9
Cr-Commit-Position: refs/heads/master@{#309011}

[mithro-old, 2014-12-15 15:56:49]

mithro@mithis.com changed reviewers:
+ brianderson@chromium.org

[mithro-old, 2014-12-15 15:56:49]

Hi Brian,

This is the WIP CL to make sure we have BeginFrameArgs before sending a
BeginMainFrame message. I started work on trying to fix all the tests and run
into the problems shown in "cc: Fixing scheduler state machine tests around
BeginMainFrameAborted" - http://crrev.com/805843002

This CL is in turn blocking: "cc: Adding BeginFrameTracker object and removing
Now() from LTHI." - http://crrev.com/787763006

Tim 'mithro' Ansell

[brianderson, 2014-12-16 23:52:29]

https://codereview.chromium.org/798323003/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/798323003/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:391: if (output_surface_state_ !=
OUTPUT_SURFACE_ACTIVE)
Why does this change need to be part of this patch?

This would defer the first animate until after the first activate. Is that too
long? Don't we need to animate while we have the pending tree?

[mithro-old, 2014-12-17 03:17:30]

https://codereview.chromium.org/798323003/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/798323003/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:391: if (output_surface_state_ !=
OUTPUT_SURFACE_ACTIVE)
On 2014/12/16 23:52:28, brianderson wrote:
> Why does this change need to be part of this patch?
> 
> This would defer the first animate until after the first activate. Is that too
> long? Don't we need to animate while we have the pending tree?

We use to only ANIMATE in the DRAW phase right? I think that means that neither
Commit or Activate depends on the ANIMATE call.

The OUTPUT_SURFACE_ACTIVE state occurs before we can draw. I've added the check
to ShouldDraw() now too.

[mithro-old, 2014-12-17 07:28:48]

mithro@mithis.com changed reviewers:
+ skyostil@google.com, sunnyps@chromium.org

[mithro-old, 2014-12-17 07:29:44]

Hi everyone,

Slowly making process on the tests. All the scheduler_state_machine tests now
pass.

Can you start reviewing and I'll keep making tests pass. Will hopefully have
everything passing before I go home tonight.

Tim 'mithro' Ansell

[mithro-old, 2014-12-17 14:06:48]

Down to 4 tests which seem to fail;

[  FAILED  ] 4 tests, listed below:
[  FAILED  ]
LayerTreeHostTestSendBeginFramesToChildrenWithExternalBFS.RunSingleThread_DirectRenderer_MainThreadPaint
[  FAILED  ]
LayerTreeHostTestSendBeginFramesToChildrenWithExternalBFS.RunSingleThread_DirectRenderer_ImplSidePaint
[  FAILED  ]
LayerTreeHostTestSendBeginFramesToChildrenWithExternalBFS.RunSingleThread_DelegatingRenderer_MainThreadPaint
[  FAILED  ]
LayerTreeHostTestSendBeginFramesToChildrenWithExternalBFS.RunSingleThread_DelegatingRenderer_ImplSidePaint

Tim 'mithro' Ansell

[mithro-old, 2014-12-17 15:20:38]

All tests pass now! \o/ \o/ \o/

This patch is blocked on the following patches which you should review first.
 * 809723003: cc: Removing 127 lines of duplicate code in scheduler_unittest.cc
 * 810953003: cc: Small cleanup for AdvanceFrame() in FakeSchedulerClient.

Please make sure you review the right patch set because to test on the try bots
there are a couple of uploads which include the dependent patches.

Tim 'mithro' Ansell

[brianderson, 2014-12-18 01:21:51]

https://codereview.chromium.org/798323003/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/798323003/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:391: if (output_surface_state_ !=
OUTPUT_SURFACE_ACTIVE)
On 2014/12/17 03:17:30, mithro wrote:
> On 2014/12/16 23:52:28, brianderson wrote:
> > Why does this change need to be part of this patch?
> > 
> > This would defer the first animate until after the first activate. Is that
too
> > long? Don't we need to animate while we have the pending tree?
> 
> We use to only ANIMATE in the DRAW phase right? I think that means that
neither
> Commit or Activate depends on the ANIMATE call.
> 
> The OUTPUT_SURFACE_ACTIVE state occurs before we can draw. I've added the
check
> to ShouldDraw() now too.

I thought we wanted to split animate into 2 or 3 different phases. Only one of
which occurs during draw.

Sunny, do you think this is okay?

[brianderson, 2014-12-18 01:33:30]

lgtm if you are sure we don't need to animate after a new output surface until
we have our first active tree.

https://codereview.chromium.org/798323003/diff/140001/cc/test/fake_external_b...
File cc/test/fake_external_begin_frame_source.cc (right):

https://codereview.chromium.org/798323003/diff/140001/cc/test/fake_external_b...
cc/test/fake_external_begin_frame_source.cc:43: PostTestOnBeginFrame();
Why is posting the next BeginFrame needed now when it wasn't before this patch?

[mithro-old, 2014-12-18 13:45:02]

https://codereview.chromium.org/798323003/diff/140001/cc/test/fake_external_b...
File cc/test/fake_external_begin_frame_source.cc (right):

https://codereview.chromium.org/798323003/diff/140001/cc/test/fake_external_b...
cc/test/fake_external_begin_frame_source.cc:43: PostTestOnBeginFrame();
On 2014/12/18 01:33:30, brianderson wrote:
> Why is posting the next BeginFrame needed now when it wasn't before this
patch?

Because the LTHI tests now often need two BeginFrameArgs to successfully
complete (as they set state when the first BeginMainFrame is sent and previously
that took effect before the first BeginImplFrame occurred). This is just a side
effect of the way the set sets things up.

[mithro-old, 2014-12-18 13:45:17]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-12-18 13:45:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/798323003/160001

[commit-bot: I haz the power, 2014-12-18 13:53:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-18 13:53:43]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mithro-old, 2014-12-18 14:09:01]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-12-18 14:10:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/798323003/180001

[commit-bot: I haz the power, 2014-12-18 15:13:50]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-12-18 15:14:41]

Patchset 10 (id:??) landed as
https://crrev.com/428ecd106b95281ec67396cc82197c367142d3c9
Cr-Commit-Position: refs/heads/master@{#309011}