cc: Rename VSync to BeginFrame

cc: Rename VSync to BeginFrame

We are replacing hard vsync scheduling with BeginFrame+deadline
intervals.

This patch removes references to vsync in CC where it will no
longer make sense. (One exception is cc::VSyncTimeSource, which
will be removed in future patches to be incorporated into the
scheduler itself.)

Additionally, BeginFrames are clarified with suffixes whenever
context is not enough and existing identifiers associated with
BeginFrame when it no longer makes sense are renamed.

A summary of the important renames are as follows:

Browser Side Renames:
COutputSurface::EnableVSyncNotification -> SetNeedsBeginFrame
COutputSurface::OnDidVSync -> OnBeginFrame
ViewHostMsg_SetVSyncNotificationEnabled -> ViewHostMsg_SetNeedsBeginFrame
ViewMsg_DidVSync -> ViewMsg_BeginFrame

Impl Side Renames:
LTHI::EnableVSyncNotification -> SetNeedsBeginFrame
LTHI::DidVSync -> BeginFrame
LTHI::DidRecieveLastInputEventForVSync -> DidReceiveLastInputEventForBeginFrame

Main+Impl Side Renames:
LTHIClient::DidVSync -> BeginFrameOnImplThread
LTHIClient::DidRecieveLastInputEventForVSync -> DidReceiveLastInputEventForBeginFrameOnImplThread
TProxy::BeginFrame -> BeginFrameOnMainThread
TProxy::ForceBeginFrameOnImplThread -> ForceCommitOnImplThread
TProxy::BeginFrameCompleteOnImplThread -> StartCommitOnImplThread
Scheduler::BeginFrameComplete -> FinishCommit
Scheduler::BeginFrameAborted -> BeginFrameAbortedByMainThread
Scheduler::LastVsyncTime -> LastBeginFrameOnImplThreadTime
Scheduler::VSyncTick -> BeginFrame
SchedulerSM::VSyncCallbackNeeded -> BeginFrameNeededByImplThread
SchedulerSM::ACTION_BEGIN_FRAME -> ACTION_SEND_BEGIN_FRAME_TO_MAIN_THREAD
FRC::DidBeginFrame -> DidSwapBuffers
FRC::DidFinishFrame -> DidSwapBuffersComplete

Random Renames:
LTSettings::render_vsync_enabled -> throttle_frame_production
LTSettings::render_vsync_notification_enabled -> render_parent_drives_begin_frame
LTSettings::synchronously_disable_vsync -> using_synchronous_renderer_compositor
RenderWidget::SynchronouslyDisableVSync -> UsingSynchronousRendererCompositor

BUG=240945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201739

[Sami, 2013-05-15 12:11:20]

Thanks Brian, I like where this is going.

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

https://codereview.chromium.org/15058004/diff/11001/cc/output/output_surface....
cc/output/output_surface.h:102: virtual void EnableBeginFrameNotification(bool
enable) {}
Should we rename this to SetNeedsBeginFrame (here and elsewhere) to match the
design even though the semantics are still a little different (BeginFrame isn't
necessarily sent periodically).

[danakj, 2013-05-15 13:32:29]

I find it a little weird we'll end up with 2 "BeginFrame"s that are very
different. What do you think of BeginImplFrame? or something to set it apart
from BeginFrame that is part of the commit flow?

[brianderson, 2013-05-15 18:01:00]

On 2013/05/15 13:32:29, danakj wrote:
> I find it a little weird we'll end up with 2 "BeginFrame"s that are very
> different. What do you think of BeginImplFrame? or something to set it apart
> from BeginFrame that is part of the commit flow?

There are more similarities than differences, so I wanted to use the same name.
It does get confusing though, especially in thread_proxy.cc which has to deal
with both types of BeginFrames.

How about differentiating BeginFrames if they are used in the same file, like in
thread_proxy.cc - but allow context to differentiate them otherwise. There will
probably be other places that need to deal with two types of BeginFrames as well
and naming will be relative; BeginImplFrame in one context might be a
"BeginChildFrame" in another context.

[brianderson, 2013-05-15 18:03:30]

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

https://codereview.chromium.org/15058004/diff/11001/cc/output/output_surface....
cc/output/output_surface.h:102: virtual void EnableBeginFrameNotification(bool
enable) {}
On 2013/05/15 12:11:20, Sami wrote:
> Should we rename this to SetNeedsBeginFrame (here and elsewhere) to match the
> design even though the semantics are still a little different (BeginFrame
isn't
> necessarily sent periodically).

