Decouple BrowserCompositorOutputSurface from BeginFrameSource.

content/browser/compositor/gpu_process_transport_factory.cc

 // Copyright 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 "content/browser/compositor/gpu_process_transport_factory.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 265 matching lines @@
   return GpuDataManagerImpl::GetInstance()->CanUseGpuBrowserCompositor();
 }
 
 void GpuProcessTransportFactory::CreateCompositorFrameSink(
     base::WeakPtr<ui::Compositor> compositor) {
   DCHECK(!!compositor);
   PerCompositorData* data = per_compositor_data_[compositor.get()].get();
   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;
     data->begin_frame_source = nullptr;
   }
 
 #if defined(OS_WIN)
   gfx::RenderingWindowManager::GetInstance()->UnregisterParent(
       compositor->widget());
 #endif
@@ skipping 144 matching lines @@
   if (!compositor->GetRendererSettings().disable_display_vsync) {
     begin_frame_source.reset(new cc::DelayBasedBeginFrameSource(
         base::MakeUnique<cc::DelayBasedTimeSource>(
             compositor->task_runner().get())));
   } else {
     begin_frame_source.reset(new cc::BackToBackBeginFrameSource(
         base::MakeUnique<cc::DelayBasedTimeSource>(
             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();
     } else {
       display_output_surface = std::move(vulkan_surface);
     }
   }
 #endif
 
   if (!display_output_surface) {
     if (!create_gpu_output_surface) {
       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();
       if (data->surface_handle == gpu::kNullSurfaceHandle) {
         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());
 #endif
       } else {
         std::unique_ptr<display_compositor::CompositorOverlayCandidateValidator>
             validator;
         const bool use_mus = IsUsingMus();
 #if !defined(OS_MACOSX)
         // Overlays are only supported on surfaceless output surfaces on Mac.
         if (!use_mus)
           validator = CreateOverlayCandidateValidator(compositor->widget());
 #endif
         if (!use_mus) {
           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()) {
             // TODO(mfomitchev): Remove this clause once we complete the switch
             // to Aura-Mus.
             display_output_surface =
                 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(
                     compositor->widget());
             display_output_surface =
                 base::MakeUnique<MusBrowserCompositorOutputSurface>(
                     host->window(), context_provider,
                     GetGpuMemoryBufferManager(), compositor->vsync_manager(),
-                    begin_frame_source.get(), std::move(validator));
+                    std::move(validator));
           }
 #else
           NOTREACHED();
 #endif
         }
       }
     }
   }
 
   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);
 
 #if defined(OS_WIN)
   gfx::RenderingWindowManager::GetInstance()->DoSetParentOnChild(
       compositor->widget());
 #endif
 
   std::unique_ptr<cc::DisplayScheduler> scheduler(new cc::DisplayScheduler(
@@ skipping 55 matching lines @@
 void GpuProcessTransportFactory::RemoveCompositor(ui::Compositor* compositor) {
   PerCompositorDataMap::iterator it = per_compositor_data_.find(compositor);
   if (it == per_compositor_data_.end())
     return;
   PerCompositorData* data = it->second.get();
   DCHECK(data);
 #if !defined(GPU_SURFACE_HANDLE_IS_ACCELERATED_WINDOW)
   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,
     // causing things to request a new GLHelper. Due to crbug.com/176091 the
     // GLHelper created in this case would be lost/leaked if we just reset()
     // on the |gl_helper_| variable directly. So instead we call reset() on a
     // local std::unique_ptr.
     std::unique_ptr<display_compositor::GLHelper> helper =
         std::move(gl_helper_);
 
@@ skipping 254 matching lines @@
       shared_vulkan_context_provider_ =
           cc::VulkanInProcessContextProvider::Create();
     }
 
     shared_vulkan_context_provider_initialized_ = true;
   }
   return shared_vulkan_context_provider_;
 }
 
 }  // namespace content