Read compositor VSync information from platform, when possible

ui/compositor/compositor_vsync_manager.cc

Index: ui/compositor/compositor_vsync_manager.cc
diff --git a/ui/compositor/compositor_vsync_manager.cc b/ui/compositor/compositor_vsync_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..33436ef42a13c768082d22d343cdfe0d9be23c57
--- /dev/null
+++ b/ui/compositor/compositor_vsync_manager.cc
@@ -0,0 +1,57 @@
+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/compositor/compositor_vsync_manager.h"
+
+// These constants define a reasonable range for a calculated refresh interval.
+// Calculating refreshes out of this range will be considered a fatal error.
+const int64 kMinVsyncIntervalUs = base::Time::kMicrosecondsPerSecond / 400;
+const int64 kMaxVsyncIntervalUs = base::Time::kMicrosecondsPerSecond / 10;
+
+namespace ui {
+
+CompositorVSyncManager::CompositorVSyncManager()
+    : observer_list_(new ObserverListThreadSafe<Observer>()),
+      authoritative_vsync_interval_(base::TimeDelta::FromSeconds(0)) {}
+
+CompositorVSyncManager::~CompositorVSyncManager() {}
+
+void CompositorVSyncManager::SetAuthoritativeVSyncInterval(
+    base::TimeDelta interval) {
+  base::AutoLock lock(vsync_parameters_lock_);
+  authoritative_vsync_interval_ = interval;

[piman, 2014/02/04 04:51:01]

nit: I think we should update the observers at this point too.
We can keep the timebase when UpdateVSyncParameters was called.

The risk is that a very bogus interval might get through from the VSyncProvider,
which would then essentially prevent the compositor from swapping again (since
the deadline would be far into the future), which means we wouldn't call
UpdateVSyncParameters again to update the interval (since that's triggered by
the swap).

[sheu, 2014/02/04 22:01:45]

Sure, we'll do that.

+}
+
+void CompositorVSyncManager::UpdateVSyncParameters(base::TimeTicks timebase,
+                                                   base::TimeDelta interval) {
+  {
+    base::AutoLock lock(vsync_parameters_lock_);
+    if (authoritative_vsync_interval_ != base::TimeDelta::FromSeconds(0))
+      interval = authoritative_vsync_interval_;
+  }
+
+  // The vsync interval reported here controls the repaint rate of the
+  // compositor. If too low, kill the process instead of silently appearing to
+  // hang the UI.

[sheu, 2014/02/04 03:23:21]

This is probably not the best place to kill the process, since it nuke the
browser process at this point.  Doing this in FrameRateController would also
kill the browser process, since the browser process also has one.

Should I try:
1. Killing in CompositorOutputSurface (on the renderer side), and
2. Somehow terminating the GPU process in BrowserCompositorOutputSurface (on the
browser side)?

A bit of duplication of code there.  Suggestions welcome.

[piman, 2014/02/04 04:51:01]

If anything, the assert should go to SetAuthoritativeVSyncInterval, so that it
triggers when we have a chance to look at the stack, rather than too late.
Ideally this would actually go up to the source of the refresh interval.

We mainly introduced this assert in the GPU process to catch:
1- bogus drivers that send an invalid rate even though it returns success
2- bogus assumption in the code that estimates the rate with successive
timebase.

2 doesn't apply for the "authoritative vsync interval" that comes from RR, and
Stéphane tells me that if the RR info is bogus, there's probably a lot more
things going wrong.

[sheu, 2014/02/04 22:01:45]

I've moved the check so that it's run in both this and the
SetAuthoritativeVSyncInterval cases.  The question still remains though -- do we
want to kill the browser process if we hit this?  Previously it was just the GPU
process.

[piman, 2014/02/04 22:52:54]

I don't think we want to kill the browser process in response to a bogus GPU
message.
Previously, killing the GPU process would lose the contexts and would unblock
the draw loop in case the interval was off the charts. User would see a minor
glitch, but could otherwise continue. This would be a lot more dramatic,
possibly losing data.

+  if (interval.InMicroseconds() < kMinVsyncIntervalUs ||
+      interval.InMicroseconds() > kMaxVsyncIntervalUs) {
+    LOG(FATAL) << "Calculated bogus refresh interval of "
+               << interval.InMicroseconds() << " us.";
+  }
+
+  observer_list_->Notify(
+      &CompositorVSyncManager::Observer::OnUpdateVSyncParameters,
+      timebase,
+      interval);
+}
+
+void CompositorVSyncManager::AddObserver(Observer* observer) {
+  observer_list_->AddObserver(observer);

[piman, 2014/02/04 04:51:01]

It would be reasonable to Notify the observer here, and/or have a way for the
observer to query the current timebase/interval, so that the RWHVA can update
the renderer as it changes screens.

[sheu, 2014/02/04 22:01:45]

Done.

+}
+
+void CompositorVSyncManager::RemoveObserver(Observer* observer) {
+  observer_list_->RemoveObserver(observer);
+}
+
+}  // namespace ui