Decouple BrowserCompositorOutputSurface from BeginFrameSource.

content/browser/compositor/gpu_process_transport_factory.cc

Index: content/browser/compositor/gpu_process_transport_factory.cc
diff --git a/content/browser/compositor/gpu_process_transport_factory.cc b/content/browser/compositor/gpu_process_transport_factory.cc
index 7d1cf0c2d283a6620d1de210bc8fc8348d66ee4d..33383a84812f9fead30efb7101b65571d4e4d254 100644
--- a/content/browser/compositor/gpu_process_transport_factory.cc
+++ b/content/browser/compositor/gpu_process_transport_factory.cc
@@ -283,6 +283,11 @@ void GpuProcessTransportFactory::CreateCompositorFrameSink(
   if (!data) {
     data = CreatePerCompositorData(compositor.get());
   } else {
+    // TODO: is this the right place to remove data->begin_frame_source from
+    // the compositor VSync manager?
+    if (data->begin_frame_source)
+      compositor->vsync_manager()->RemoveObserver(data->begin_frame_source);
+
     // TODO(danakj): We can destroy the |data->display| here when the compositor
     // destroys its CompositorFrameSink before calling back here.
     data->display_output_surface = nullptr;
@@ -447,13 +452,14 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
             compositor->task_runner().get())));
   }
 
+  compositor->vsync_manager()->AddObserver(begin_frame_source.get());

[danakj, 2016/11/21 22:32:11]

I think there are at least 3 ways to do this:

1. What you did here exactly. Has a bunch of manual book-keeping about
add/remove observer in this class that's easy to mess up.
2. Make Display take a SyntheticBeginFrameSource instead of a BeginFrameSource,
have it Add/Remove observer.  I think Display take the base-class interface
right now because we used to sometimes give it a non-synthethic subclass, but we
don't now as BackToBack is synthetic too. Maybe something else would break I'm
forgetting?
3. Make a VsyncInformer type interface in cc that SyntheticBFS takes, it uses
that to register itself as an observer. CompositorVsyncManager is-a
VsyncInformer then.

The last 2 have easier to maintain lifetimes around observer-ness. enne@ any
preferences or do you prefer 1?

[stanisc, 2016/11/21 22:52:31]

Thanks for the suggestion! The idea was that a new BeginFrameSource that
receives actual VSync signals from GPU process won't be based on
SyntheticBeginFrameSource. So the suggestion #2 might not work.

The approach #3 sounds good - let BeginFrameSource to subscribe/unsubscribe
itself from VSyncManager. I agree it would be much cleaner. What name for the
interface would you prefer - VSyncInformer, VSyncSource, VSyncManager?

[danakj, 2016/11/21 23:06:36]

Ok so one problem with this approach is that we're going to have to undo it when
we move Display out of process.

Right now VsyncManager is part of ui::Compositor and used to inform things in
the browser process. This is making it also inform the display's BFS which will
move to the GPU process. So we're taking backward steps toward an out-of-process
Display compositor I think.

At a high level I think what we need is to get rid of both the VSyncManager* and
the SyntheticBFS* from the BrowserCompositorOutputSurface. We should delegate
the receipt of vsync parameters up to a higher level that is going to stay in
the browser process and have it tell the VsyncManager. That place can also tell
the SyntheticBFS* when it is present, later we'd just delete that and tell the
display's SyntheticBFS on the gpu process side before sending that IPC to the
browser.

The most obvious way to do that is to give a client* or a Callback to the
BrowserCompositorOutputSurface that it can call. Make that function a part of
GpuProcessTransportFactory, and have it receive the Compositor* as part of its
inputs to find the per-compositor-data with the VsyncManager and SyntheticBFS*
to update.

