Add DisplayScheduler for Surfaces

Add DisplayScheduler for Surfaces

This uses a SyntheticBeginFrameSource and a simple
BeginFrame + deadline to determine when to draw.

A simple heuristic based on recent activity is
used to detect currently active surfaces that we
want to wait for.

Future patches will put this in charge of sending
BeginFrames to surface produces, recovering
producer latency, and replace the heuristic with
something more correct.

BUG=467750
Committed: https://crrev.com/893604464fd906112dd5ff3eec9cc99f6ca645fc
Cr-Commit-Position: refs/heads/master@{#324980}

[brianderson, 2015-03-16 23:17:26]

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

[brianderson, 2015-03-16 23:17:27]

Ptal.

Still need to add unit tests, but would like some early feedback.

The first patch set is a straight copy of jbauman's patch from
https://codereview.chromium.org/720333005/ to make diff'ing easier.

[mithro-old, 2015-03-23 01:23:55]

On 2015/03/16 23:17:27, brianderson wrote:
> Ptal.
> 
> Still need to add unit tests, but would like some early feedback.
> 
> The first patch set is a straight copy of jbauman's patch from
> https://codereview.chromium.org/720333005/ to make diff'ing easier.

Hi Brian,

I have a slightly different approach to make this work, I think it scheduler
significantly more extendable and deals with the children problem directly. I'll
try and get a patch up today for you to have a look at and see if it makes
sense.

Tim

[brianderson, 2015-04-03 01:35:48]

Talked with Tim and I'm going to push the initial version of DisplayScheduler
through to hopefully get Surfaces unblocked on Android and then hand-off for Tim
to re-write and complete afterwards.

John, if you can provide some early feedback to my inline comments, even if you
can't get to the whole patch, it would be appreciated!

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:36: current_surface_has_resources_(false),
jbauman: Can you check my logic for how I update this variable? It seems to
work, but I'm worried about corner cases.

Is there a better way to let the DisplayScheduler know that draws will fail
because the Browser has locked its resources?

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:65: if (current_surface_id_ == id && device_scale_factor_
== device_scale_factor)
jbauman: Is this early return ok to avoid calling EntireDisplayDamaged()?

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:85: // TODO(mithro): Use hints from
SetNeedsBeginFrames.
mithro: I'm puting a TODO here for you, if you don't mind =)

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.h (right):

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.h:26: void SurfaceDamaged(SurfaceId surface_id);
I split the generic SurfaceDamaged() into EntireDisplayDamaged() and
SurfaceDamaged(id) so we can roughly track which surfaces are active and wait
for them (or not wait for them when the entire display is damaged).

[jbauman, 2015-04-03 22:27:51]

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:65: if (current_surface_id_ == id && device_scale_factor_
== device_scale_factor)
On 2015/04/03 01:35:48, brianderson wrote:
> jbauman: Is this early return ok to avoid calling EntireDisplayDamaged()?

In general this should be ok, as OnSurfaceDamaged should be called otherwise to
damage the Surface.

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:244: current_surface_has_resources_ = true;
This may not necessarily be true - we probably need to do the same check as
above.

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.h (right):

https://codereview.chromium.org/1012853003/diff/80001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.h:34: bool CanAggregate();
Doesn't seem to be implemented.

[brianderson, 2015-04-07 02:32:20]

ptal!

Unit tests added. Comments about design decisions inline.

