Implement vsync notification on Android

Implement vsync notification on Android

This patch implements a vsync notification from the browser to the
renderer on Android. The vsync signal provider keeps track of all
subscriptions to the signal and makes sure it is not enabled when not
needed.

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

[Sami, 2013-03-28 19:30:26]

Here's the vsync provider implementation for Android. PTAL.

[jamesr, 2013-03-28 19:37:28]

lgtm on idea (although i have no OWNERS)

[Sami, 2013-04-08 17:27:41]

Thanks, adding some owners:

Daniel, ptal at content/browser/android and content/browser/renderer_host.

Ted, how do you like the changes under content/public/android/?

[no sievers, 2013-04-08 17:58:06]

Looks fine, basically. But comes out a bit confusing semantically now with
UpdateVSyncParameters() and SendVSync(), and notifications being and off in the
two modes. Maybe we can combine that a bit somehow (also both notifications seem
to have the same first arg)?
Looks like the difference is sending a vsync every frame vs. sending it only as
deemed appropriate (for the timer). Maybe that detail mostly needs to be visible
in the vsync provider?

Also wondering, who will decide and call SetVSyncNotificationEnabled?

https://codereview.chromium.org/11959036/diff/5001/content/browser/android/co...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/11959036/diff/5001/content/browser/android/co...
content/browser/android/content_view_core_impl.cc:1203:
host->SendVSync(base::TimeTicks::FromInternalValue(frame_time_micros));
Hmm I can't find the definition for this function. Is that a separate patch?

[Sami, 2013-04-08 18:18:17]

On 2013/04/08 17:58:06, Daniel Sievers wrote:
> Looks fine, basically. But comes out a bit confusing semantically now with
> UpdateVSyncParameters() and SendVSync(), and notifications being and off in
the
> two modes. Maybe we can combine that a bit somehow (also both notifications
seem
> to have the same first arg)?

Agreed, it's a little confusing because of the two modes. Once the vsync
notification is working I think we can completely remove the timer-based code on
Android which should streamline things.

> Looks like the difference is sending a vsync every frame vs. sending it only
as
> deemed appropriate (for the timer). Maybe that detail mostly needs to be
visible
> in the vsync provider?

I also thought about reusing the existing UpdateVSyncParameters message, but I
think given the different semantics I think it's better to add a new message so
that people won't confuse the concepts. The main reason is that the renderer
needs to actively manage the state of the vsync notification to only keep it
enabled when necessary, and doing that needs more book keeping both in the
renderer and the browser.
 
> Also wondering, who will decide and call SetVSyncNotificationEnabled?

That'll be the renderer. See the patch below.

>
https://codereview.chromium.org/11959036/diff/5001/content/browser/android/co...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/11959036/diff/5001/content/browser/android/co...
> content/browser/android/content_view_core_impl.cc:1203:
> host->SendVSync(base::TimeTicks::FromInternalValue(frame_time_micros));
> Hmm I can't find the definition for this function. Is that a separate patch?

Right, that's from https://codereview.chromium.org/13068002 which is still
pending.

[no sievers, 2013-04-09 15:36:55]

Ok, sounds good then. What do you think about renaming SendVSync() to OnVSync()
to differentiate it a bit more from the other API (it'd express better that it
happens on every tick)? Maybe just in CVC and RWHV, and leaving RWH::SendVSync()
as is.

Similarly, SetVSyncNotificationEnabled -> EnableVSyncTIcks or something like
that? Don't feel strongly about it though....

[Sami, 2013-04-09 15:55:02]

On 2013/04/09 15:36:55, Daniel Sievers wrote:
> Ok, sounds good then. What do you think about renaming SendVSync() to
OnVSync()
> to differentiate it a bit more from the other API (it'd express better that it
> happens on every tick)? Maybe just in CVC and RWHV, and leaving
RWH::SendVSync()
> as is.

Yeah, that makes the distinction more obvious. Done.

> Similarly, SetVSyncNotificationEnabled -> EnableVSyncTIcks or something like
> that? Don't feel strongly about it though....

This one used to be called EnableVSyncNotification but I was asked to rename it
to SetVSyncNotificationEnabled :) I don't have a strong preference either way,
but I'm leaning toward being consistent and leaving it as is.

[no sievers, 2013-04-09 16:20:03]

lgtm.

Just to be annoying:

If each compositor enables/disables notifications separately from the renderer,
it makes sense that this is all per-view at that level.

I'm wondering though if it makes sense for this to be per ContentView in the
browser. In the theoretical case of multiple live tabs, this would mean we'd be
spinning multiple vsync monitors and sending ticks from each host to each view.

