cc: Emulate BeginFrame in OutputSurfaces that don't support it natively

cc: Emulate BeginFrame in OutputSurfaces that don't support it natively

This will allow us to avoid having two different code paths
in the Scheduler. It also allows us to more easily remove the
VSyncTimeSource and FrameRateController from the Scheduler.

This patch instantiates the FrameRateController inside of
OutputSurface for now, but the FrameRateController could be
removed in future patches.

BUG=245920
BUG=243497
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205750

[brianderson, 2013-06-01 04:17:01]

I still need to fix a ton of tests, but this is the direction I am headed.

Looking for feedback on the approach for now.

Note: There is also a bug where it only works with max_frames_pending = 1,
otherwise I get tons of checkerboarding.

[brianderson, 2013-06-01 04:30:29]

Added some comments for context to aid review.

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.cc#n...
cc/output/output_surface.cc:120: //if (pending_begin_frames_ >=
frame_rate_controller_->MaxFramesPending())
This should be uncommented and tested on Android.

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface_clie...
File cc/output/output_surface_client.h (left):

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface_clie...
cc/output/output_surface_client.h:19: virtual void
OnVSyncParametersChanged(base::TimeTicks timebase,
This will be communicated as part of the BeginFrame in the near future.

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/frame_rate_contr...
File cc/scheduler/frame_rate_controller.cc (right):

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/frame_rate_contr...
cc/scheduler/frame_rate_controller.cc:126: void
FrameRateController::WillSwapBuffers() {
This now corresponds to a BeginFrame rather than a Swap which is why I call it
WillSwapBuffers now.

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

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:123: }
This is way hacky, but will be hard to improve until we implement a message to
the BeginFrame provider that we need another BeginFrame without having drawn.

Without this, the texture uploader stops uploading because the
AnticipatedDrawTime becomes stale.

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:141: (needs_begin_frame !=
last_set_needs_begin_frame_)) {
I'll need someone working on the synchronous compositor to verify I transfered
the logic over correctly here.

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:149: // TODO(brianderson): Replace this duplicate swap
with a "SwapSkip".
I should move this into a function; it's duplicated code from
ProcessScheduledActions.

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

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:121: base::WeakPtrFactory<Scheduler> weak_factory_;
I'll probably move these into the StateMachine so it's aware of all state.

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler_state_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler_state_...
cc/scheduler/scheduler_state_machine.cc:335: // We should proactively request a
BeginFrame if a commit is pending.
I would get hangs without this.

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

https://codereview.chromium.org/15836005/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:1323: void
ThreadProxy::StartScrollbarAnimationOnImplThread() {
not sure why i moved this. will put it back.

https://codereview.chromium.org/15836005/diff/1/content/renderer/gpu/composit...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/15836005/diff/1/content/renderer/gpu/composit...
content/renderer/gpu/compositor_output_surface.cc:73: if (!hasClient())
client_ is a private member of the base class, which is why i had to add this
method.

[Sami, 2013-06-03 17:30:32]

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.cc#n...
cc/output/output_surface.cc:120: //if (pending_begin_frames_ >=
frame_rate_controller_->MaxFramesPending())
On 2013/06/01 04:30:29, Brian Anderson wrote:
> This should be uncommented and tested on Android.

Do you have an idea what we'd need in the browser to implement throttling -- and
where it'd go? Maybe it's simpler just to implement that so this code can go
away immediately.

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

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.h#ne...
cc/output/output_surface.h:128: // The FrameRateController is depricated.
Typo: deprecated.

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

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:123: }
On 2013/06/01 04:30:29, Brian Anderson wrote:
> This is way hacky, but will be hard to improve until we implement a message to
> the BeginFrame provider that we need another BeginFrame without having drawn.

Is this the only instance where we're requesting a BeginFrame without actually
wanting to render? If so, maybe we should consider using some other mechanism to
drive texture uploads so that BeginFrame can be used for one specific purpose.

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

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:124: int pending_begin_frames_;
Does it ever make sense to have more than one of these?

