cc: Make scheduling be driven by vsync for android webview.

cc: Make scheduling be driven by vsync for android webview.

This CL makes android webview use the similar mechanisms for
scheduling as other platforms instead of using special
polling code in the scheduler.

Design Doc: https://docs.google.com/a/chromium.org/document/d/1w5UiuA2uZcAiU9-1Y23bxXaStUThkAK8wHI4ULqho2Y/edit#

BUG=439275
Committed: https://crrev.com/bf27da634790bb6eecf4b89f278cfd55c5e5d1f3
Cr-Commit-Position: refs/heads/master@{#323361}

Committed: https://crrev.com/eab5ac980c7b7a5781dc18a1217911eb362698f0
Cr-Commit-Position: refs/heads/master@{#323553}

[sunnyps, 2014-12-19 01:35:39]

sunnyps@chromium.org changed reviewers:
+ boliu@chromium.org, brianderson@chromium.org, danakj@chromium.org,
enne@chromium.org

[brianderson, 2014-12-20 00:34:14]

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:347: // drawing a frame, however it's possible that
the frame rate controller
frame rate controller doesn't exist anymore. Since you are touching this
comment, might as well update it so it refers to the BeginFrameSource instead.

Note: I notice the advance_commit_state_task_ isn't actually needed anywhere at
ToT, because the SytheticBeginFrame source won't get blocked like the frame rate
controller used to. However, once Windows switches to the Browser as the
BeginFrame source, we could have the same deadlock issues - so we sadly can't
get rid of all polling. So close.

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:543: ProcessScheduledActions();
Nice.

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:640: // Separate scope for mark_inside.
Do you only need this to allow re-entrancy when ScheduleBeginImplFrameDeadline
calls the deadline synchronously?

Would it be easier to read if this block is split out into a private method?

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:679: // to advance out of it's waiting to draw state
without actually
it's -> its

https://codereview.chromium.org/817603002/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/817603002/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:466: void ThreadProxy::OutputSurfaceDidRequestDraw() {
OutputSurfaceDidRequestDrawOnImplThread

https://codereview.chromium.org/817603002/diff/1/content/browser/android/in_p...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h
(right):

https://codereview.chromium.org/817603002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h:24:
void SendBeginFrameNow(); // Uses cc::BeginFrameArgs::DefaultInterval.
Why do you need this?

[sunnyps, 2014-12-20 00:39:30]

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:347: // drawing a frame, however it's possible that
the frame rate controller
On 2014/12/20 00:34:13, brianderson wrote:
> frame rate controller doesn't exist anymore. Since you are touching this
> comment, might as well update it so it refers to the BeginFrameSource instead.
> 
> Note: I notice the advance_commit_state_task_ isn't actually needed anywhere
at
> ToT, because the SytheticBeginFrame source won't get blocked like the frame
rate
> controller used to. However, once Windows switches to the Browser as the
> BeginFrame source, we could have the same deadlock issues - so we sadly can't
> get rid of all polling. So close.

Acknowledged.

https://codereview.chromium.org/817603002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:679: // to advance out of it's waiting to draw state
without actually
On 2014/12/20 00:34:13, brianderson wrote:
> it's -> its

Acknowledged.

https://codereview.chromium.org/817603002/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/817603002/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:466: void ThreadProxy::OutputSurfaceDidRequestDraw() {
On 2014/12/20 00:34:13, brianderson wrote:
> OutputSurfaceDidRequestDrawOnImplThread

Acknowledged.

https://codereview.chromium.org/817603002/diff/1/content/browser/android/in_p...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h
(right):

https://codereview.chromium.org/817603002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h:24:
void SendBeginFrameNow(); // Uses cc::BeginFrameArgs::DefaultInterval.
On 2014/12/20 00:34:13, brianderson wrote:
> Why do you need this?

This is for out-of-sequence draws on Android. For e.g. when onDraw is called by
user code.

E.g. someone might have a webview that's not attached to the view hierarchy but
it's still being used to draw at regular intervals to a canvas.

[boliu, 2015-02-11 02:28:15]

some scattered comments in aw and content

Overall very exciting :D

SetIsActive addressed a lot of my concerns for regressing power usage

I think aw code quality is not on par to see a big change like this. A lot of
things can be cleaned up due to recent changes, that will make this change a lot
easier.

https://codereview.chromium.org/817603002/diff/160001/content/browser/android...
File
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc
(right):

https://codereview.chromium.org/817603002/diff/160001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.cc:35:
cc::BeginFrameArgs args = last_begin_frame_args_;
What happens if last_begin_frame_args_ is never set?

Probably ok to just return, ie this draw fails without swapping?

https://codereview.chromium.org/817603002/diff/160001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/817603002/diff/160001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:131:
OnNeedsBeginFramesChange(begin_frame_source_->NeedsBeginFrames());
Move this to BFS::SetCompositor

https://codereview.chromium.org/817603002/diff/160001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:280:
RenderWidgetHostViewAndroid* rwhv = static_cast<RenderWidgetHostViewAndroid*>(
nts and general comment I guess: should check lifetime of rwhva, maybe it's safe
or one to hold a raw pointer to another (probably this holds a raw pointer to
rwhva)

https://codereview.chromium.org/817603002/diff/160001/content/public/browser/...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/817603002/diff/160001/content/public/browser/...
content/public/browser/android/synchronous_compositor.h:97: virtual void
SendBeginFrame(cc::BeginFrameArgs args) = 0;
No need to expose this to public interface. It's only called from content code.
Also const& args?

https://codereview.chromium.org/817603002/diff/200001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/817603002/diff/200001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:344:
UpdateCompositorIsActive();
Hmm, for now when fallback tick is still not removed, it's still important to
check the state of fallback ticks, since fallback tick might go from throttled
to non-throttled. In that case, if compositor is waiting for an invalidate, then
we need to schedule a fallback tick.

https://codereview.chromium.org/817603002/diff/200001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:612:
invalidate_after_composite_ = true;
I think BVR needs a general clean up before having massive changes like this :(

invalidate_after_composite_ and skip_reschedule_tick can be removed period, we
don't need or want that behavior anymore

block_invalidate_ and fallback_tick_pending_ can probably be merged into one
because DidSkipCompositeInDraw should be removed too

I don't know what's easier, me trying to clean this up first and you try a
rebase, just remove these things here and I'll clean the pieces up later..

https://codereview.chromium.org/817603002/diff/200001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:630: (is_paused_ &&
!clear_view_) || (attached_to_window_ && !window_visible_);
clear_view_ *was* here because we still want activations to happen, because
activations unsets clear_view_. So in theory, we don't care about clear_view_
here (ie draw), but we do care about it in UpdateCompositorIsActive (ie
activation).

But this is all too complicated to reason about. How about use the exact same
(or I guess opposite) condition in both places for now?

https://codereview.chromium.org/817603002/diff/200001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:655: if (compositor_) {
don't really need this check either

https://codereview.chromium.org/817603002/diff/200001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:662: DidComposite();
just remove this, once compositor is gone, it's gone for good

https://codereview.chromium.org/817603002/diff/200001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:679: DidComposite();
ditto

[hush (inactive), 2015-02-11 20:43:39]

hush@chromium.org changed reviewers:
+ hush@chromium.org

[hush (inactive), 2015-02-11 20:43:40]

https://codereview.chromium.org/817603002/diff/200001/content/public/browser/...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/817603002/diff/200001/content/public/browser/...
content/public/browser/android/synchronous_compositor.h:92: // compositor won't
ask for vsyncs when it's active. NOTE: The compositor
do you mean 
The compositor won't ask for vsyncs when it's inactive?

[sunnyps, 2015-02-20 04:49:42]

An update: I've discovered a few problems in the scheduler with respect to
throttling based on active tree draw. This has been the source of Play Newsstand
and Play Books getting stuck while navigating.

In webview we can't rely on a draw happening at a time of our choosing - in
particular draws might stop happening at any given time.

There's a bunch of places where the scheduler stalls it's pipeline waiting for
the first draw of an active tree. Of these stalling the commit is the most
dangerous because that means the main thread stays blocked until a draw happens.
Because we can't guarantee draws it means that the main thread can be blocked
forever - which blocks future navigations, etc. The other actions which are
blocked by active tree draw are activation and (indirectly) send begin main
frame. These aren't so bad because the scheduler can resume later and still run
these actions however this does cause
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor to fail (with
impl side painting only).

[mithro-old, 2015-02-27 02:15:31]

mithro@mithis.com changed reviewers:
+ mithro@mithis.com

[boliu, 2015-03-20 18:59:06]

Looked over android_webview and content and left some comments

And house-keeping:

android_webview_unittests_apk doesn't build

also doesn't build with clang

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:202: fallback_tick_pending_ =
false;
Factor this out into a CancelFallbackTick function.

Also in both cases, we need to make sure to only cancel fallback tick after
CanOnDraw returns true. Probably easier to do that by moving this to the
CompositeHw/Sw functions, which also matches previous behavior.

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:279:
PostInvalidateWithFallback();
Put this before the client update, where EnsureContinuousInvalidation used to be

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:646: client_->PostInvalidate();
This should be above the throttle/fallback pending check. Even the comment says
it should be.

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:219:
DCHECK(!invoking_composite_);
invoking_composite_ isn't really needed anymore (except for the DCHECK that
DrawHw/Sw doesn't re-enter, which isn't that important), feel free to remove it.

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.h:63: void
DidActivatePendingTree();
This can stay private

https://codereview.chromium.org/817603002/diff/400001/content/public/browser/...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/817603002/diff/400001/content/public/browser/...
content/public/browser/android/synchronous_compositor.h:97: virtual void
SendBeginFrame(const cc::BeginFrameArgs& args) = 0;
Not needed here

[sunnyps, 2015-03-20 22:48:40]

Addressed all issues.

PTAL.

NOTE: I found that a DCHECK in InputHandlerProxy fails on flinging while another
fling is active in news stand. I don't believe it has anything to do with my
changes and we've seen similar issues in the past where the app changes the
input stream so that certain invariants don't hold. I'll try without my changes
and file a separate bug for it.

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:202: fallback_tick_pending_ =
false;
On 2015/03/20 18:59:06, boliu wrote:
> Factor this out into a CancelFallbackTick function.
> 
> Also in both cases, we need to make sure to only cancel fallback tick after
> CanOnDraw returns true. Probably easier to do that by moving this to the
> CompositeHw/Sw functions, which also matches previous behavior.

Done.

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:279:
PostInvalidateWithFallback();
On 2015/03/20 18:59:06, boliu wrote:
> Put this before the client update, where EnsureContinuousInvalidation used to
be

Done.

https://codereview.chromium.org/817603002/diff/400001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:646: client_->PostInvalidate();
On 2015/03/20 18:59:06, boliu wrote:
> This should be above the throttle/fallback pending check. Even the comment
says
> it should be.

Done.

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:219:
DCHECK(!invoking_composite_);
On 2015/03/20 18:59:06, boliu wrote:
> invoking_composite_ isn't really needed anymore (except for the DCHECK that
> DrawHw/Sw doesn't re-enter, which isn't that important), feel free to remove
it.

Done.

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/817603002/diff/400001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.h:63: void
DidActivatePendingTree();
On 2015/03/20 18:59:06, boliu wrote:
> This can stay private

Done.

https://codereview.chromium.org/817603002/diff/400001/content/public/browser/...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/817603002/diff/400001/content/public/browser/...
content/public/browser/android/synchronous_compositor.h:97: virtual void
SendBeginFrame(const cc::BeginFrameArgs& args) = 0;
On 2015/03/20 18:59:06, boliu wrote:
> Not needed here

Done.

[mithro-old, 2015-03-23 12:47:10]

Hi Sunny,

You know how webview is suppose to operate better than I do, so if this
suggestion feels wrong, please feel free to ignore. Also added a bunch of other
random comments.

If I understand correctly, we want to drive the whole scheduler and rendering
pipeline via BeginFrame messages but we now draw on a different signal rather
than in the BeginImplFrameDeadline? The webview draw signal could happen
multiple times too.

Why isn't the signal from webview just a "DoBeginImplFrameDeadline" trigger? Is
the problem that multiple BeginFrame's could be triggered before WebView ever
wants to draw?

I was pondering if this would be better keeping the normal deadline structured
but always using something like the  "DRAW_AND_SWAP_ABORT" action during the
DRAW step. Then the new draw signal would just cause a real draw step?

Does that make any sense?

Tim 'mithro' Ansell

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:328: // crbug.com/317430 for an example of a swap ack
being held on commit. Thus
Is the stuff in crbug.com/317430 the only reason keeping this
SetupPollingMechanisms method alive?

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:382: begin_retro_frame_args_.push_back(adjusted_args);
Changing this too adjusted_args seems like an unrelated change?

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:410: void Scheduler::OnDrawForOutputSurface() {
This "hack" for forcing the state_machine_ like this feels dangerous but I can't
really think of a better solution.

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:533: if
(settings_.using_synchronous_renderer_compositor) {
Should we be switching on begin_impl_frame_deadline_mode here?

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:535: state_machine_.OnBeginImplFrameIdle();
This feels very similar to the OnBeginImplFrameDeadline method below.

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:600: void Scheduler::OnBeginImplFrameDeadline() {
Does it make sense to split up this in some way so we don't have to duplicate
the state_machine_.OnBeginImplFrame calls?

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:686: "461509 Scheduler::ProcessScheduledActions2"));
Is changing these FROM_HERE_WITH_EXPLICIT_FUNCTION values cause things to be
collated in a weird way?

Really these should have been named Scheduler::ProcessScheduledActions::Animate
or something....

[sunnyps, 2015-03-23 22:53:19]

Addressed mithro's comments.

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:328: // crbug.com/317430 for an example of a swap ack
being held on commit. Thus
On 2015/03/23 12:47:09, mithro wrote:
> Is the stuff in crbug.com/317430 the only reason keeping this
> SetupPollingMechanisms method alive?

I don't know for sure but I imagine so.

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:382: begin_retro_frame_args_.push_back(adjusted_args);
On 2015/03/23 12:47:09, mithro wrote:
> Changing this too adjusted_args seems like an unrelated change?

Done. I moved this to another CL: https://codereview.chromium.org/1025373002

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:410: void Scheduler::OnDrawForOutputSurface() {
On 2015/03/23 12:47:09, mithro wrote:
> This "hack" for forcing the state_machine_ like this feels dangerous but I
can't
> really think of a better solution.
> 

I don't intend for this to be a hack for long. After this CL lands I intend to
rename any functions/variables in the state machine and scheduler client to
remove any mention of the deadline and replace those with "draw phase" or
similar e.g. SchedulerStateMachine::OnBeginImplFrameDeadline will become
OnBeginImplFrameDraw (or OnDraw).

IMO the deadline is really a platform-specific implementation detail of the
scheduler and "inside the deadline" should not be a synonym for "inside draw".

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:533: if
(settings_.using_synchronous_renderer_compositor) {
On 2015/03/23 12:47:09, mithro wrote:
> Should we be switching on begin_impl_frame_deadline_mode here?
> 

That won't work because the deadline mode is NONE outside of the deadline
pending state and we don't enter the deadline pending state here. Also, I think
it's better to make it clear that we are switching because of the synchronous
compositor.

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:535: state_machine_.OnBeginImplFrameIdle();
On 2015/03/23 12:47:09, mithro wrote:
> This feels very similar to the OnBeginImplFrameDeadline method below.

Done. There are some differences, notably that we don't enter the deadline state
here. I have factored this out into Scheduler::FinishImplFrame.

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:600: void Scheduler::OnBeginImplFrameDeadline() {
On 2015/03/23 12:47:08, mithro wrote:
> Does it make sense to split up this in some way so we don't have to duplicate
> the state_machine_.OnBeginImplFrame calls?

Done. Factored the common stuff out into a FinishImplFrame function.

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:686: "461509 Scheduler::ProcessScheduledActions2"));
On 2015/03/23 12:47:09, mithro wrote:
> Is changing these FROM_HERE_WITH_EXPLICIT_FUNCTION values cause things to be
> collated in a weird way?
> 
> Really these should have been named
Scheduler::ProcessScheduledActions::Animate
> or something....

Yeah, I agree with that. I think robliao@ is removing these in another CL any
way.

[mithro-old, 2015-03-24 05:43:45]

Hi Sunny,

Did you see the suggestions in the top section of the review?

BTW If Brian gives you a LGTM, you don't need to wait for me.

Tim 'mithro' Ansell

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:410: void Scheduler::OnDrawForOutputSurface() {
On 2015/03/23 22:53:19, sunnyps wrote:
> On 2015/03/23 12:47:09, mithro wrote:
> > This "hack" for forcing the state_machine_ like this feels dangerous but I
> can't
> > really think of a better solution.
> > 
> 
> I don't intend for this to be a hack for long. After this CL lands I intend to
> rename any functions/variables in the state machine and scheduler client to
> remove any mention of the deadline and replace those with "draw phase" or
> similar e.g. SchedulerStateMachine::OnBeginImplFrameDeadline will become
> OnBeginImplFrameDraw (or OnDraw).
> 
> IMO the deadline is really a platform-specific implementation detail of the
> scheduler and "inside the deadline" should not be a synonym for "inside draw".

Excellent, I think that will be great to do!

https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:686: "461509 Scheduler::ProcessScheduledActions2"));
On 2015/03/23 22:53:19, sunnyps wrote:
> On 2015/03/23 12:47:09, mithro wrote:
> > Is changing these FROM_HERE_WITH_EXPLICIT_FUNCTION values cause things to be
> > collated in a weird way?
> > 
> > Really these should have been named
> Scheduler::ProcessScheduledActions::Animate
> > or something....
> 
> Yeah, I agree with that. I think robliao@ is removing these in another CL any
> way.

Just want to make sure you don't break robliao@'s stat collection before he
removes it.

[sunnyps, 2015-03-24 05:57:51]

On 2015/03/23 12:47:10, mithro wrote:
> Hi Sunny,
> 
> You know how webview is suppose to operate better than I do, so if this
> suggestion feels wrong, please feel free to ignore. Also added a bunch of
other
> random comments.
> 
> If I understand correctly, we want to drive the whole scheduler and rendering
> pipeline via BeginFrame messages but we now draw on a different signal rather
> than in the BeginImplFrameDeadline? The webview draw signal could happen
> multiple times too.
> 
> Why isn't the signal from webview just a "DoBeginImplFrameDeadline" trigger?
Is
> the problem that multiple BeginFrame's could be triggered before WebView ever
> wants to draw?

Yes, that is correct. WebView can choose not to draw for many frames or at all.

> 
> I was pondering if this would be better keeping the normal deadline structured
> but always using something like the  "DRAW_AND_SWAP_ABORT" action during the
> DRAW step. Then the new draw signal would just cause a real draw step?
> 
> Does that make any sense?

That was essentially what I had done before with the only difference being that
the scheduler would not draw instead of doing an aborted draw. There are
numerous difficulties with that approach - one being that it's possible to
perform actions twice - once in the regular BeginFrame and then in the draw.
Preventing that requires a lot of extra bookkeeping. I think it's easier to
develop a mental model for what I've implemented here: I think of several phases
occurring during a BeginFrame - an animate phase that happens at the beginning,
a deadline phase where we wait for the deadline, a draw phase where we do
anything required for a draw and an idle phase in between frames. How these
phases occur within a BeginFrame (or a Draw in webview's case) is up to the
scheduler but what happens inside the phase is the domain of the state machine.
I believe this will become easy to understand once I rename some things in the
state machine to explicitly refer to these phases by name.

A regular BeginFrame looks like this: idle phase -> animate phase -> deadline
phase -> draw phase -> idle phase
A webview BeginFrame looks like this: idle phase -> animate phase -> idle phase
A webview draw looks like this: idle phase -> draw phase -> idle phase

In the future I can imagine that a regular BeginFrame might be split into two
different types of operation, one for low-latency mode and another for
high-latency mode.

High-latency mode could look something like this: idle phase -> animate phase ->
draw phase (previous frame) -> idle phase which would replace what is called the
"late" deadline mode now.

In short I believe that thinking in terms of these phases is a better way of
thinking about scheduling in cc.

> 
> Tim 'mithro' Ansell
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:328: // crbug.com/317430 for an example of a swap
ack
> being held on commit. Thus
> Is the stuff in crbug.com/317430 the only reason keeping this
> SetupPollingMechanisms method alive?
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:382:
begin_retro_frame_args_.push_back(adjusted_args);
> Changing this too adjusted_args seems like an unrelated change?
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:410: void Scheduler::OnDrawForOutputSurface() {
> This "hack" for forcing the state_machine_ like this feels dangerous but I
can't
> really think of a better solution.
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:533: if
> (settings_.using_synchronous_renderer_compositor) {
> Should we be switching on begin_impl_frame_deadline_mode here?
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:535: state_machine_.OnBeginImplFrameIdle();
> This feels very similar to the OnBeginImplFrameDeadline method below.
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:600: void Scheduler::OnBeginImplFrameDeadline() {
> Does it make sense to split up this in some way so we don't have to duplicate
> the state_machine_.OnBeginImplFrame calls?
> 
>
https://codereview.chromium.org/817603002/diff/440001/cc/scheduler/scheduler....
> cc/scheduler/scheduler.cc:686: "461509 Scheduler::ProcessScheduledActions2"));
> Is changing these FROM_HERE_WITH_EXPLICIT_FUNCTION values cause things to be
> collated in a weird way?
> 
> Really these should have been named
Scheduler::ProcessScheduledActions::Animate
> or something....

[mithro-old, 2015-03-24 06:14:15]

On 24 March 2015 at 16:57, <sunnyps@chromium.org> wrote:

> On 2015/03/23 12:47:10, mithro wrote:
>
>> Hi Sunny,
>>
>
>  You know how webview is suppose to operate better than I do, so if this
>> suggestion feels wrong, please feel free to ignore. Also added a bunch of
>>
> other
>
>> random comments.
>>
>
>  If I understand correctly, we want to drive the whole scheduler and
>> rendering
>> pipeline via BeginFrame messages but we now draw on a different signal
>> rather
>> than in the BeginImplFrameDeadline? The webview draw signal could happen
>> multiple times too.
>>
>
>  Why isn't the signal from webview just a "DoBeginImplFrameDeadline"
>> trigger?
>>
> Is
>
>> the problem that multiple BeginFrame's could be triggered before WebView
>> ever
>> wants to draw?
>>
>
> Yes, that is correct. WebView can choose not to draw for many frames or at
> all.


Is rendering frames when WebView is not drawing them a problem at all? (Are
we wasting resources / CPU time then?)

Do we need some type of throttle on the incoming BeginFrameArgs which tries
to match the Draw() rate? Or is this a case we just don't care about?


> I was pondering if this would be better keeping the normal deadline
>> structured
>> but always using something like the  "DRAW_AND_SWAP_ABORT" action during
>> the
>> DRAW step. Then the new draw signal would just cause a real draw step?
>>
>
>  Does that make any sense?
>>
>
> That was essentially what I had done before with the only difference being
> that
> the scheduler would not draw instead of doing an aborted draw. There are

numerous difficulties with that approach - one being that it's possible to
> perform actions twice - once in the regular BeginFrame and then in the
> draw.
> Preventing that requires a lot of extra bookkeeping.


Did that code end up being uploaded somewhere (IE maybe an older patchset)?
I'm interested in understanding the bookkeeping needed but don't worry if
it is lots in time now.

I think it's easier to
> develop a mental model for what I've implemented here: I think of several
> phases
> occurring during a BeginFrame - an animate phase that happens at the
> beginning,
> a deadline phase where we wait for the deadline, a draw phase where we do
> anything required for a draw and an idle phase in between frames. How these
> phases occur within a BeginFrame (or a Draw in webview's case) is up to the
> scheduler but what happens inside the phase is the domain of the state
> machine.
> I believe this will become easy to understand once I rename some things in
> the
> state machine to explicitly refer to these phases by name.
>
> A regular BeginFrame looks like this: idle phase -> animate phase ->
> deadline
> phase -> draw phase -> idle phase
> A webview BeginFrame looks like this: idle phase -> animate phase -> idle
> phase
> A webview draw looks like this: idle phase -> draw phase -> idle phase
>
> In the future I can imagine that a regular BeginFrame might be split into
> two
> different types of operation, one for low-latency mode and another for
> high-latency mode.
>
> High-latency mode could look something like this: idle phase -> animate
> phase ->
> draw phase (previous frame) -> idle phase which would replace what is
> called the
> "late" deadline mode now.
>
> In short I believe that thinking in terms of these phases is a better way
> of
> thinking about scheduling in cc.


I think if you replace the name "phases" with "tasks" then we are actually
converging on very similar ideas about how the scheduler should end up
operating. Not sure if "phases" is a better name than "tasks" though.

Maybe we should try and collaborate on a design document for the "future of
cc/scheduler"?

Brian, do you have any thoughts?

Tim

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sunnyps, 2015-03-24 06:25:30]

On Mon, Mar 23, 2015 at 11:13 PM, Tim Ansell <mithro@mithis.com> wrote:

> On 24 March 2015 at 16:57, <sunnyps@chromium.org> wrote:
>
>> On 2015/03/23 12:47:10, mithro wrote:
>>
>>> Hi Sunny,
>>>
>>
>>  You know how webview is suppose to operate better than I do, so if this
>>> suggestion feels wrong, please feel free to ignore. Also added a bunch of
>>>
>> other
>>
>>> random comments.
>>>
>>
>>  If I understand correctly, we want to drive the whole scheduler and
>>> rendering
>>> pipeline via BeginFrame messages but we now draw on a different signal
>>> rather
>>> than in the BeginImplFrameDeadline? The webview draw signal could happen
>>> multiple times too.
>>>
>>
>>  Why isn't the signal from webview just a "DoBeginImplFrameDeadline"
>>> trigger?
>>>
>> Is
>>
>>> the problem that multiple BeginFrame's could be triggered before WebView
>>> ever
>>> wants to draw?
>>>
>>
>> Yes, that is correct. WebView can choose not to draw for many frames or
>> at all.
>
>
> Is rendering frames when WebView is not drawing them a problem at all?
> (Are we wasting resources / CPU time then?)
>
> Do we need some type of throttle on the incoming BeginFrameArgs which
> tries to match the Draw() rate? Or is this a case we just don't care about?
>
>
>> I was pondering if this would be better keeping the normal deadline
>>> structured
>>> but always using something like the  "DRAW_AND_SWAP_ABORT" action during
>>> the
>>> DRAW step. Then the new draw signal would just cause a real draw step?
>>>
>>
>>  Does that make any sense?
>>>
>>
>> That was essentially what I had done before with the only difference
>> being that
>> the scheduler would not draw instead of doing an aborted draw. There are
>
> numerous difficulties with that approach - one being that it's possible to
>> perform actions twice - once in the regular BeginFrame and then in the
>> draw.
>> Preventing that requires a lot of extra bookkeeping.
>
>
> Did that code end up being uploaded somewhere (IE maybe an older
> patchset)? I'm interested in understanding the bookkeeping needed but don't
> worry if it is lots in time now.
>

Have a look at patch set 18:
https://codereview.chromium.org/817603002/#ps340001


>
> I think it's easier to
>> develop a mental model for what I've implemented here: I think of several
>> phases
>> occurring during a BeginFrame - an animate phase that happens at the
>> beginning,
>> a deadline phase where we wait for the deadline, a draw phase where we do
>> anything required for a draw and an idle phase in between frames. How
>> these
>> phases occur within a BeginFrame (or a Draw in webview's case) is up to
>> the
>> scheduler but what happens inside the phase is the domain of the state
>> machine.
>> I believe this will become easy to understand once I rename some things
>> in the
>> state machine to explicitly refer to these phases by name.
>>
>> A regular BeginFrame looks like this: idle phase -> animate phase ->
>> deadline
>> phase -> draw phase -> idle phase
>> A webview BeginFrame looks like this: idle phase -> animate phase -> idle
>> phase
>> A webview draw looks like this: idle phase -> draw phase -> idle phase
>>
>> In the future I can imagine that a regular BeginFrame might be split into
>> two
>> different types of operation, one for low-latency mode and another for
>> high-latency mode.
>>
>> High-latency mode could look something like this: idle phase -> animate
>> phase ->
>> draw phase (previous frame) -> idle phase which would replace what is
>> called the
>> "late" deadline mode now.
>>
>> In short I believe that thinking in terms of these phases is a better way
>> of
>> thinking about scheduling in cc.
>
>
> I think if you replace the name "phases" with "tasks" then we are actually
> converging on very similar ideas about how the scheduler should end up
> operating. Not sure if "phases" is a better name than "tasks" though.
>

I said phase because any number of actions could happen during a phase.
E.g. during the draw phase, these actions could happen: animate (because of
commit), draw and prepare tiles. Not all (or any) of these actions might
happen.


>
> Maybe we should try and collaborate on a design document for the "future
> of cc/scheduler"?
>

I think that's overkill at this point. I believe the changes I'm proposing
can be attained in a few small CLs. Moreover I don't think these changes
are absolutely required except for fixing webview scheduling which is this
CL (and maybe removing the late deadline - which I've always hated). I
don't think there's a lot of variety in how these phases can be arranged in
a BeginFrame - low latency, high latency, webview and webview draw are the
only "modes" that come to mind.


>
> Brian, do you have any thoughts?
>
> Tim
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2015-03-25 23:10:38]

Regarding the TODO's for renaming: is that mostly to keep this patch smaller?

Should the Draw functions move to be next to the Swap functions in general since
they are related?

https://codereview.chromium.org/817603002/diff/480001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (left):

https://codereview.chromium.org/817603002/diff/480001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:43: SYNCHRONOUS,
If we kept this around, could it help get rid of the
using_synchronous_renderer_compositor from SchedulerSettings?

https://codereview.chromium.org/817603002/diff/480001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/817603002/diff/480001/cc/output/output_surfac...
cc/output/output_surface.h:158: virtual void Invalidate() {}
Might want to update comments somewhere describing SwapBuffers as the push
model.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:490: return needs_redraw_ ||
needs_prepare_tiles_;
Why needs_prepare_tiles_?

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:563: case
ACTION_INVALIDATE_OUTPUT_SURFACE:
Thanks for making UpdateStateOn* method usage consistent.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:575: // us.
Comment can be one line now.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:703: active_tree_needs_first_draw_ =
false;
I'm worried that this will make us skip frames or draw duplicate frames. For
example, if we activated since the last draw, clear
active_tree_needs_first_draw_ here and then activate again before the next draw.

How often might we not get a draw from WebView after an Invalidate? If WebView
knows it's in this state, can it call something like DrawAbort() instead? That
way we wouldn't have to unconditionally remove the back pressure.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:714: if (HasInitializedOutputSurface())
The other BeginFrameNeeded* methods don't check HasInitializedOutputSurface
since BeginFrameNeeded does that already. SHould we remove it from here too?

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:725: return
(BeginFrameNeededToAnimateOrDraw() || BeginFrameNeededForChildren() ||
So much easier to understand!

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:814: // Clear funnels for any actions we
perform during the deadline.
Why are these cleared in both the deadline and BeginImplFrame?

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:819: // "Drain" the PrepareTiles funnel.
Why is this moved to the deadline rather than the BeginImplFrame where it was
before?

https://codereview.chromium.org/817603002/diff/480001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/817603002/diff/480001/cc/surfaces/display.h#n...
cc/surfaces/display.h:87: void OnDraw() override {}
NOTREACHED?

https://codereview.chromium.org/817603002/diff/480001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/817603002/diff/480001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:907: void
SingleThreadProxy::ScheduledActionInvalidateOutputSurface() {
NOTREACHED?

[sunnyps, 2015-03-27 00:52:49]

Addressed comments. PTAL.

https://codereview.chromium.org/817603002/diff/480001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (left):

https://codereview.chromium.org/817603002/diff/480001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:43: SYNCHRONOUS,
On 2015/03/25 23:10:37, brianderson wrote:
> If we kept this around, could it help get rid of the
> using_synchronous_renderer_compositor from SchedulerSettings?

I would prefer not keeping this around - it seems like the wrong place to store
whether we're in webview mode or not. Also having this in SchedulerSettings
means that it's a static flag - we don't have to worry about it changing from
frame to frame.

https://codereview.chromium.org/817603002/diff/480001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/817603002/diff/480001/cc/output/output_surfac...
cc/output/output_surface.h:158: virtual void Invalidate() {}
On 2015/03/25 23:10:37, brianderson wrote:
> Might want to update comments somewhere describing SwapBuffers as the push
> model.

In both the push and pull model, swap buffers is called. The difference is who
decides when that happens.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:490: return needs_redraw_ ||
needs_prepare_tiles_;
On 2015/03/25 23:10:37, brianderson wrote:
> Why needs_prepare_tiles_?

The previous draw might have caused tile properties to become dirty and set
needs_prepare_tiles_. It's possible we didn't prepare tiles then because of the
prepare tiles funnel. If we don't ask webview for a draw again we will never
call PrepareTiles but still keep asking for BeginFrames indefinitely.

In practice not having this causes the scheduler to never go to idle on certain
apps like Play Books.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:575: // us.
On 2015/03/25 23:10:37, brianderson wrote:
> Comment can be one line now.

Done.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:703: active_tree_needs_first_draw_ =
false;
On 2015/03/25 23:10:37, brianderson wrote:
> I'm worried that this will make us skip frames or draw duplicate frames. For
> example, if we activated since the last draw, clear
> active_tree_needs_first_draw_ here and then activate again before the next
draw.
> 
> How often might we not get a draw from WebView after an Invalidate? If WebView
> knows it's in this state, can it call something like DrawAbort() instead? That
> way we wouldn't have to unconditionally remove the back pressure.

Webview can't even tell if it's in this state. Since webview allows apps to call
Java code from JS - it's possible to stop drawing at any point. Not setting this
flag to false can cause commit to be blocked indefinitely. An alternative to
this would be to not block commit due to this flag.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:714: if (HasInitializedOutputSurface())
On 2015/03/25 23:10:37, brianderson wrote:
> The other BeginFrameNeeded* methods don't check HasInitializedOutputSurface
> since BeginFrameNeeded does that already. SHould we remove it from here too? 

Done.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:814: // Clear funnels for any actions we
perform during the deadline.
On 2015/03/25 23:10:37, brianderson wrote:
> Why are these cleared in both the deadline and BeginImplFrame?

For invalidate_output_surface_funnel_ the reason is that we can invalidate in
both the BeginImplFrame phase and the Draw phase.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:819: // "Drain" the PrepareTiles funnel.
On 2015/03/25 23:10:37, brianderson wrote:
> Why is this moved to the deadline rather than the BeginImplFrame where it was
> before?

Because prepare tiles (when initiated by the scheduler) is performed only in the
deadline. We should revisit this if we make changes to when PrepareTiles gets
called.

https://codereview.chromium.org/817603002/diff/480001/cc/surfaces/display.h
File cc/surfaces/display.h (right):

https://codereview.chromium.org/817603002/diff/480001/cc/surfaces/display.h#n...
cc/surfaces/display.h:87: void OnDraw() override {}
On 2015/03/25 23:10:37, brianderson wrote:
> NOTREACHED?

Done.

https://codereview.chromium.org/817603002/diff/480001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/817603002/diff/480001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:907: void
SingleThreadProxy::ScheduledActionInvalidateOutputSurface() {
On 2015/03/25 23:10:37, brianderson wrote:
> NOTREACHED?

Done.

[brianderson, 2015-03-27 21:24:24]

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:490: return needs_redraw_ ||
needs_prepare_tiles_;
On 2015/03/27 00:52:48, sunnyps wrote:
> On 2015/03/25 23:10:37, brianderson wrote:
> > Why needs_prepare_tiles_?
> 
> The previous draw might have caused tile properties to become dirty and set
> needs_prepare_tiles_. It's possible we didn't prepare tiles then because of
the
> prepare tiles funnel. If we don't ask webview for a draw again we will never
> call PrepareTiles but still keep asking for BeginFrames indefinitely.
> 
> In practice not having this causes the scheduler to never go to idle on
certain
> apps like Play Books.

I think you can remove needs_prepare_tiles_ from here and still avoid the
scheduler never going idle if you drain the prepare_tiles_funnel__ in
BeginImplFrame rather than the deadline.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:703: active_tree_needs_first_draw_ =
false;
On 2015/03/27 00:52:48, sunnyps wrote:
> On 2015/03/25 23:10:37, brianderson wrote:
> > I'm worried that this will make us skip frames or draw duplicate frames. For
> > example, if we activated since the last draw, clear
> > active_tree_needs_first_draw_ here and then activate again before the next
> draw.
> > 
> > How often might we not get a draw from WebView after an Invalidate? If
WebView
> > knows it's in this state, can it call something like DrawAbort() instead?
That
> > way we wouldn't have to unconditionally remove the back pressure.
> 
> Webview can't even tell if it's in this state. Since webview allows apps to
call
> Java code from JS - it's possible to stop drawing at any point. Not setting
this
> flag to false can cause commit to be blocked indefinitely. An alternative to
> this would be to not block commit due to this flag.

Is the problem that we are blocking the main thread after sending a
BeginMainFrame? Or is it that we never send a BeginMainFrame in the first place?

In the first case, I think that could only happen because of the
SetNextCommitWaitsForActivation logic. In which case, maybe we should remove it.

In the second case, I think that is what we want. If the app isn't drawing, we
shouldn't start new commits.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:814: // Clear funnels for any actions we
perform during the deadline.
On 2015/03/27 00:52:48, sunnyps wrote:
> On 2015/03/25 23:10:37, brianderson wrote:
> > Why are these cleared in both the deadline and BeginImplFrame?
> 
> For invalidate_output_surface_funnel_ the reason is that we can invalidate in
> both the BeginImplFrame phase and the Draw phase.

Can add a comment about that and also explain in what cases it makes sense to
invalidate twice in one frame?

Without knowing details, I feel like resetting the funnel only in the deadline
would be sufficient. If we invalidate just after draw1, we should be able to
expect a draw2 even if we don't invalidate in the BeginImplFrame2.  If we don't
invalidate just after draw1, the funnel will be empty, allowing BeginImplFrame2
to invalidate if needed.

Is there a hole in my understanding of WebView?

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:819: // "Drain" the PrepareTiles funnel.
On 2015/03/27 00:52:48, sunnyps wrote:
> On 2015/03/25 23:10:37, brianderson wrote:
> > Why is this moved to the deadline rather than the BeginImplFrame where it
was
> > before?
> 
> Because prepare tiles (when initiated by the scheduler) is performed only in
the
> deadline. We should revisit this if we make changes to when PrepareTiles gets
> called.

Was the intent not to have a functional change by moving it here?

I think this will sometimes call prepare tiles inside of a draw just after
having prepared tiles for a commit, which is something we want to avoid.

[sunnyps, 2015-03-27 22:38:25]

Addressed comments. PTAL.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:490: return needs_redraw_ ||
needs_prepare_tiles_;
On 2015/03/27 21:24:24, brianderson wrote:
> On 2015/03/27 00:52:48, sunnyps wrote:
> > On 2015/03/25 23:10:37, brianderson wrote:
> > > Why needs_prepare_tiles_?
> > 
> > The previous draw might have caused tile properties to become dirty and set
> > needs_prepare_tiles_. It's possible we didn't prepare tiles then because of
> the
> > prepare tiles funnel. If we don't ask webview for a draw again we will never
> > call PrepareTiles but still keep asking for BeginFrames indefinitely.
> > 
> > In practice not having this causes the scheduler to never go to idle on
> certain
> > apps like Play Books.
> 
> I think you can remove needs_prepare_tiles_ from here and still avoid the
> scheduler never going idle if you drain the prepare_tiles_funnel__ in
> BeginImplFrame rather than the deadline.

I moved the prepare_tiles_funnel_ decrement to OnBeginImplFrame but that does
not alleviate the need for checking needs_prepare_tiles here.

If needs_prepare_tiles is true we keep getting BeginFrames but unless we enter
the deadline phase we don't call PrepareTiles.

I know this causes some extra drawing to happen just for the sake of calling
PrepareTiles but it should be okay.

Aside: It would be great if we could call UDP+PrepareTiles on BeginImplFrame if
commit state is idle. That would solve this problem and probably other problems
as well.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:703: active_tree_needs_first_draw_ =
false;
On 2015/03/27 21:24:24, brianderson wrote:
> On 2015/03/27 00:52:48, sunnyps wrote:
> > On 2015/03/25 23:10:37, brianderson wrote:
> > > I'm worried that this will make us skip frames or draw duplicate frames.
For
> > > example, if we activated since the last draw, clear
> > > active_tree_needs_first_draw_ here and then activate again before the next
> > draw.
> > > 
> > > How often might we not get a draw from WebView after an Invalidate? If
> WebView
> > > knows it's in this state, can it call something like DrawAbort() instead?
> That
> > > way we wouldn't have to unconditionally remove the back pressure.
> > 
> > Webview can't even tell if it's in this state. Since webview allows apps to
> call
> > Java code from JS - it's possible to stop drawing at any point. Not setting
> this
> > flag to false can cause commit to be blocked indefinitely. An alternative to
> > this would be to not block commit due to this flag.
> 
> Is the problem that we are blocking the main thread after sending a
> BeginMainFrame? Or is it that we never send a BeginMainFrame in the first
place?

The problem is that the main thread is blocked indefinitely while it's waiting
for a commit to happen.

> 
> In the first case, I think that could only happen because of the
> SetNextCommitWaitsForActivation logic. In which case, maybe we should remove
it.
> 
> In the second case, I think that is what we want. If the app isn't drawing, we
> shouldn't start new commits.
> 

The first case also happens if commit is blocked indefinitely.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:814: // Clear funnels for any actions we
perform during the deadline.
On 2015/03/27 21:24:24, brianderson wrote:
> On 2015/03/27 00:52:48, sunnyps wrote:
> > On 2015/03/25 23:10:37, brianderson wrote:
> > > Why are these cleared in both the deadline and BeginImplFrame?
> > 
> > For invalidate_output_surface_funnel_ the reason is that we can invalidate
in
> > both the BeginImplFrame phase and the Draw phase.
> 
> Can add a comment about that and also explain in what cases it makes sense to
> invalidate twice in one frame?
> 
> Without knowing details, I feel like resetting the funnel only in the deadline
> would be sufficient. If we invalidate just after draw1, we should be able to
> expect a draw2 even if we don't invalidate in the BeginImplFrame2.  If we
don't
> invalidate just after draw1, the funnel will be empty, allowing
BeginImplFrame2
> to invalidate if needed.
> 
> Is there a hole in my understanding of WebView?

I changed this so that it's invalidated only once, I think you're looking at old
patch set.

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:819: // "Drain" the PrepareTiles funnel.
On 2015/03/27 21:24:24, brianderson wrote:
> On 2015/03/27 00:52:48, sunnyps wrote:
> > On 2015/03/25 23:10:37, brianderson wrote:
> > > Why is this moved to the deadline rather than the BeginImplFrame where it
> was
> > > before?
> > 
> > Because prepare tiles (when initiated by the scheduler) is performed only in
> the
> > deadline. We should revisit this if we make changes to when PrepareTiles
gets
> > called.
> 
> Was the intent not to have a functional change by moving it here?
> 
> I think this will sometimes call prepare tiles inside of a draw just after
> having prepared tiles for a commit, which is something we want to avoid.

Good catch. Done.

[sunnyps, 2015-03-30 21:49:18]

brianderson, boliu: PTAL

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/817603002/diff/480001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:703: active_tree_needs_first_draw_ =
false;
On 2015/03/27 22:38:24, sunnyps wrote:
> On 2015/03/27 21:24:24, brianderson wrote:
> > On 2015/03/27 00:52:48, sunnyps wrote:
> > > On 2015/03/25 23:10:37, brianderson wrote:
> > > > I'm worried that this will make us skip frames or draw duplicate frames.
> For
> > > > example, if we activated since the last draw, clear
> > > > active_tree_needs_first_draw_ here and then activate again before the
next
> > > draw.
> > > > 
> > > > How often might we not get a draw from WebView after an Invalidate? If
> > WebView
> > > > knows it's in this state, can it call something like DrawAbort()
instead?
> > That
> > > > way we wouldn't have to unconditionally remove the back pressure.
> > > 
> > > Webview can't even tell if it's in this state. Since webview allows apps
to
> > call
> > > Java code from JS - it's possible to stop drawing at any point. Not
setting
> > this
> > > flag to false can cause commit to be blocked indefinitely. An alternative
to
> > > this would be to not block commit due to this flag.
> > 
> > Is the problem that we are blocking the main thread after sending a
> > BeginMainFrame? Or is it that we never send a BeginMainFrame in the first
> place?
> 
> The problem is that the main thread is blocked indefinitely while it's waiting
> for a commit to happen.
> 
> > 
> > In the first case, I think that could only happen because of the
> > SetNextCommitWaitsForActivation logic. In which case, maybe we should remove
> it.
> > 
> > In the second case, I think that is what we want. If the app isn't drawing,
we
> > shouldn't start new commits.
> > 
> 
> The first case also happens if commit is blocked indefinitely.

I removed this piece of code and found that I could not reproduce the bug that
it fixed any more. So I'm removing it - we can add it later if we see the same
symptoms again.

[boliu, 2015-03-30 22:23:59]

android_webview lgtm

in_process/ lgtm too even though I don't own that

You'll want jdduke@ and sievers@ to look at other parts of content, especially
render_widget_host_view_android.cc

[jdduke (slow), 2015-03-30 22:30:40]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-03-30 22:30:42]

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1488: if
(stop_observing_root_window)
Bo pointed out that WebView apps might conceivably be visible *and* animating
after the activity is paused and we call RWHVAndroid::Hide.

One solution for that would be to remove this StopObservingRootWindow() call, as
it's really an optimization. Instead, we'd rely solely on the
OnSetNeedsBeginFrame logic, which for WebView isn't disabled when the web
contents is hidden. However, Bo mentioned that it might be worth breaking such
non-conforming cases explicitly for the sake of power savings, so we can
probably just cross that bridge when we come to it.

[sunnyps, 2015-03-30 22:32:05]

sunnyps@chromium.org changed reviewers:
- jdduke@chromium.org

[sunnyps, 2015-03-30 22:35:14]

sunnyps@chromium.org changed reviewers:
+ jdduke@chromium.org, sievers@chromium.org

[sunnyps, 2015-03-30 22:35:15]

jdduke, sievers: PTAL at content/ changes. Thanks!

[brianderson, 2015-04-01 00:23:39]

cc lgtm

https://codereview.chromium.org/817603002/diff/640001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/817603002/diff/640001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:494: return needs_redraw_ ||
needs_prepare_tiles_;
Can you add a TODO explaining what would need to happen to remove
needs_prepare_tiles_ here?

[jdduke (slow), 2015-04-01 16:01:15]

Looks good though I'm not an owner for any of this code.

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1592:
SynchronousCompositorImpl* compositor = SynchronousCompositorImpl::FromID(
Hmm, the other places we use SynchronousCompositorImpl we always check for null.
Should we be doing that here as well?

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1594: 
Can you add a note here briefly mentioning why the SynchronousCompositor doesn't
use the deadline?

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1595:
compositor->SendBeginFrame(cc::BeginFrameArgs::Create(
I'm sure this has already been covered, but why is this called |SendBeginFrame|
instead of just |BeginFrame|?

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1944: // This
allows for SendBeginFrame to modify outstanding_vsync_requests.
"allows for SendBeginFrame and FlushInput"

[jdduke (slow), 2015-04-01 16:23:39]

On 2015/04/01 16:01:15, jdduke wrote:
> Looks good though I'm not an owner for any of this code.

That is, RWHVAndroid changes look good, I haven't reviewed anything else :/.

[jdduke (slow), 2015-04-01 16:23:44]

On 2015/04/01 16:01:15, jdduke wrote:
> Looks good though I'm not an owner for any of this code.

That is, RWHVAndroid changes look good, I haven't reviewed anything else :/.

[no sievers, 2015-04-01 17:37:43]

lgtm, nice.

[sunnyps, 2015-04-01 19:53:02]

Addressed remaining comments. Thanks everyone!

https://codereview.chromium.org/817603002/diff/200001/content/public/browser/...
File content/public/browser/android/synchronous_compositor.h (right):

https://codereview.chromium.org/817603002/diff/200001/content/public/browser/...
content/public/browser/android/synchronous_compositor.h:92: // compositor won't
ask for vsyncs when it's active. NOTE: The compositor
On 2015/02/11 20:43:40, hush wrote:
> do you mean 
> The compositor won't ask for vsyncs when it's inactive?

Done.

https://codereview.chromium.org/817603002/diff/640001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/817603002/diff/640001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:494: return needs_redraw_ ||
needs_prepare_tiles_;
On 2015/04/01 00:23:38, brianderson wrote:
> Can you add a TODO explaining what would need to happen to remove
> needs_prepare_tiles_ here?

Done.

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1592:
SynchronousCompositorImpl* compositor = SynchronousCompositorImpl::FromID(
On 2015/04/01 16:01:14, jdduke wrote:
> Hmm, the other places we use SynchronousCompositorImpl we always check for
null.
> Should we be doing that here as well?

Done.

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1594: 
On 2015/04/01 16:01:15, jdduke wrote:
> Can you add a note here briefly mentioning why the SynchronousCompositor
doesn't
> use the deadline?

Done.

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1595:
compositor->SendBeginFrame(cc::BeginFrameArgs::Create(
On 2015/04/01 16:01:15, jdduke wrote:
> I'm sure this has already been covered, but why is this called
|SendBeginFrame|
> instead of just |BeginFrame|?

Done.

https://codereview.chromium.org/817603002/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1944: // This
allows for SendBeginFrame to modify outstanding_vsync_requests.
On 2015/04/01 16:01:14, jdduke wrote:
> "allows for SendBeginFrame and FlushInput"

Done.

[sunnyps, 2015-04-01 19:53:42]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-04-01 19:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com,
boliu@chromium.org, brianderson@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/817603002/#ps680001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-04-01 19:54:36]

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

[sunnyps, 2015-04-01 21:00:48]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2015-04-01 21:02:00]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-04-01 21:02:38]

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

[sunnyps, 2015-04-01 22:00:36]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-04-01 22:00:36]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
mithro@mithis.com, brianderson@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/817603002/#ps720001
(title: "Fix broken tests.")

[commit-bot: I haz the power, 2015-04-01 22:03:49]

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

[commit-bot: I haz the power, 2015-04-01 23:08:05]

Committed patchset #37 (id:720001)

[commit-bot: I haz the power, 2015-04-01 23:08:53]

Patchset 37 (id:??) landed as
https://crrev.com/bf27da634790bb6eecf4b89f278cfd55c5e5d1f3
Cr-Commit-Position: refs/heads/master@{#323361}

[sunnyps, 2015-04-02 00:42:00]

A revert of this CL (patchset #37 id:720001) has been created in
https://codereview.chromium.org/1052103002/ by sunnyps@chromium.org.

The reason for reverting is: Caused failures on memory bot..

[sunnyps, 2015-04-02 01:31:54]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-04-02 01:31:55]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
mithro@mithis.com, brianderson@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/817603002/#ps740001
(title: "Fix uninitialized variable.")

[commit-bot: I haz the power, 2015-04-02 01:32:34]

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

[commit-bot: I haz the power, 2015-04-02 02:32:11]

Committed patchset #38 (id:740001)

[sunnyps, 2015-04-02 03:03:21]

A revert of this CL (patchset #38 id:740001) has been created in
https://codereview.chromium.org/1055743002/ by sunnyps@chromium.org.

The reason for reverting is: Causes a webview test to fail.
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/19230.

[boliu, 2015-04-02 03:32:56]

VisualStateTest.java lgtm

[sunnyps, 2015-04-02 03:33:14]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-04-02 03:33:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com,
brianderson@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/817603002/#ps760001
(title: "Disable test as suggested by boliu")

[commit-bot: I haz the power, 2015-04-02 03:33:51]

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

[commit-bot: I haz the power, 2015-04-02 06:23:54]

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

[commit-bot: I haz the power, 2015-04-02 06:23:56]

Failed to apply patch for cc/scheduler/scheduler_state_machine.cc:
While running git apply --index -3 -p1;
  error: patch failed: cc/scheduler/scheduler_state_machine.cc:52
  Falling back to three-way merge...
  Applied patch to 'cc/scheduler/scheduler_state_machine.cc' with conflicts.
  U cc/scheduler/scheduler_state_machine.cc

Patch:       cc/scheduler/scheduler_state_machine.cc
Index: cc/scheduler/scheduler_state_machine.cc
diff --git a/cc/scheduler/scheduler_state_machine.cc
b/cc/scheduler/scheduler_state_machine.cc
index
e3c576306bbe57c6604e3f096b901c0016cf6fe5..24170f7dca34a4c947f94b8c3d45c0d0b733bc9d
100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -26,10 +26,11 @@ SchedulerStateMachine::SchedulerStateMachine(const
SchedulerSettings& settings)
       last_frame_number_swap_performed_(-1),
       last_frame_number_swap_requested_(-1),
       last_frame_number_begin_main_frame_sent_(-1),
+      last_frame_number_invalidate_output_surface_performed_(-1),
       animate_funnel_(false),
-      perform_swap_funnel_(false),
       request_swap_funnel_(false),
       send_begin_main_frame_funnel_(false),
+      invalidate_output_surface_funnel_(false),
       prepare_tiles_funnel_(0),
       consecutive_checkerboard_animations_(0),
       max_pending_swaps_(1),
@@ -38,7 +39,6 @@ SchedulerStateMachine::SchedulerStateMachine(const
SchedulerSettings& settings)
       needs_animate_(false),
       needs_prepare_tiles_(false),
       needs_commit_(false),
-      inside_poll_for_anticipated_draw_triggers_(false),
       visible_(false),
       can_start_(false),
       can_draw_(false),
@@ -52,7 +52,9 @@ SchedulerStateMachine::SchedulerStateMachine(const
SchedulerSettings& settings)
       continuous_painting_(false),
       children_need_begin_frames_(false),
       defer_commits_(false),
-      last_commit_had_no_updates_(false) {
+      last_commit_had_no_updates_(false),
+      did_request_swap_in_last_frame_(false),
+      did_perform_swap_in_last_draw_(false) {
 }
 
 const char* SchedulerStateMachine::OutputSurfaceStateToString(
@@ -89,6 +91,22 @@ const char*
SchedulerStateMachine::BeginImplFrameStateToString(
   return "???";
 }
 
+const char* SchedulerStateMachine::BeginImplFrameDeadlineModeToString(
+    BeginImplFrameDeadlineMode mode) {
+  switch (mode) {
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_NONE:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_NONE";
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_IMMEDIATE:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_IMMEDIATE";
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_REGULAR:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_REGULAR";
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_LATE:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_LATE";
+  }
+  NOTREACHED();
+  return "???";
+}
+
 const char* SchedulerStateMachine::CommitStateToString(CommitState state) {
   switch (state) {
     case COMMIT_STATE_IDLE:
@@ -146,6 +164,8 @@ const char* SchedulerStateMachine::ActionToString(Action
action) {
       return "ACTION_BEGIN_OUTPUT_SURFACE_CREATION";
     case ACTION_PREPARE_TILES:
       return "ACTION_PREPARE_TILES";
+    case ACTION_INVALIDATE_OUTPUT_SURFACE:
+      return "ACTION_INVALIDATE_OUTPUT_SURFACE";
   }
   NOTREACHED();
   return "???";
@@ -184,11 +204,12 @@ void SchedulerStateMachine::AsValueInto(
   state->SetInteger("last_frame_number_begin_main_frame_sent",
                     last_frame_number_begin_main_frame_sent_);
   state->SetBoolean("funnel: animate_funnel", animate_funnel_);
-  state->SetBoolean("funnel: perform_swap_funnel", perform_swap_funnel_);
   state->SetBoolean("funnel: request_swap_funnel", request_swap_funnel_);
   state->SetBoolean("funnel: send_begin_main_frame_funnel",
                     send_begin_main_frame_funnel_);
   state->SetInteger("funnel: prepare_tiles_funnel", prepare_tiles_funnel_);
+  state->SetBoolean("funnel: invalidate_output_surface_funnel",
+                    invalidate_output_surface_funnel_);
   state->SetInteger("consecutive_checkerboard_animations",
                     consecutive_checkerboard_animations_);
   state->SetInteger("max_pending_swaps_", max_pending_swaps_);
@@ -218,26 +239,14 @@ void SchedulerStateMachine::AsValueInto(
   state->SetBoolean("continuous_painting", continuous_painting_);
   state->SetBoolean("children_need_begin_frames", children_need_begin_frames_);
   state->SetBoolean("defer_commits", defer_commits_);
+  state->SetBoolean("last_commit_had_no_updates", last_commit_had_no_updates_);
+  state->SetBoolean("did_request_swap_in_last_frame",
+                    did_request_swap_in_last_frame_);
+  state->SetBoolean("did_perform_swap_in_last_draw",
+                    did_perform_swap_in_last_draw_);
   state->EndDictionary();
 }
 
-void SchedulerStateMachine::AdvanceCurrentFrameNumber() {
-  current_frame_number_++;
-
-  animate_funnel_ = false;
-  perform_swap_funnel_ = false;
-  request_swap_funnel_ = false;
-  send_begin_main_frame_funnel_ = false;
-
-  // "Drain" the PrepareTiles funnel.
-  if (prepare_tiles_funnel_ > 0)
-    prepare_tiles_funnel_--;
-
-  skip_begin_main_frame_to_reduce_latency_ =
-      skip_next_begin_main_frame_to_reduce_latency_;
-  skip_next_begin_main_frame_to_reduce_latency_ = false;
-}
-
 bool SchedulerStateMachine::PendingDrawsShouldBeAborted() const {
   // These are all the cases where we normally cannot or do not want to draw
   // but, if needs_redraw_ is true and we do not draw to make forward progress,
@@ -427,7 +436,7 @@ bool SchedulerStateMachine::ShouldSendBeginMainFrame() const
{
   // TODO(brianderson): Remove this restriction to improve throughput.
   bool just_swapped_in_deadline =
       begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE &&
-      perform_swap_funnel_;
+      did_perform_swap_in_last_draw_;
   if (pending_swaps_ >= max_pending_swaps_ && !just_swapped_in_deadline)
     return false;
 
@@ -462,14 +471,32 @@ bool SchedulerStateMachine::ShouldPrepareTiles() const {
     return false;
 
   // Limiting to once per-frame is not enough, since we only want to
-  // prepare tiles _after_ draws. Polling for draw triggers and
-  // begin-frame are mutually exclusive, so we limit to these two cases.
-  if (begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE &&
-      !inside_poll_for_anticipated_draw_triggers_)
+  // prepare tiles _after_ draws.
+  if (begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE)
     return false;
+
   return needs_prepare_tiles_;
 }
 
+bool SchedulerStateMachine::ShouldInvalidateOutputSurface() const {
+  // Do not invalidate too many times in a frame.
+  if (invalidate_output_surface_funnel_)
+    return false;
+
+  // Only the synchronous compositor requires invalidations.
+  if (!settings_.using_synchronous_renderer_compositor)
+    return false;
+
+  // Invalidations are only performed inside a BeginFrame.
+  if (begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING)
+    return false;
+
+  // TODO(sunnyps): needs_prepare_tiles_ is needed here because PrepareTiles is
+  // called only inside the deadline / draw phase. We could remove this if we
+  // allowed PrepareTiles to happen in OnBeginImplFrame.
+  return needs_redraw_ || needs_prepare_tiles_;
+}
+
 SchedulerStateMachine::Action SchedulerStateMachine::NextAction() const {
   if (ShouldActivatePendingTree())
     return ACTION_ACTIVATE_SYNC_TREE;
@@ -489,6 +516,8 @@ SchedulerStateMachine::Action
SchedulerStateMachine::NextAction() const {
     return ACTION_PREPARE_TILES;
   if (ShouldSendBeginMainFrame())
     return ACTION_SEND_BEGIN_MAIN_FRAME;
+  if (ShouldInvalidateOutputSurface())
+    return ACTION_INVALIDATE_OUTPUT_SURFACE;
   if (ShouldBeginOutputSurfaceCreation())
     return ACTION_BEGIN_OUTPUT_SURFACE_CREATION;
   return ACTION_NONE;
@@ -504,25 +533,11 @@ void SchedulerStateMachine::UpdateState(Action action) {
       return;
 
     case ACTION_ANIMATE:
-      DCHECK(!animate_funnel_);
-      last_frame_number_animate_performed_ = current_frame_number_;
-      animate_funnel_ = true;
-      needs_animate_ = false;
-      // TODO(skyostil): Instead of assuming this, require the client to tell
-      // us.
-      SetNeedsRedraw();
+      UpdateStateOnAnimate();
       return;
 
     case ACTION_SEND_BEGIN_MAIN_FRAME:
-      DCHECK(!has_pending_tree_ ||
-             settings_.main_frame_before_activation_enabled);
-      DCHECK(visible_);
-      DCHECK(!send_begin_main_frame_funnel_);
-      commit_state_ = COMMIT_STATE_BEGIN_MAIN_FRAME_SENT;
-      needs_commit_ = false;
-      send_begin_main_frame_funnel_ = true;
-      last_frame_number_begin_main_frame_sent_ =
-          current_frame_number_;
+      UpdateStateOnSendBeginMainFrame();
       return;
 
     case ACTION_COMMIT: {
@@ -545,26 +560,42 @@ void SchedulerStateMachine::UpdateState(Action action) {
     }
 
     case ACTION_BEGIN_OUTPUT_SURFACE_CREATION:
-      DCHECK_EQ(output_surface_state_, OUTPUT_SURFACE_LOST);
-      output_surface_state_ = OUTPUT_SURFACE_CREATING;
-
-      // The following DCHECKs make sure we are in the proper quiescent state.
-      // The pipeline should be flushed entirely before we start output
-      // surface creation to avoid complicated corner cases.
-      DCHECK_EQ(commit_state_, COMMIT_STATE_IDLE);
-      DCHECK(!has_pending_tree_);
-      DCHECK(!active_tree_needs_first_draw_);
+      UpdateStateOnBeginOutputSurfaceCreation();
       return;
 
     case ACTION_PREPARE_TILES:
       UpdateStateOnPrepareTiles();
       return;
+
+    case ACTION_INVALIDATE_OUTPUT_SURFACE:
+      UpdateStateOnInvalidateOutputSurface();
+      return;
   }
 }
 
+void SchedulerStateMachine::UpdateStateOnAnimate() {
+  DCHECK(!animate_funnel_);
+  last_frame_number_animate_performed_ = current_frame_number_;
+  animate_funnel_ = true;
+  needs_animate_ = false;
+  // TODO(skyostil): Instead of assuming this, require the client to tell us.
+  SetNe…
(message too large)

[commit-bot: I haz the power, 2015-04-02 06:24:17]

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

[sunnyps, 2015-04-02 16:29:27]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-04-02 16:29:27]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
mithro@mithis.com, brianderson@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/817603002/#ps780001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-04-02 16:30:48]

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

[sunnyps, 2015-04-02 16:43:06]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2015-04-02 16:50:42]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-04-02 16:50:43]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
mithro@mithis.com, brianderson@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/817603002/#ps800001
(title: "Fix rebase compile errors.")

[sunnyps, 2015-04-02 16:50:44]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2015-04-02 16:51:04]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-04-02 16:52:29]

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

[commit-bot: I haz the power, 2015-04-02 20:26:26]

Committed patchset #41 (id:800001)

[Justin Novosad, 2015-04-02 21:17:35]

A revert of this CL (patchset #41 id:800001) has been created in
https://codereview.chromium.org/1051273003/ by junov@chromium.org.

The reason for reverting is: This broke the build:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20GN/builds/26654/....

[sunnyps, 2015-04-02 21:24:09]

On 2015/04/02 21:17:35, junov wrote:
> A revert of this CL (patchset #41 id:800001) has been created in
> https://codereview.chromium.org/1051273003/ by mailto:junov@chromium.org.
> 
> The reason for reverting is: This broke the build:
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20GN/builds/26654/....

Unchecked the commit bit on the revert. The build broke due to colliding commits
and was fixed in https://codereview.chromium.org/1053083003/

[commit-bot: I haz the power, 2015-04-03 20:21:52]

Patchset 41 (id:??) landed as
https://crrev.com/5333e1468ef3af0099769d4ec6a4c74e9e603766
Cr-Commit-Position: refs/heads/master@{#323410}

[commit-bot: I haz the power, 2015-04-03 20:28:39]

Patchset 41 (id:??) landed as
https://crrev.com/eab5ac980c7b7a5781dc18a1217911eb362698f0
Cr-Commit-Position: refs/heads/master@{#323553}