Add LayerTreeHostSettings for synchronous compositor

Add cc option to non-lazily disable vsync notification

The option will be used by Android WebView using synchronous compositor
which requires this behavior that conflicts with other platforms.

BUG=

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

[boliu, 2013-05-03 03:17:34]

Coding up one way to disable some compositor vsync optimizations that won't work
for webview. Please let me know if some other way is preferred.

Lazy disable of vsync notification doesn't work for webview because it would
lead to infinite invalidate/draw loop.

Input event triggering vsync won't work because hardware draw requires a gl
context. This behavior can be disabled in ContentViewCore though, if that's a
better place to make this change.

[jamesr, 2013-05-03 05:00:18]

If you don't want to treat input events as vsync, have the event-is-vsync flag
set to false for the input events.  I don't think you should have this setting
here.

[Sami, 2013-05-03 10:58:37]

On 2013/05/03 05:00:18, jamesr wrote:
> If you don't want to treat input events as vsync, have the event-is-vsync flag
> set to false for the input events.  I don't think you should have this setting
> here.

Right, you can do that over in ContentViewGestureHandler.java.

Maybe the option -- should we decide to have it -- could be called something
like enable_synchronous_compositing since this seems more like a new mode
instead tweaks to the existing one.

I'm also not crazy about the whole forced invalidation + vsync scheme, but I
don't have a good suggestion to make it better with the current architecture
other than tweaking VSyncTimeSource like you've done here. Maybe you could talk
to brianderson@ and nduca@ to see if we could expedite some of the BeginFrame
changes to make this better?

[boliu, 2013-05-03 16:39:14]

Removed input event vsync change; will do that in content in separate patch as
suggested. Renamed setting variable. Added a test for vsync time source.

I don't think we should wait for the begin frame changes though. There are some
pieces of webview changes we want to land very soon that depends on this:
https://codereview.chromium.org/14888002/

Talked to Grace yesterday and as you suggest here, if we could tweak the begin
frame message with deadline of now to draw synchronously, then the overall
scheduler change will be a win for webview. It's just that we don't want to wait
for that long.

[Sami, 2013-05-03 17:02:55]

Fair enough. Let's just make sure the BeginFrame changes will actually work for
you guys :)

https://codereview.chromium.org/14898002/diff/5001/cc/scheduler/vsync_time_so...
File cc/scheduler/vsync_time_source.h (right):

https://codereview.chromium.org/14898002/diff/5001/cc/scheduler/vsync_time_so...
cc/scheduler/vsync_time_source.h:37: VSyncProvider* vsync_provider, bool
lazy_disable);
Instead of a bool could you make this into an enum? That way it's a little more
obvious what the parameter does at the call site.

https://codereview.chromium.org/14898002/diff/5001/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/14898002/diff/5001/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:280:
settings.disable_optimizations_for_synchronous_compositor =
Forgot to rename this setting?

[boliu, 2013-05-03 17:20:39]

https://codereview.chromium.org/14898002/diff/5001/cc/scheduler/vsync_time_so...
File cc/scheduler/vsync_time_source.h (right):

https://codereview.chromium.org/14898002/diff/5001/cc/scheduler/vsync_time_so...
cc/scheduler/vsync_time_source.h:37: VSyncProvider* vsync_provider, bool
lazy_disable);
On 2013/05/03 17:02:55, Sami wrote:
> Instead of a bool could you make this into an enum? That way it's a little
more
> obvious what the parameter does at the call site.

Done.

https://codereview.chromium.org/14898002/diff/5001/content/renderer/gpu/rende...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/14898002/diff/5001/content/renderer/gpu/rende...
content/renderer/gpu/render_widget_compositor.cc:280:
settings.disable_optimizations_for_synchronous_compositor =
On 2013/05/03 17:02:55, Sami wrote:
> Forgot to rename this setting?

Oh darn! Only compiled cc_unittests to check. Fixed now!

[Sami, 2013-05-03 17:55:33]

https://codereview.chromium.org/14898002/diff/11001/cc/scheduler/vsync_time_s...
File cc/scheduler/vsync_time_source.cc (right):