[brianderson, 2013-06-03 18:51:40]

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.cc
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.cc#n...
cc/output/output_surface.cc:120: //if (pending_begin_frames_ >=
frame_rate_controller_->MaxFramesPending())
On 2013/06/03 17:30:33, Sami wrote:
> On 2013/06/01 04:30:29, Brian Anderson wrote:
> > This should be uncommented and tested on Android.
> 
> Do you have an idea what we'd need in the browser to implement throttling --
and
> where it'd go? Maybe it's simpler just to implement that so this code can go
> away immediately.

Daniel is working on a patch to do that here:
https://codereview.chromium.org/16290002/

We could combine our patches or have his go in right after this one.

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

https://codereview.chromium.org/15836005/diff/1/cc/output/output_surface.h#ne...
cc/output/output_surface.h:128: // The FrameRateController is depricated.
On 2013/06/03 17:30:33, Sami wrote:
> Typo: deprecated.

Done.

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

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:123: }
On 2013/06/03 17:30:33, Sami wrote:
> On 2013/06/01 04:30:29, Brian Anderson wrote:
> > This is way hacky, but will be hard to improve until we implement a message
to
> > the BeginFrame provider that we need another BeginFrame without having
drawn.
> 
> Is this the only instance where we're requesting a BeginFrame without actually
> wanting to render? If so, maybe we should consider using some other mechanism
to
> drive texture uploads so that BeginFrame can be used for one specific purpose.

Later we'll want to request a new BeginFrame anytime the Main thread runs long
so that the Impl thread can get an updated deadline.

Thinking about this function some more though, I think I can implement it in a
less hacky way by updating the draw time in BeginFrameRetry and setting the draw
time to 0 when there isn't a pending draw.

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

https://codereview.chromium.org/15836005/diff/1/cc/scheduler/scheduler.h#newc...
cc/scheduler/scheduler.h:124: int pending_begin_frames_;
On 2013/06/03 17:30:33, Sami wrote:
> Does it ever make sense to have more than one of these?

Currently, it does not make sense: one BeginFrame will correspond to one Impl
frame. The compositor should be fast enough that we don't need to pipeline the
Impl thread.

When we get to phase II and potentially pipeline commit and raster, I think
being able to have 2 BeginFrames will help.

[brianderson, 2013-06-04 03:08:55]

The second patch fixes the checkerboarding issue. Somehow, my changes tweaked
the ManageTiles scheduling such that a ManageTiles wouldn't necessarily show up
between a commit and pending tree activation. This patch explicitly ManagesTiles
if necessary before attempting activation.

This version also adds OutputSurface::pending_swap_buffers_ to make frame
tracking more explicit.

It works on Linux now, but I need to test it on other platforms.  Also, I still
need to fix the tests.

[enne (OOO), 2013-06-04 03:23:10]

https://codereview.chromium.org/15836005/diff/11001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/15836005/diff/11001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:867: ManageTilesOnImplThread();
I don't know that ManageTiles should always be paired with activate pending tree
since it's an expensive call.  It definitely needs to be called post-commit, but
not necessarily on every activation call.

I also fixed the checkerboarding issue here (which exists prior to your patch)
doing something similar: https://codereview.chromium.org/15855008/

[nduca, 2013-06-04 04:53:59]