I guess it could be left to the embedder to optimize this to use a single vsync
provider for all tabs. Or it could be hidden in content/ in that it uses a
single monitor in the right way at that level.
Related question: If we had multiple live tabs, would vsync ticks be the right
mechanism to throttle them? Or should that be done differently, for example
through throttling swap acks?

[Sami, 2013-04-09 16:48:57]

On 2013/04/09 16:20:03, Daniel Sievers wrote:
> lgtm.
> 
> Just to be annoying:

I expect nothing less :)

> If each compositor enables/disables notifications separately from the
renderer,
> it makes sense that this is all per-view at that level.
> 
> I'm wondering though if it makes sense for this to be per ContentView in the
> browser. In the theoretical case of multiple live tabs, this would mean we'd
be
> spinning multiple vsync monitors and sending ticks from each host to each
view.
> 
> I guess it could be left to the embedder to optimize this to use a single
vsync
> provider for all tabs. Or it could be hidden in content/ in that it uses a
> single monitor in the right way at that level.

Since the vsync provider lives outside the ContentView, so with multiple tabs
it's still straightforward to have just one vsync monitor. Here's how I've
implemented it downstream: https://gerrit-int.chromium.org/#/c/30041/.

> Related question: If we had multiple live tabs, would vsync ticks be the right
> mechanism to throttle them? Or should that be done differently, for example
> through throttling swap acks?

Great question. I've thought about that too, but I'm not sure what the best way
is. The benefit of the vsync notification is that it's guaranteed to be
delivered at the right time w.r.t. input events. Throttling swap acks would work
too, but I'm not sure how we'd decide which tab to prefer and when we even need
to throttle.

[no sievers, 2013-04-09 17:12:42]

On 2013/04/09 16:48:57, Sami wrote:
> On 2013/04/09 16:20:03, Daniel Sievers wrote:
> > lgtm.
> > 
> > Just to be annoying:
> 
> I expect nothing less :)
> 
> > If each compositor enables/disables notifications separately from the
> renderer,
> > it makes sense that this is all per-view at that level.
> > 
> > I'm wondering though if it makes sense for this to be per ContentView in the
> > browser. In the theoretical case of multiple live tabs, this would mean we'd
> be
> > spinning multiple vsync monitors and sending ticks from each host to each
> view.
> > 
> > I guess it could be left to the embedder to optimize this to use a single
> vsync
> > provider for all tabs. Or it could be hidden in content/ in that it uses a
> > single monitor in the right way at that level.
> 
> Since the vsync provider lives outside the ContentView, so with multiple tabs
> it's still straightforward to have just one vsync monitor. Here's how I've
> implemented it downstream: https://gerrit-int.chromium.org/#/c/30041/.
> 
> > Related question: If we had multiple live tabs, would vsync ticks be the
right
> > mechanism to throttle them? Or should that be done differently, for example
> > through throttling swap acks?
> 
> Great question. I've thought about that too, but I'm not sure what the best
way
> is. The benefit of the vsync notification is that it's guaranteed to be
> delivered at the right time w.r.t. input events. Throttling swap acks would
work
> too, but I'm not sure how we'd decide which tab to prefer and when we even
need
> to throttle.

Sounds all totally reasonable then. 

Random sidenote: To have a bit more of the scheduling logic shared and live in
content/ rather than in the app would allow us to more easily do cross-platform
(and upstream vs. downstream) perf tests. We should look into moving some of
that stuff when we find time.

[Sami, 2013-04-09 17:50:54]

> Random sidenote: To have a bit more of the scheduling logic shared and live in
> content/ rather than in the app would allow us to more easily do
cross-platform
> (and upstream vs. downstream) perf tests. We should look into moving some of
> that stuff when we find time.

Yes, definitely. It's crazy that we have to implement essentially the same thing
many times over. Good point about the perf tests too.

[Sami, 2013-04-19 11:11:10]

Marcus, can I get your stamp of approval for content/public/android?

[bulach, 2013-04-19 11:28:49]

lgtm, one suggestion below, feel free to do separately if you prefer..

https://codereview.chromium.org/11959036/diff/20001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/11959036/diff/20001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:118:
mCurrentContentView.getContentViewCore().setVSyncProvider(this);
suggestion: I suppose the responsibilities here a bit mixed, that is, the
massive outer ContentViewRenderView implements ContentViewCore.VSyncProvider,
and yet the VSyncMonitor itself is a poor little anonymous class in 75... :)

I suppose it'd be clearer to just have one single inner, non-static class that
implements ContentViewCore.VSyncProvider and VSyncMonitor? then
mVSyncNotificationEnabled handling will be self-contained within that class..
wdyt?

[Sami, 2013-04-19 13:29:21]