But I do wonder if GPTF is the right place for this at all. Most of the calls to
these things are coming out of ui::Compositor (except for the one from the
GPUProcess which is looking more like an outlier to me at this point). So this
makes me wonder if we shouldn't instead be listening to vsync info from the
ui::Compositor instead and it could pass that info along the same path that it
is for Mac currently
(https://cs.chromium.org/chromium/src/ui/compositor/compositor.h?rcl=147974512...).
And we'd do the same for software compositing..

I'm not totally sure if that's a feasible idea but that seems to be most aligned
with where things are going - in that world ui::Compositor would be telling the
display compositor out-of-process about vsync stuff on some platforms, and
there's no reason to involve GPTF in that anymore at that point.

[stanisc, 2016/11/22 19:55:43]

How about passing a pointer to VSyncObserver to BrowserCompositorOutputSurface
instead of CompositorVSyncManager. In this case it might be better to rename 
VSyncObserver to something like VSyncClient. An object that implements
VSyncClient
might be owned by PerCompositorData and it would replace a pointer to
SyntheticBFS there.
The responsibility of this object will be to pass VSync notification to other
objects -
BFS and VSync Manager. VSyncClient could also be passed to a new type of BFS
that I am
going to introduce later to allow it to control VSync production on the GPU
side.

+
   std::unique_ptr<BrowserCompositorOutputSurface> display_output_surface;
 #if defined(ENABLE_VULKAN)
   std::unique_ptr<VulkanBrowserCompositorOutputSurface> vulkan_surface;
   if (vulkan_context_provider) {
     vulkan_surface.reset(new VulkanBrowserCompositorOutputSurface(
-        vulkan_context_provider, compositor->vsync_manager(),
-        begin_frame_source.get()));
+        vulkan_context_provider, compositor->vsync_manager()));
     if (!vulkan_surface->Initialize(compositor.get()->widget())) {
       vulkan_surface->Destroy();
       vulkan_surface.reset();
@@ -468,8 +474,7 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
       display_output_surface =
           base::MakeUnique<SoftwareBrowserCompositorOutputSurface>(
               CreateSoftwareOutputDevice(compositor.get()),
-              compositor->vsync_manager(), begin_frame_source.get(),
-              compositor->task_runner());
+              compositor->vsync_manager(), compositor->task_runner());
     } else {
       DCHECK(context_provider);
       const auto& capabilities = context_provider->ContextCapabilities();
@@ -477,21 +482,20 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
         display_output_surface =
             base::MakeUnique<OffscreenBrowserCompositorOutputSurface>(
                 context_provider, compositor->vsync_manager(),
-                begin_frame_source.get(),
                 std::unique_ptr<
                     display_compositor::CompositorOverlayCandidateValidator>());
       } else if (capabilities.surfaceless) {
 #if defined(OS_MACOSX)
         display_output_surface = base::MakeUnique<GpuOutputSurfaceMac>(
             compositor->widget(), context_provider, data->surface_handle,
-            compositor->vsync_manager(), begin_frame_source.get(),
+            compositor->vsync_manager(),
             CreateOverlayCandidateValidator(compositor->widget()),
             GetGpuMemoryBufferManager());
 #else
         display_output_surface =
             base::MakeUnique<GpuSurfacelessBrowserCompositorOutputSurface>(
                 context_provider, data->surface_handle,
-                compositor->vsync_manager(), begin_frame_source.get(),
+                compositor->vsync_manager(),
                 CreateOverlayCandidateValidator(compositor->widget()),
                 GL_TEXTURE_2D, GL_RGB, ui::DisplaySnapshot::PrimaryFormat(),
                 GetGpuMemoryBufferManager());
@@ -509,7 +513,7 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
           display_output_surface =
               base::MakeUnique<GpuBrowserCompositorOutputSurface>(
                   context_provider, compositor->vsync_manager(),
-                  begin_frame_source.get(), std::move(validator));
+                  std::move(validator));
         } else {
 #if defined(USE_AURA)
           if (compositor->window()) {
@@ -519,7 +523,7 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
                 base::MakeUnique<MusBrowserCompositorOutputSurface>(
                     compositor->window(), context_provider,
                     GetGpuMemoryBufferManager(), compositor->vsync_manager(),
-                    begin_frame_source.get(), std::move(validator));
+                    std::move(validator));
           } else {
             aura::WindowTreeHost* host =
                 aura::WindowTreeHost::GetForAcceleratedWidget(
@@ -528,7 +532,7 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
                 base::MakeUnique<MusBrowserCompositorOutputSurface>(
                     host->window(), context_provider,
                     GetGpuMemoryBufferManager(), compositor->vsync_manager(),
-                    begin_frame_source.get(), std::move(validator));
+                    std::move(validator));
           }
 #else
           NOTREACHED();
@@ -539,6 +543,7 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
   }
 
   data->display_output_surface = display_output_surface.get();
+  // TODO: is this still needed?
   data->begin_frame_source = begin_frame_source.get();
   if (data->reflector)
     data->reflector->OnSourceSurfaceReady(data->display_output_surface);
@@ -614,6 +619,12 @@ void GpuProcessTransportFactory::RemoveCompositor(ui::Compositor* compositor) {
   if (data->surface_handle)
     gpu::GpuSurfaceTracker::Get()->RemoveSurface(data->surface_handle);
 #endif
+
+  // TODO: is this the right place to remove data->begin_frame_source from
+  // the compositor VSync manager?
+  if (data->begin_frame_source)
+    compositor->vsync_manager()->RemoveObserver(data->begin_frame_source);
+
   per_compositor_data_.erase(it);
   if (per_compositor_data_.empty()) {
     // Destroying the GLHelper may cause some async actions to be cancelled,