https://codereview.chromium.org/14898002/diff/11001/cc/scheduler/vsync_time_s...
cc/scheduler/vsync_time_source.cc:7: #include "base/logging.h"
Not needed?

https://codereview.chromium.org/14898002/diff/11001/content/renderer/render_w...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/14898002/diff/11001/content/renderer/render_w...
content/renderer/render_widget.h:212: bool EnableSynchronousCompositing() const;
SynchronousCompositingEnabled() might be a better name for this since this is a
query instead of actual enable.

[boliu, 2013-05-03 17:59:39]

https://codereview.chromium.org/14898002/diff/11001/cc/scheduler/vsync_time_s...
File cc/scheduler/vsync_time_source.cc (right):

https://codereview.chromium.org/14898002/diff/11001/cc/scheduler/vsync_time_s...
cc/scheduler/vsync_time_source.cc:7: #include "base/logging.h"
On 2013/05/03 17:55:33, Sami wrote:
> Not needed?

For DCHECK in DidVSync.

https://codereview.chromium.org/14898002/diff/11001/content/renderer/render_w...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/14898002/diff/11001/content/renderer/render_w...
content/renderer/render_widget.h:212: bool EnableSynchronousCompositing() const;
On 2013/05/03 17:55:33, Sami wrote:
> SynchronousCompositingEnabled() might be a better name for this since this is
a
> query instead of actual enable.

Done.

[Sami, 2013-05-03 18:02:55]

Thanks, lgtm.

[boliu, 2013-05-03 21:04:06]

James: Friendly ping!

[jamesr, 2013-05-03 21:07:16]

I think having a general "this is a different mode" setting for crap to be piled
on to is a bad direction to go in, since the tendency will be for it to
accumulate untested cruft all over the place.  What specifically do you expect
to use this for?  If it's just to tweak the vsync provider, can we do something
more targetted?

https://codereview.chromium.org/14898002/diff/16001/cc/scheduler/vsync_time_s...
File cc/scheduler/vsync_time_source.cc (left):

https://codereview.chromium.org/14898002/diff/16001/cc/scheduler/vsync_time_s...
cc/scheduler/vsync_time_source.cc:30: // The notification will be lazily
disabled in the callback to ensure
why did you remove this comment?

https://codereview.chromium.org/14898002/diff/16001/cc/scheduler/vsync_time_s...
File cc/scheduler/vsync_time_source.h (right):

https://codereview.chromium.org/14898002/diff/16001/cc/scheduler/vsync_time_s...
cc/scheduler/vsync_time_source.h:60: explicit VSyncTimeSource(VSyncProvider*
vsync_provider,
you don't need explicit for 2-arg c'tors

[boliu, 2013-05-03 21:30:16]

On 2013/05/03 21:07:16, jamesr wrote:
> I think having a general "this is a different mode" setting for crap to be
piled
> on to is a bad direction to go in, since the tendency will be for it to
> accumulate untested cruft all over the place.  What specifically do you expect
> to use this for?  If it's just to tweak the vsync provider, can we do
something
> more targetted?

Webview specific things like don't try to recreate the output surface on context
loss, instead fall back to software mode until the next gl init.

But again, this is just me thinking too far ahead. Renamed to
synchronously_disable_vsync, and put it next to the other vsync settings, so it
won't be forgotten when all vsync logic is removed.

https://codereview.chromium.org/14898002/diff/16001/cc/scheduler/vsync_time_s...
File cc/scheduler/vsync_time_source.cc (left):

https://codereview.chromium.org/14898002/diff/16001/cc/scheduler/vsync_time_s...
cc/scheduler/vsync_time_source.cc:30: // The notification will be lazily
disabled in the callback to ensure
On 2013/05/03 21:07:16, jamesr wrote:
> why did you remove this comment?

Moved to header file where the enum is declared.

[jamesr, 2013-05-06 16:59:29]

lgtm

[boliu, 2013-05-06 17:14:30]

Thanks James. Updated the description too.

[commit-bot: I haz the power, 2013-05-06 17:15:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/14898002/26001

[commit-bot: I haz the power, 2013-05-06 19:43:15]

Change committed as 198510