https://codereview.chromium.org/1012853003/diff/100001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1012853003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:48: void
DisplayScheduler::SetAuthoritativeVSyncInterval(
Got rid of this for now. We can add it when it's actually needed.

https://codereview.chromium.org/1012853003/diff/100001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.h (right):

https://codereview.chromium.org/1012853003/diff/100001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.h:55: OutputSurface* output_surface_;
This wasn't used anywhere, so I got rid of it.

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:96:
base::STLSetIntersection<std::vector<SurfaceId>>(
Changed this to an intersection of the last two frames so we only count 60fps
sources as being active.

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:200: // TODO(brianderson): Figure out why
supressing DrawAndSwap while
jbauman: Any idea why suppressing DrawAndSwap while resources_locked_by_browser_
is true would cause a hang?

It's not really needed to make the scheduler work well, but it's a bit
unexpected.

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.h (right):

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.h:40: virtual void DidSwapBuffers();
Above tree made virtual for testing.

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.h:50: virtual void ScheduleBeginFrameDeadline();
Virtual for testing.

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.h:55: BeginFrameSource* begin_frame_source_;
Made the BeginFrameSource always be external. This simplified testing and made
it clear in setup code what frame source is being used.

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/onscreen_d...
File cc/surfaces/onscreen_display_client.h (right):

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/onscreen_d...
cc/surfaces/onscreen_display_client.h:56: scoped_ptr<SyntheticBeginFrameSource>
synthetic_begin_frame_source_;
I wanted to always give the scheduler an external BeginFrameSource, so I moved
ownership of the SyntheticBeginFrameSource here. Later patches can replace this
source with one tied to an OS hook if available.

jbauman: Does it make sense to move ownership here?

[jbauman, 2015-04-07 23:51:31]

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:200: // TODO(brianderson): Figure out why
supressing DrawAndSwap while
On 2015/04/07 02:32:19, brianderson wrote:
> jbauman: Any idea why suppressing DrawAndSwap while
resources_locked_by_browser_
> is true would cause a hang?
> 
> It's not really needed to make the scheduler work well, but it's a bit
> unexpected.

Hopefully because resources_locked_by_browser_ was inverted.

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/onscreen_d...
File cc/surfaces/onscreen_display_client.h (right):

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/onscreen_d...
cc/surfaces/onscreen_display_client.h:56: scoped_ptr<SyntheticBeginFrameSource>
synthetic_begin_frame_source_;
On 2015/04/07 02:32:19, brianderson wrote:
> I wanted to always give the scheduler an external BeginFrameSource, so I moved
> ownership of the SyntheticBeginFrameSource here. Later patches can replace
this
> source with one tied to an OS hook if available.
> 
> jbauman: Does it make sense to move ownership here?

Seems fine.

https://codereview.chromium.org/1012853003/diff/140001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1012853003/diff/140001/cc/surfaces/display.cc...
cc/surfaces/display.cc:130: resources_locked_by_browser = false;
I think this is wrong - this is the "resources_locked_by_browser = true" case -
as the resources are locked by the browser if the frame or resource list is
empty.

[brianderson, 2015-04-08 02:23:02]

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1012853003/diff/120001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:200: // TODO(brianderson): Figure out why
supressing DrawAndSwap while
On 2015/04/07 23:51:29, jbauman wrote:
> On 2015/04/07 02:32:19, brianderson wrote:
> > jbauman: Any idea why suppressing DrawAndSwap while
> resources_locked_by_browser_
> > is true would cause a hang?
> > 
> > It's not really needed to make the scheduler work well, but it's a bit
> > unexpected.
> 
> Hopefully because resources_locked_by_browser_ was inverted.

Magic! Thanks debugging my code. =)

I fixed resources_locked_by_browser_ and removed the TODOs.

https://codereview.chromium.org/1012853003/diff/140001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1012853003/diff/140001/cc/surfaces/display.cc...
cc/surfaces/display.cc:130: resources_locked_by_browser = false;
On 2015/04/07 23:51:30, jbauman wrote:
> I think this is wrong - this is the "resources_locked_by_browser = true" case
-
> as the resources are locked by the browser if the frame or resource list is
> empty.

Oops!

[jbauman, 2015-04-08 20:06:08]

lgtm with a few nits.

https://codereview.chromium.org/1012853003/diff/160001/cc/surfaces/onscreen_d...
File cc/surfaces/onscreen_display_client.cc (right):

https://codereview.chromium.org/1012853003/diff/160001/cc/surfaces/onscreen_d...
cc/surfaces/onscreen_display_client.cc:42: task_runner_,
display_->GetMaxFramesPending()));
This call to GetMaxFramesPending() may not return the correct value because the
display_ hasn't been given an OutputSurface to query yet.

https://codereview.chromium.org/1012853003/diff/160001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.h (right):

https://codereview.chromium.org/1012853003/diff/160001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.h:34: bool CanAggregate();
Should probably be removed.

[brianderson, 2015-04-11 00:43:44]

brianderson@chromium.org changed reviewers:
+ jamesr@chromium.org

[brianderson, 2015-04-11 00:43:45]

+James for owners review of mojo.

[jamesr, 2015-04-11 00:46:31]

jamesr@chromium.org changed reviewers:
+ sky@chromium.org
- jamesr@chromium.org

[jamesr, 2015-04-11 00:46:32]

lgtm, although I'm not an owner here.  Try sky

[brianderson, 2015-04-11 02:23:17]

Hmm. Looks like I need to resolve some things with a SurfacesScheduler that the
DisplayImpl uses and the DisplayScheduler in this patch to resolve the compile
errors. Will look at this next week.

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

https://codereview.chromium.org/1012853003/diff/220001/cc/surfaces/display.h#...
cc/surfaces/display.h:66: bool CurrentSurfaceHasResources();
Unused. Will delete.

[sky, 2015-04-13 15:35:07]

LGTM