Consistency is good, so I will change this to SetNeedsBeginFrame.

[brianderson, 2013-05-16 20:56:46]

ptal.

Enough places had to deal with different BeginFrames that making it explicit
everywhere made sense. See updated summary for details.

[brianderson, 2013-05-16 21:19:19]

Adding folks for OWNERS reviews.
This patch touches a lot of files, but is just a naming change.

+sievers for:
content/browser/android/content_view_core_impl.cc
content/browser/renderer_host/compositor_impl_android.h
content/browser/renderer_host/render_widget_host_view_android.cc
content/browser/renderer_host/render_widget_host_view_android.h

+palmer for:
content/common/view_messages.h

+yfriedman for:
content/renderer/android/synchronous_compositor_output_surface.cc

+kbr for:
content/renderer/gpu/compositor_output_surface.cc
content/renderer/gpu/compositor_output_surface.h
content/renderer/gpu/render_widget_compositor.cc
content/renderer/gpu/render_widget_compositor.h

+jamesr for:
content/renderer/render_widget.cc
content/renderer/render_widget.h

+vollick for:
ui/compositor/compositor.h

[jamesr, 2013-05-16 21:21:27]

On 2013/05/16 21:19:19, Brian Anderson wrote:
> Adding folks for OWNERS reviews.
> This patch touches a lot of files, but is just a naming change.
> 
> +sievers for:
> content/browser/android/content_view_core_impl.cc
> content/browser/renderer_host/compositor_impl_android.h
> content/browser/renderer_host/render_widget_host_view_android.cc
> content/browser/renderer_host/render_widget_host_view_android.h
> 
> +palmer for:
> content/common/view_messages.h
> 
> +yfriedman for:
> content/renderer/android/synchronous_compositor_output_surface.cc
> 
> +kbr for:
> content/renderer/gpu/compositor_output_surface.cc
> content/renderer/gpu/compositor_output_surface.h
> content/renderer/gpu/render_widget_compositor.cc
> content/renderer/gpu/render_widget_compositor.h
> 
> +jamesr for:
> content/renderer/render_widget.cc
> content/renderer/render_widget.h