some thoughts

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/frame_rate_c...
File cc/scheduler/frame_rate_controller.h (right):

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/frame_rate_c...
cc/scheduler/frame_rate_controller.h:56: void DidAbortAllPendingFrames();
this makes me think the OutputSurface needs a DidAbortBeginFrame or something...
or at least, we should talk through how it behaves in context lost cases

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:117: while (anticipated_draw_time_ < now) {
i think delay based timesource has some crap in it to compute this without a
loop

iirw, its  floor((now - antipcated draw time) / interval_) * interval_  +
anticipated_draw_time_;

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:153: pending_begin_frames_++;
okay this bit i dont get. why would we ever have this?

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:230: SetupNextBeginFrameIfNeeded();
This feels like more data suggesting that the tasks should live in the output
surface impl [and maybe all the existing output surfaces should have a output
surface impl base class so we can have common code]

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:231:
client_->DidAnticipatedDrawTimeChange(AnticipatedDrawTime());
so, if you actually look at who needs this, its the resource updater. And,
whereas in the scheduler we have to think that this might change anytime, when
youre in the output surface, you know exactly when the output time changes. THis
is yet more data to me that suggests output surface should just call
BeginFrame(ts, draw deadline, etc) and then that calls to scheduler, and to
resource update controller.

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:58: Thread* thread) {
So, typically the only thing in cc that gets to post tasks is thread proxy... 
want to share some thoughts on this? This is sorta the reason why I was
suggested making the output surface impl do the task posting logic.

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:104: void BeginFrame(base::TimeTicks frame_time);
an explanation of what these mean would be good. I haven't quite figured them
out yet.

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:126: int incomplete_swaps_;
what are incomplete swaps?

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:128: base::TimeTicks anticipated_draw_time_;
this is the deadline, right? Calling it anticipated is a bit funky since it
implies to me a sort of yes or we might be later.

[brianderson, 2013-06-04 17:49:34]

On 2013/06/04 03:23:10, enne wrote:
> https://codereview.chromium.org/15836005/diff/11001/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/15836005/diff/11001/cc/trees/thread_proxy.cc#...
> cc/trees/thread_proxy.cc:867: ManageTilesOnImplThread();
> I don't know that ManageTiles should always be paired with activate pending
tree
> since it's an expensive call.  It definitely needs to be called post-commit,
but
> not necessarily on every activation call.

In this patch, ManageTiles only gets triggered before activation if
manage_tiles_pending_ is set.

> 
> I also fixed the checkerboarding issue here (which exists prior to your patch)
> doing something similar: https://codereview.chromium.org/15855008/

Oh, great! I had a hard time reproducing the checkerboarding at ToT, so I though
this patch caused it. I will remove my hacky fix from this patch then.

[brianderson, 2013-06-04 18:33:27]

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:117: while (anticipated_draw_time_ < now) {
On 2013/06/04 04:53:59, nduca wrote:
> i think delay based timesource has some crap in it to compute this without a
> loop
> 
> iirw, its  floor((now - antipcated draw time) / interval_) * interval_  +
> anticipated_draw_time_;

Will change this.

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:153: pending_begin_frames_++;
On 2013/06/04 04:53:59, nduca wrote:
> okay this bit i dont get. why would we ever have this?

In this patch, pending_begin_frames_ will only ever be 0 or 1. I kept it a
counter though, in case we ever wanted to have more than 1 BeginFrame pending
for pipelining reasons.

Using it as a counter now implies something that isn't true (that there can be
more than one BeginFrame pending), which is confusing, so I will change it to a
bool

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:230: SetupNextBeginFrameIfNeeded();
On 2013/06/04 04:53:59, nduca wrote:
> This feels like more data suggesting that the tasks should live in the output
> surface impl [and maybe all the existing output surfaces should have a output
> surface impl base class so we can have common code]

I will revisit this idea in future patches that add the deadline.

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:231:
client_->DidAnticipatedDrawTimeChange(AnticipatedDrawTime());
On 2013/06/04 04:53:59, nduca wrote:
> so, if you actually look at who needs this, its the resource updater. And,
> whereas in the scheduler we have to think that this might change anytime, when
> youre in the output surface, you know exactly when the output time changes.
THis
> is yet more data to me that suggests output surface should just call
> BeginFrame(ts, draw deadline, etc) and then that calls to scheduler, and to
> resource update controller.

This patch is already pretty big, so I opened another bug to fix this in a
future patch:
https://code.google.com/p/chromium/issues/detail?id=246714

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:58: Thread* thread) {
On 2013/06/04 04:53:59, nduca wrote:
> So, typically the only thing in cc that gets to post tasks is thread proxy... 
> want to share some thoughts on this? This is sorta the reason why I was
> suggested making the output surface impl do the task posting logic.

That sounds like a good restriction to have. I brought the Thread in because it
used to be used by the DelayBasedTimeSource, but that no longer exists here.

When we add deadlines in a future patch, we could have the OutputSurface do the
task posting logic (although the OutputSurface also doesn't have access to the
Thread). For now though, I think I will add a
SchedulerClient::RequestBeginFrameRetry so Scheduler doesn't need to know about
the Thread and task posting.

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:104: void BeginFrame(base::TimeTicks frame_time);
On 2013/06/04 04:53:59, nduca wrote:
> an explanation of what these mean would be good. I haven't quite figured them
> out yet.

Added the following comments:

  // BeginFrame gets called when we should produce a frame.
  // Each BeginFrame will have a corresponding swap.
  void BeginFrame(base::TimeTicks frame_time);

  // BeginFrame will not get called again until we swap.
  // We use BeginFrameRetry to retry a BeginFrame that hasn't yet swapped.
  // TODO(brianderson): Allow the scheduler to request a new BeginFrame
  // without swapping.
  void BeginFrameRetry();

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:126: int incomplete_swaps_;
On 2013/06/04 04:53:59, nduca wrote:
> what are incomplete swaps?

This should go away. I was using this to debug the checkerboarding I saw and
forgot to remove it.

https://codereview.chromium.org/15836005/diff/11001/cc/scheduler/scheduler.h#...
cc/scheduler/scheduler.h:128: base::TimeTicks anticipated_draw_time_;
On 2013/06/04 04:53:59, nduca wrote:
> this is the deadline, right? Calling it anticipated is a bit funky since it
> implies to me a sort of yes or we might be later.

The variable corresponds to the AnticipatedDrawTime member function. We haven't
added the concept of a deadline yet, so I would like to stay away from using
that naming for now. It is a kind of deadline though: the scheduler will try to
draw at that time, but it might not if not needed.

[brianderson, 2013-06-05 02:29:42]

This latest patch:
1) Addresses the comments.
2) Removes the ManageTiles workaround.
3) Gets rid of BeginFrameRetry and uses SetNeedsBeginFrame to request another
BeginFrame.
4) Depends on https://codereview.chromium.org/15651006 to get Android to work.