On 2013/04/19 11:28:49, bulach wrote:
> suggestion: I suppose the responsibilities here a bit mixed, that is, the
> massive outer ContentViewRenderView implements ContentViewCore.VSyncProvider,
> and yet the VSyncMonitor itself is a poor little anonymous class in 75... :)
> 
> I suppose it'd be clearer to just have one single inner, non-static class that
> implements ContentViewCore.VSyncProvider and VSyncMonitor? then
> mVSyncNotificationEnabled handling will be self-contained within that class..
> wdyt?

Great point, spreading the code out like this is hard to follow. I've now
brought the code together into a new VSyncProvider inner class. PTAL.

[bulach, 2013-04-19 14:20:22]

lgtm, thanks!

[commit-bot: I haz the power, 2013-04-19 14:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11959036/24002

[commit-bot: I haz the power, 2013-04-19 14:39:25]

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=ios_rel_de...

[commit-bot: I haz the power, 2013-04-19 14:51:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11959036/24002

[commit-bot: I haz the power, 2013-04-19 16:05:37]

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

[Sami, 2013-04-19 17:56:07]

Thanks Marcus! Here's the refactoring you suggested to reduce coupling.

[bulach, 2013-04-19 18:11:51]

lgtm, thanks! one suggestion below, not sure if the name would make it any
better..

https://codereview.chromium.org/11959036/diff/26008/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/11959036/diff/26008/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:73:
private class VSyncProvider implements VSyncManager.Provider,
VSyncMonitor.Listener {
how about "VSyncBridge", since it's both a provider and a listener? also,
besides the @Override methods, the other ones could be package private..

[Sami, 2013-04-19 18:24:43]

On 2013/04/19 18:11:51, bulach wrote:
> lgtm, thanks! one suggestion below, not sure if the name would make it any
> better..
> 
>
https://codereview.chromium.org/11959036/diff/26008/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
> (right):
> 
>
https://codereview.chromium.org/11959036/diff/26008/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:73:
> private class VSyncProvider implements VSyncManager.Provider,
> VSyncMonitor.Listener {
> how about "VSyncBridge", since it's both a provider and a listener? also,
> besides the @Override methods, the other ones could be package private..

Thanks, done, although I went for VSyncAdapter.

[commit-bot: I haz the power, 2013-04-19 18:24:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11959036/23009

[commit-bot: I haz the power, 2013-04-19 20:04:35]

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

[commit-bot: I haz the power, 2013-04-22 09:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11959036/36001

[commit-bot: I haz the power, 2013-04-22 09:50:55]

Failed to apply patch for
content/browser/renderer_host/render_widget_host_view_android.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/renderer_host/render_widget_host_view_android.cc
  Hunk #1 FAILED at 120.
  Hunk #2 succeeded at 402 with fuzz 1 (offset 17 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/renderer_host/render_widget_host_view_android.cc.rej

Patch:       content/browser/renderer_host/render_widget_host_view_android.cc
Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc
b/content/browser/renderer_host/render_widget_host_view_android.cc
index
e247e3b7b9dcda1813adbbe7c0c1a4f9d9cc33a5..6379e145b7655cadb9478a795f9bff535422a43c
100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -120,6 +120,8 @@ bool RenderWidgetHostViewAndroid::OnMessageReceived(
     IPC_MESSAGE_HANDLER(ViewHostMsg_StartContentIntent, OnStartContentIntent)
     IPC_MESSAGE_HANDLER(ViewHostMsg_DidChangeBodyBackgroundColor,
                         OnDidChangeBodyBackgroundColor)
+    IPC_MESSAGE_HANDLER(ViewHostMsg_SetVSyncNotificationEnabled,
+                        OnSetVSyncNotificationEnabled)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -385,6 +387,15 @@ void
RenderWidgetHostViewAndroid::OnDidChangeBodyBackgroundColor(
     content_view_core_->OnBackgroundColorChanged(color);
 }
 
+void RenderWidgetHostViewAndroid::SendVSync(base::TimeTicks frame_time) {
+  host_->Send(new ViewMsg_DidVSync(host_->GetRoutingID(), frame_time));
+}
+
+void RenderWidgetHostViewAndroid::OnSetVSyncNotificationEnabled(bool enabled) {
+  if (content_view_core_)
+    content_view_core_->SetVSyncNotificationEnabled(enabled);
+}
+
 void RenderWidgetHostViewAndroid::OnStartContentIntent(
     const GURL& content_url) {
   if (content_view_core_)

[commit-bot: I haz the power, 2013-04-22 10:17:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11959036/24008

[commit-bot: I haz the power, 2013-04-22 11:03:36]

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

[commit-bot: I haz the power, 2013-04-22 11:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/11959036/24008

[commit-bot: I haz the power, 2013-04-22 12:41:11]

Change committed as 195491