[brianderson, 2015-04-13 21:29:59]

I opened https://code.google.com/p/chromium/issues/detail?id=476676 to track the
reconciliation of the existing SurfacesScheduler and this DisplayScheduler.

This patches uses the new DisplayScheduler for chromium and the existing
SurfacesScheduler for mojo.

[brianderson, 2015-04-13 22:56:34]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-04-13 22:56:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, jamesr@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1012853003/#ps280001
(title: "rebase; fix mojo compile")

[commit-bot: I haz the power, 2015-04-13 22:58:25]

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

[commit-bot: I haz the power, 2015-04-14 01:34:47]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-04-14 01:35:45]

Patchset 15 (id:??) landed as
https://crrev.com/893604464fd906112dd5ff3eec9cc99f6ca645fc
Cr-Commit-Position: refs/heads/master@{#324980}

[brianderson, 2015-04-14 05:41:29]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1068743003/ by brianderson@chromium.org.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=476544#c5.

[brianderson, 2015-05-01 01:02:23]

Sunny, John, and I looked at how the latest patch works with vsynctester on
Windows and noticed that it should change a bit to:
1) Not wait so long when the Browser goes idle. Our fixed deadline is too late
and we miss the vsync.
2) Expire "missed" BeginFrames.

[brianderson, 2015-05-01 19:04:25]

I was previously uploading to an incorrect patch.

Latest upload includes previous rebase and a few changes to the scheduling to
avoid some of the things we saw yesterday:
1) Skip "missed" DisplayScheduler::BeginFrames if they are already too late.
2) Don't wait for a previously active Browser for very long.
3) In the Renderer, look at the timing of the swap ack and determine if we
should skip a BeginImplFrame to reduce latency using a very conservative
heuristic. This can probably go into a separate patch.

[jbauman, 2015-05-02 01:20:04]

https://codereview.chromium.org/1012853003/diff/340001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1012853003/diff/340001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:73: browser_damaged_ = true;
Could this cause all_active_surfaces_ready_to_draw_ to be stale (incorrectly
false) if the browser is damaged after the renderer is damaged?

https://codereview.chromium.org/1012853003/diff/340001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:190:
BeginFrameArgs::DefaultEstimatedParentDrawTime();
Would all_active_surfaces_ready_to_draw_ be true if we're waiting on damage from
the browser?

Also, is there any theoretical justification for using
DefaultEstimatedParentDrawTime here? We already subtracted that (to account for
the time to draw this surface) in OnBeginFrameMixInDelegate.

[brianderson, 2015-05-02 03:28:37]

I changed all "Browser" references to "root surface" based on John's
recommendation.

Need to fix lots of unit tests and add a few.

https://codereview.chromium.org/1012853003/diff/340001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/1012853003/diff/340001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:73: browser_damaged_ = true;
On 2015/05/02 01:20:03, jbauman wrote:
> Could this cause all_active_surfaces_ready_to_draw_ to be stale (incorrectly
> false) if the browser is damaged after the renderer is damaged?

I will add a unit test to verify, but it should be ok. I tried to avoid that
situation by not keeping track of the browser_surface_id_ in the
surface_ids_damaged_.

The all_active_surfaces_ready_to_draw_ name is deceiving. I will give it a
better name.

https://codereview.chromium.org/1012853003/diff/340001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:190:
BeginFrameArgs::DefaultEstimatedParentDrawTime();
On 2015/05/02 01:20:03, jbauman wrote:
> Would all_active_surfaces_ready_to_draw_ be true if we're waiting on damage
from
> the browser?

As coded, it will. I need to rename all_active_surfaces_ready_to_draw_.

> 
> Also, is there any theoretical justification for using
> DefaultEstimatedParentDrawTime here? We already subtracted that (to account
for
> the time to draw this surface) in OnBeginFrameMixInDelegate.

I need to verify that this solution is actually helpful in practice. It's
intended to solve the problem where the Browser goes inactive and we wait for it
too long and miss the vsync. We saw that this was a problem on Sunny's Windows
machine yesterday.
 
Theoretically, the justification is that Surfaces and the UI run on the same
thread and run serially. If we give Surfaces 1/3 of the vsync and the UI 1/3 of
the vsync, the UI can have up till 1/3 of the vsync in to *start* producing
(which is what this deadline represents). If the UI starts to draw before this
point, it will lock the resources and we'll wait for it to unlock before we
draw.

I will update the comments with better justification.

It would be nice to replace these constants with better estimates and replace
these heuristics with better coordination, but I'm hoping we can leave that to
followup patches.

[brianderson, 2015-05-08 20:54:21]

Moving code review for relanding to: https://codereview.chromium.org/1138563002