Adding support for VSyncProvider to the software drawing path

content/browser/aura/software_browser_compositor_output_surface.cc

Index: content/browser/aura/software_browser_compositor_output_surface.cc
diff --git a/content/browser/aura/software_browser_compositor_output_surface.cc b/content/browser/aura/software_browser_compositor_output_surface.cc
index 773e7df75bb6edc62e1ded4cf8fe07e71472e616..882828d1647cb6081cf88711255e39f0dc856e24 100644
--- a/content/browser/aura/software_browser_compositor_output_surface.cc
+++ b/content/browser/aura/software_browser_compositor_output_surface.cc
@@ -4,18 +4,79 @@
 
 #include "content/browser/aura/software_browser_compositor_output_surface.h"
 
+#include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/time/time.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/output/software_output_device.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "ui/events/latency_info.h"
+#include "ui/gl/vsync_provider.h"
 
 namespace content {
 
+class VSyncListener : public base::RefCountedThreadSafe<VSyncListener> {
+ public:
+  VSyncListener(SoftwareBrowserCompositorOutputSurface* surface,
+                scoped_refptr<base::MessageLoopProxy> compositor_message_loop,
+                base::WeakPtr<ui::Compositor> compositor);
+
+  // Called by the VSyncProvider when updating the time of the most recent
+  // screen refresh.
+  void VSyncCallback(const base::TimeTicks timebase,
+                     const base::TimeDelta interval);
+
+ private:
+  friend class base::RefCountedThreadSafe<VSyncListener>;
+
+  virtual ~VSyncListener();
+
+  SoftwareBrowserCompositorOutputSurface* surface_;
+  scoped_refptr<base::MessageLoopProxy> compositor_message_loop_;
+  base::WeakPtr<ui::Compositor> compositor_;
+
+  DISALLOW_COPY_AND_ASSIGN(VSyncListener);
+};

[piman, 2013/11/05 23:32:27]

So this class is really only there to forward the call to the OutputSurface and
the Compositor, is it really necessary?
The BrowserCompositorOutputSurface does it all directly in there, I imagined we
could do the same. Possibly, even better, share the code between the 2 version,
maybe by sharing a base class?

[dnicoara, 2013/11/06 18:11:47]

Yes, this would be similar to the BrowserCompositorOutputSurfaceProxy. I was
reluctant to inherit from BrowserCompositorOutputSurface since it has some GPU
specific code.

I've been reading the code a bit to better understand how the accelerated path
works. On the accelerated path the GPU process would send a message to update
the VSync parameters which would then propagate to the Compositor and
OutputSurface. On the software path there isn't any such process, so I'm trying
to make that call directly from SoftwareCompositorOutputSurface.

Based on your feedback, I think the best course is to refactor
BrowserCompositorOutputSurface a bit such that
SoftwareBrowserCompositorOutputSurface can inherit it. Then I can use the
output_surface_map_ in GpuProcessTransportFactory to keep track of the allocated
BCOS. Then the VSyncListener wouldn't be needed and SBCOS can update the VSync
parameters by directly calling BCOSProxy. This way, it would also solve the
following comments you had.

Does this sound reasonable?

Most of the refactoring looks easy. I'm thinking of leaving BCOS as a base class
and have a GpuBrowserCompositorOutputSurface to provide the GPU specific
implementation. Though I'm not sure if the Reflectors make sense in the software
path since they seem to imply an accelerated path.

Adding danakj@ for reviewers to get more insight, since she left a TODO on
adding the inheritance.

+
+VSyncListener::VSyncListener(
+    SoftwareBrowserCompositorOutputSurface* surface,
+    scoped_refptr<base::MessageLoopProxy> compositor_message_loop,
+    base::WeakPtr<ui::Compositor> compositor)
+    : surface_(surface),
+      compositor_message_loop_(compositor_message_loop),
+      compositor_(compositor) {
+}
+
+VSyncListener::~VSyncListener() {
+}
+
+void VSyncListener::VSyncCallback(
+    const base::TimeTicks timebase,
+    const base::TimeDelta interval) {

[piman, 2013/11/05 23:32:27]

So, this should be called on the compositor already - the VSyncProvider has to
leave on that thread, right? (otherwise we'd need locks I think). If that's the
case, VSyncListener can simply be RefCounted instead of RefCountedThreadSafe.

+  // Update the OutputSurface.
+  surface_->OnVSyncParametersChanged(timebase, interval);

[piman, 2013/11/05 23:32:27]

What guarantee do we have that surface_ is still valid? In particular,
VSyncListener is refcounted whereas SoftwareBrowserCompositorOutputSurface is
not, and I don't think there's anything obvious that guarantees the
VSyncProvider won't call the callback after
SoftwareBrowserCompositorOutputSurface is destroyed.

Maybe we need to bind a WeakPtr<VSyncListener> into the VsyncProvider callback,
instead of making it refcounted?

+  // Propagate the update to the compositor.
+  compositor_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&ui::Compositor::OnUpdateVSyncParameters,
+                 compositor_,
+                 timebase,
+                 interval));
+}
+
 SoftwareBrowserCompositorOutputSurface::SoftwareBrowserCompositorOutputSurface(
-    scoped_ptr<cc::SoftwareOutputDevice> software_device)
-    : cc::OutputSurface(software_device.Pass()) {}
+    scoped_ptr<cc::SoftwareOutputDevice> software_device,
+    base::MessageLoopProxy* compositor_message_loop,
+    base::WeakPtr<ui::Compositor> compositor)
+    : cc::OutputSurface(software_device.Pass()),
+      vsync_listener_(new VSyncListener(this,
+                                        compositor_message_loop,
+                                        compositor)) {
+}
+
+SoftwareBrowserCompositorOutputSurface::
+~SoftwareBrowserCompositorOutputSurface() {

[piman, 2013/11/05 23:32:27]

nit: this looks indented wrong. git cl format?

+}
 
 void SoftwareBrowserCompositorOutputSurface::SwapBuffers(
     cc::CompositorFrame* frame) {
@@ -28,6 +89,13 @@ void SoftwareBrowserCompositorOutputSurface::SwapBuffers(
       base::Bind(
           &RenderWidgetHostImpl::CompositorFrameDrawn,
           latency_info));
+
+  gfx::VSyncProvider* vsync_provider = software_device_->GetVSyncProvider();
+  if (vsync_provider) {
+    vsync_provider->GetVSyncParameters(base::Bind(
+        &VSyncListener::VSyncCallback,
+        vsync_listener_));
+  }
 }
 
 }  // namespace content