[brianderson, 2013-06-05 02:39:05]

Ignore the previous patch set, it was only a partial upload.

https://codereview.chromium.org/15836005/diff/32001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/15836005/diff/32001/cc/scheduler/scheduler.cc...
cc/scheduler/scheduler.cc:183: "state", state_machine_.ToString());
Need to remove this debug code.

[nduca, 2013-06-05 02:58:19]

lgtm for when you're ready

[brianderson, 2013-06-11 02:43:55]

+kbr for content/renderer/gpu owners review.
+sievers for android-specific owners review.

I'm not sure why my patch is failing to apply on the trybots, I don't have any
conflicts locally.

[no sievers, 2013-06-11 18:16:31]

On 2013/06/11 02:43:55, Brian Anderson wrote:
> +kbr for content/renderer/gpu owners review.
> +sievers for android-specific owners review.

lgtm

> 
> I'm not sure why my patch is failing to apply on the trybots, I don't have any
> conflicts locally.

[Ken Russell (switch to Gerrit), 2013-06-11 18:29:53]

content/renderer/gpu LGTM

[commit-bot: I haz the power, 2013-06-12 00:54:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/15836005/81001

[commit-bot: I haz the power, 2013-06-12 03:10:55]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-06-12 03:16:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/15836005/81001

[commit-bot: I haz the power, 2013-06-12 13:21:51]

Change committed as 205750