OWNERS inherit - I can do all the content/renderer/** changes (although other
folks are welcome to look as well if they like, of course).

> 
> +vollick for:
> ui/compositor/compositor.h

[jamesr, 2013-05-16 21:23:39]

https://codereview.chromium.org/15058004/diff/32001/content/renderer/gpu/rend...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/15058004/diff/32001/content/renderer/gpu/rend...
content/renderer/gpu/render_widget_compositor.cc:509:
widget_->willBeginCompositorFrame();
WillBeginMainFrame() is now inconsistent with the devtools instrumentation. 
What's the plan for fixing that?

[palmer, 2013-05-16 22:00:25]

LGTM rubberstamp for content/common/view_messages.h

[Yaron, 2013-05-16 22:43:45]

content/renderer/android lgtm

[jamesr, 2013-05-17 00:27:51]

On 2013/05/15 18:01:00, Brian Anderson wrote:
> On 2013/05/15 13:32:29, danakj wrote:
> > I find it a little weird we'll end up with 2 "BeginFrame"s that are very
> > different. What do you think of BeginImplFrame? or something to set it apart
> > from BeginFrame that is part of the commit flow?
> 
> There are more similarities than differences, so I wanted to use the same
name.
> It does get confusing though, especially in thread_proxy.cc which has to deal
> with both types of BeginFrames.

Not sure I agree.  Thus far, BeginFrame has been solely a part of the compositor
<-> main thread synchronization process and not something directly related to
the frame scheduling (although it's definitely part of the equation).  I think
reusing the name is going to be more problematic than coming up with a new name
for this concept.


> 
> How about differentiating BeginFrames if they are used in the same file, like
in
> thread_proxy.cc - but allow context to differentiate them otherwise. There
will
> probably be other places that need to deal with two types of BeginFrames as
well
> and naming will be relative; BeginImplFrame in one context might be a
> "BeginChildFrame" in another context.

[no sievers, 2013-05-17 00:54:52]

On 2013/05/16 21:19:19, Brian Anderson wrote:
> Adding folks for OWNERS reviews.

> +sievers for:
> content/browser/android/content_view_core_impl.cc
> content/browser/renderer_host/compositor_impl_android.h
> content/browser/renderer_host/render_widget_host_view_android.cc
> content/browser/renderer_host/render_widget_host_view_android.h
> 

Nice, lgtm.

[brianderson, 2013-05-17 01:01:38]

On 2013/05/16 21:23:39, jamesr wrote:
>
https://codereview.chromium.org/15058004/diff/32001/content/renderer/gpu/rend...
> File content/renderer/gpu/render_widget_compositor.cc (right):
> 
>
https://codereview.chromium.org/15058004/diff/32001/content/renderer/gpu/rend...
> content/renderer/gpu/render_widget_compositor.cc:509:
> widget_->willBeginCompositorFrame();
> WillBeginMainFrame() is now inconsistent with the devtools instrumentation. 
> What's the plan for fixing that?

I started fixing it and then realized that the change would span multiple repos
(chromium and blink). I would like to defer that to a later patch, if it's still
needed.

[brianderson, 2013-05-17 03:30:49]

On 2013/05/17 00:27:51, jamesr wrote:
> On 2013/05/15 18:01:00, Brian Anderson wrote:
> > On 2013/05/15 13:32:29, danakj wrote:
> > > I find it a little weird we'll end up with 2 "BeginFrame"s that are very
> > > different. What do you think of BeginImplFrame? or something to set it
apart
> > > from BeginFrame that is part of the commit flow?
> > 
> > There are more similarities than differences, so I wanted to use the same
> name.
> > It does get confusing though, especially in thread_proxy.cc which has to
deal
> > with both types of BeginFrames.
> 
> Not sure I agree.  Thus far, BeginFrame has been solely a part of the
compositor
> <-> main thread synchronization process and not something directly related to
> the frame scheduling (although it's definitely part of the equation).  I think
> reusing the name is going to be more problematic than coming up with a new
name
> for this concept.
> 
> 

The eventual goal is to make BeginFrame directly related to scheduling.
Currently, it'll kick off as soon as the pending tree is free after a commit or
RAF is requested. As part of our scheduling changes though, we want to ensure
ordering of input events relative to the BeginFrame. To do so, the
"BeginImplFrame" will be used to trigger the current BeginFrame causing them to
be more directly related.

Here are the options I see for naming:
1) Rename BeginFrame to BeginMainFrame, add BeginImplFrame and BeginChildFrame.
2) Keep BeginFrame, use totally different names for Impl/Child frame requests.
3) Keep BeginFrame, add BeginImplFrame and BeginChildFrame.
4) Use BeginFrame everywhere.

I no longer consider 4 an option because overloading BeginFrame in different
contexts becomes very confusing.
I have reservations with 3 because it's not immediately clear how BeginFrame is
different than the other names.
2 would be a good option but, with upcoming changes, BeginFrame will have many
more similarities to the other names than they imply.
I really like 1, because it uses similar naming for similar concepts, while
still differentiating them explicitly.

[Sami, 2013-05-17 09:23:34]

On 2013/05/17 03:30:49, Brian Anderson wrote:
> 1) Rename BeginFrame to BeginMainFrame, add BeginImplFrame and
BeginChildFrame.

I like this idea too -- better be explicit about things that can be very easily
confused. In which context would be use child frame though? For the browser =>
renderer notification? Maybe something like BeginChildCompositorFrame would be
even clearer. It would also match the existing terminology about having a parent
compositor.

[danakj, 2013-05-17 13:35:31]

On Fri, May 17, 2013 at 5:23 AM, <skyostil@chromium.org> wrote:

> On 2013/05/17 03:30:49, Brian Anderson wrote:
>
>> 1) Rename BeginFrame to BeginMainFrame, add BeginImplFrame and
>>
> BeginChildFrame.
>
> I like this idea too -- better be explicit about things that can be very
> easily
> confused. In which context would be use child frame though? For the
> browser =>
> renderer notification? Maybe something like BeginChildCompositorFrame
> would be
> even clearer. It would also match the existing terminology about having a
> parent
> compositor.
>

Or maybe BeginChildCompositorMainFrame / BeginChildCompositorImplFrame if
that makes sense.

Personally I like (1) as well, though was hesitant to suggest it because
that "BeginFrame" name lives in a lot of places.


>
>
https://codereview.chromium.**org/15058004/<https://codereview.chromium.org/1...
>

[nduca, 2013-05-17 17:39:16]

I honestly think we're overthinking here. Inspector's BeginFrame is actually
goign to evolve over time to be exactly the same as our BeginFrame. The begin
frame message everywhere --- whether its the ipc between processes, the message
from impl thread to main thread, they're all going to mean : "you, thread, do
your work to make a frame and here's your deadline." Right now, inspector
doesn't understand the deadline. But when it does, itll have the same semantics
as everywhere: inspector will just record the begin frame & the deadline, and
could show more precisely when that deadline was missed.

TLDR, I propose we stay the course and intentionally have these names collide.
There is one begin frame message.

[Tom Hudson, 2013-05-20 08:22:23]

On 2013/05/17 17:39:16, nduca wrote:
> I honestly think we're overthinking here. Inspector's BeginFrame is actually
> goign to evolve over time to be exactly the same as our BeginFrame. The begin
> frame message everywhere --- whether its the ipc between processes, the
message
> from impl thread to main thread, they're all going to mean : "you, thread, do
> your work to make a frame and here's your deadline." Right now, inspector
> doesn't understand the deadline. But when it does, itll have the same
semantics
> as everywhere: inspector will just record the begin frame & the deadline, and
> could show more precisely when that deadline was missed.
> 
> TLDR, I propose we stay the course and intentionally have these names collide.
> There is one begin frame message.

Nat, I spend too much time down below Chrome to know for sure, but doesn't the
possibility that we're going to have BeginFrame everywhere argue for keeping
names distinct? When I look at a message send, I want to know as explicitly as
possible who's sending the message, who's receiving it, and what the meaning is.
(Or if Enne ever reads this, I'm also happy enough with what we had in
LightSpeed.) Given the Chromium message SendRequest() parameter set, having
information in the name helps make it really readable.

TLDR, "There is one begin frame message" says to me that there's a single sender
which is multicasting the message simultaneously to all interested receivers. If
that's not the case, it's important to know the difference between individual
begin-frame-type messages.

[Ken Russell (switch to Gerrit), 2013-05-20 17:54:28]

content/renderer/gpu LGTM

[enne (OOO), 2013-05-20 18:56:39]

On 2013/05/20 08:22:23, Tom Hudson wrote:

> Nat, I spend too much time down below Chrome to know for sure, but doesn't the
> possibility that we're going to have BeginFrame everywhere argue for keeping
> names distinct? When I look at a message send, I want to know as explicitly as
> possible who's sending the message, who's receiving it, and what the meaning
is.
> (Or if Enne ever reads this, I'm also happy enough with what we had in
> LightSpeed.) Given the Chromium message SendRequest() parameter set, having
> information in the name helps make it really readable.

The issue with LightSpeed's message passing system was that there was one
monolithic message loop and because every message was essentially anonymized
when it passed through, there wasn't a good way of figuring out where a message
came from either at compile time or at run time.

These BeginFrame messages have very clear senders and receivers that can be
determined statically.  There's just no way to confuse a BeginFrame that goes
through the ThreadProxy to different threads with a BeginChildFrame that gets
IPC'd between browser and renderer process.  I don't think it's the same problem
at all.

[nduca, 2013-05-20 19:03:50]

Your'e on the right track actually: in the end state we seek, there really is a
single sender of BeginFrame, and it just propagates out through the world by a
mix of ipc and PostTask.

The full flow, assuming the renderer has say JSGameBeginRunning, is this:

1. the browser compositor's output surface implementation. That will be the one
that receives vsync, and subject to swapbuffer throttling, sends a BeginFrame
message to the browser impl thread.

2. That browser impl thread will send a begin frame to the browser main thread.

3. The browser main thread will send a BeginFrame to each of its
RenderWidgetHosts on its screen.

4. Those will send a RequestBeginFrame to the RenderWidgetInputController. The
controllers will then confirm the request, and a BeginFrame IPC will be sent to
the render process.

5. The BeginFrame IPC will be picked up by the output surface input filter, and
sent to the renderer's impl thread. That will hit the compositor thread's
scheduler, which will see that a commit is needed, and post a BeginFrame to the
main thread.

6. The beginframe will run on the main thread and tell DevTools that a
BeginFrame was received.

Hope that helps understand my point of view. Basically, that its IPC vs
MessageTask is just an implementation detail --- in all cases, you're saying
"begin a frame, here's your deadline" to a subsystem system. That subsystem may
post more tasks on receipt of a message, as is the case with the browser main
thread, or it may just notate the begin of the frame, as is the case of the
devtools beginframe hook.

[nduca, 2013-05-20 19:05:38]

(lgtm by the way)

[brianderson, 2013-05-20 21:27:02]

jamesr, I think I'm just missing a thumbs up for content/renderer now. ptal.

> WillBeginMainFrame() is now inconsistent with the devtools instrumentation. 
> What's the plan for fixing that?

It appears the current plan will be to not fix this. The BeginFrame in the
inspector will just correspond to the BeginMainFrame version of BeginFrame. If
we expose the other BeginFrames to the inspector in the future, we can
differentiate them at that time.

[jamesr, 2013-05-20 21:30:56]

Leaking the "impl" nomenclature out of cc/ is pretty unfortunate - it's not a
great name for the compositor thread inside cc/ and even less so in other code. 
The main advantage of it inside cc/ is it's short and it is relatively
unambiguous since we use it fairly consistently to mean "things on the
compositor thread", but in chromium code in general there's no such convention. 
The rest of code just calls it the compositor thread.  BeginCompositorFrame
seems pretty reasonable to me.

lgtm with a better name

[nduca, 2013-05-20 21:31:52]

BeginCompositorFrame sounds nice

[brianderson, 2013-05-21 00:06:13]

Ok, here's the current plan:
* Do not refer to "Impl" outside of cc, use "Compositor" instead.
* Use Main/Impl/Compositor/Child/etc as suffixes (instead of infixes) to clarify
naming where needed. This will imply a stronger relationship between the
different BeginFrames.

[jamesr, 2013-05-21 00:08:33]

On 2013/05/21 00:06:13, Brian Anderson wrote:
> Ok, here's the current plan:
> * Do not refer to "Impl" outside of cc, use "Compositor" instead.
> * Use Main/Impl/Compositor/Child/etc as suffixes (instead of infixes) to
clarify
> naming where needed. This will imply a stronger relationship between the
> different BeginFrames.

sgtm!

[Ian Vollick, 2013-05-21 16:52:37]

On 2013/05/21 00:08:33, jamesr wrote:
> On 2013/05/21 00:06:13, Brian Anderson wrote:
> > Ok, here's the current plan:
> > * Do not refer to "Impl" outside of cc, use "Compositor" instead.
> > * Use Main/Impl/Compositor/Child/etc as suffixes (instead of infixes) to
> clarify
> > naming where needed. This will imply a stronger relationship between the
> > different BeginFrames.
> 
> sgtm!

ui/compositor rubber stamp lgtm.

[brianderson, 2013-05-21 22:59:22]

PTAL. To speed up the naming discussion without having to wade through the
entire patch, I have provided a summary of the major renames in the commit
description.

[nduca, 2013-05-21 23:48:23]

This is not what we discussed. Can you update me on why you decided against
using BeginFrame everywhere except in ThreadProxy?

[brianderson, 2013-05-22 00:32:30]

On 2013/05/21 23:48:23, nduca wrote:
> This is not what we discussed. Can you update me on why you decided against
> using BeginFrame everywhere except in ThreadProxy?

It is pretty close to what we discussed; the naming had to bleed out of
ThreadProxy a bit, but I tried to minimize that as much as possible.

Explanation:
BeginFrame only has suffixes in the following cases:
1) ThreadProxy itself.
2) The interfaces ThreadProxy implements relating to BeginFrame (LTHClient,
LTHIClient).
3) ThreadProxy's composition classes (Scheduler and SchedulerStateMachine).

In the above 3 cases, suffixes are appended to the BeginFrame identifiers to
disambiguate BeginFrames for the Main thread vs. BeginFrames for the Impl
thread.

I could remove the suffixes for the BeginFrames on LTHClient and LTHIClient, but
that would require adapter classes in ThreadProxy, which we discussed would be
more trouble than it's worth.

[nduca, 2013-05-22 00:50:47]

ok. How about not changing lthclient?

[brianderson, 2013-05-22 00:59:37]

On 2013/05/22 00:50:47, nduca wrote:
> ok. How about not changing lthclient?

I see! ThreadProxy doesn't implement LTHClient like I had assumed. There is no
need to change LTHClient then. Will fix.

[brianderson, 2013-05-22 01:37:01]

On 2013/05/22 00:59:37, Brian Anderson wrote:
> On 2013/05/22 00:50:47, nduca wrote:
> > ok. How about not changing lthclient?
> 
> I see! ThreadProxy doesn't implement LTHClient like I had assumed. There is no
> need to change LTHClient then. Will fix.

Done.

[nduca, 2013-05-22 01:37:56]

lgtm

[commit-bot: I haz the power, 2013-05-22 17:11:26]

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

[commit-bot: I haz the power, 2013-05-22 18:12:30]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-05-22 19:53:14]

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

[commit-bot: I haz the power, 2013-05-23 10:45:05]

Change committed as 201739