Route D3D VSync signal to Compositor

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 713812a1012a9e7a75bde89fbb3c362a4d9426a0..f093dfda48c499d84a351fb98ab7ec920aa9e303 100644
--- a/content/browser/compositor/gpu_process_transport_factory.cc
+++ b/content/browser/compositor/gpu_process_transport_factory.cc
@@ -58,6 +58,7 @@
 #include "ui/display/types/display_snapshot.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/switches.h"
+#include "ui/gl/gl_switches.h"
 
 #if defined(USE_AURA)
 #include "content/browser/compositor/mus_browser_compositor_output_surface.h"
@@ -108,6 +109,17 @@ bool IsUsingMus() {
   return service_manager::ServiceManagerIsRemote();
 }
 
+bool IsGpuVSyncSignalSupported() {
+#if defined(OS_WIN)
+  // TODO(stanisc): http://crbug.com/467617 Limit to Windows 8+ for now because
+  // of locking issue caused by waiting for VSync on Win7.
+  return base::win::GetVersion() >= base::win::VERSION_WIN8 &&
+         base::FeatureList::IsEnabled(features::kD3DVsync);
+#else
+  return false;
+#endif  // defined(OS_WIN)
+}
+
 scoped_refptr<ui::ContextProviderCommandBuffer> CreateContextCommon(
     scoped_refptr<gpu::GpuChannelHost> gpu_channel_host,
     gpu::SurfaceHandle surface_handle,
@@ -168,7 +180,10 @@ namespace content {
 struct GpuProcessTransportFactory::PerCompositorData {
   gpu::SurfaceHandle surface_handle = gpu::kNullSurfaceHandle;
   BrowserCompositorOutputSurface* display_output_surface = nullptr;
-  std::unique_ptr<cc::SyntheticBeginFrameSource> begin_frame_source;
+  // Either |synthetic_begin_frame_source| or |gpu_vsync_begin_frame_source| is
+  // valid but not both at the same time.
+  std::unique_ptr<cc::SyntheticBeginFrameSource> synthetic_begin_frame_source;
+  std::unique_ptr<GpuVSyncBeginFrameSource> gpu_vsync_begin_frame_source;
   ReflectorImpl* reflector = nullptr;
   std::unique_ptr<cc::Display> display;
   bool output_is_secure = false;
@@ -460,20 +475,10 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
     }
   }
 
-  std::unique_ptr<cc::SyntheticBeginFrameSource> synthetic_begin_frame_source;
-  if (!compositor->GetRendererSettings().disable_display_vsync) {
-    synthetic_begin_frame_source.reset(new cc::DelayBasedBeginFrameSource(
-        base::MakeUnique<cc::DelayBasedTimeSource>(
-            compositor->task_runner().get())));
-  } else {
-    synthetic_begin_frame_source.reset(new cc::BackToBackBeginFrameSource(
-        base::MakeUnique<cc::DelayBasedTimeSource>(
-            compositor->task_runner().get())));
-  }
-  cc::BeginFrameSource* begin_frame_source = synthetic_begin_frame_source.get();
-
   BrowserCompositorOutputSurface::UpdateVSyncParametersCallback vsync_callback =
       base::Bind(&ui::Compositor::SetDisplayVSyncParameters, compositor);
+  cc::BeginFrameSource* begin_frame_source = nullptr;
+  GpuVSyncControl* gpu_vsync_control = nullptr;
 
   std::unique_ptr<BrowserCompositorOutputSurface> display_output_surface;
 #if defined(ENABLE_VULKAN)
@@ -513,13 +518,15 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
             CreateOverlayCandidateValidator(compositor->widget()),
             GetGpuMemoryBufferManager());
 #else
-        display_output_surface =
+        auto gpu_output_surface =
             base::MakeUnique<GpuSurfacelessBrowserCompositorOutputSurface>(
                 context_provider, data->surface_handle, vsync_callback,
                 CreateOverlayCandidateValidator(compositor->widget()),
                 GL_TEXTURE_2D, GL_RGB,
                 display::DisplaySnapshot::PrimaryFormat(),
                 GetGpuMemoryBufferManager());
+        gpu_vsync_control = gpu_output_surface.get();
+        display_output_surface = std::move(gpu_output_surface);
 #endif
       } else {
         std::unique_ptr<display_compositor::CompositorOverlayCandidateValidator>
@@ -531,22 +538,22 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
           validator = CreateOverlayCandidateValidator(compositor->widget());
 #endif
         if (!use_mus) {
-          display_output_surface =
+          auto gpu_output_surface =
               base::MakeUnique<GpuBrowserCompositorOutputSurface>(
                   context_provider, vsync_callback, std::move(validator));
+          gpu_vsync_control = gpu_output_surface.get();
+          display_output_surface = std::move(gpu_output_surface);
         } else {
 #if defined(USE_AURA)
-          std::unique_ptr<MusBrowserCompositorOutputSurface> mus_output_surface;
           aura::WindowTreeHost* host =
               aura::WindowTreeHost::GetForAcceleratedWidget(
                   compositor->widget());
-          mus_output_surface =
+          auto mus_output_surface =
               base::MakeUnique<MusBrowserCompositorOutputSurface>(
                   host->window(), context_provider, GetGpuMemoryBufferManager(),
                   vsync_callback, std::move(validator));
           // We use the ExternalBeginFrameSource provided by the output surface
           // instead of our own synthetic one.
-          synthetic_begin_frame_source.reset();
           begin_frame_source = mus_output_surface->GetBeginFrameSource();
           DCHECK(begin_frame_source);
           display_output_surface = std::move(mus_output_surface);
@@ -558,7 +565,29 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
     }
   }
 
-  data->display_output_surface = display_output_surface.get();
+  std::unique_ptr<cc::SyntheticBeginFrameSource> synthetic_begin_frame_source;
+  std::unique_ptr<GpuVSyncBeginFrameSource> gpu_vsync_begin_frame_source;
+
+  if (!begin_frame_source) {
+    if (!compositor->GetRendererSettings().disable_display_vsync) {

[danakj, 2017/02/16 15:56:02]

nit: maybe it's just me but i think it's easier to read generally if the shorter
body comes from the if() than the else, so I'd invert this

[stanisc, 2017/02/16 21:43:19]

I see your point. The current if() case is predominantly more common because
disabling vsync requires a special command line switch. And it was this way
before so I am hesitant to change this.

+      if (gpu_vsync_control && IsGpuVSyncSignalSupported()) {
+        gpu_vsync_begin_frame_source.reset(
+            new GpuVSyncBeginFrameSource(gpu_vsync_control));

[danakj, 2017/02/16 15:56:02]

prefer "= MakeUnique<T>" to ".reset(new T"

[stanisc, 2017/02/16 21:43:18]

Done.

+        begin_frame_source = gpu_vsync_begin_frame_source.get();
+      } else {
+        synthetic_begin_frame_source.reset(new cc::DelayBasedBeginFrameSource(
+            base::MakeUnique<cc::DelayBasedTimeSource>(
+                compositor->task_runner().get())));
+        begin_frame_source = synthetic_begin_frame_source.get();
+      }
+    } else {
+      synthetic_begin_frame_source.reset(new cc::BackToBackBeginFrameSource(
+          base::MakeUnique<cc::DelayBasedTimeSource>(
+              compositor->task_runner().get())));
+      begin_frame_source = synthetic_begin_frame_source.get();
+    }
+  }
+
   if (data->reflector)
     data->reflector->OnSourceSurfaceReady(data->display_output_surface);
 
@@ -578,9 +607,10 @@ void GpuProcessTransportFactory::EstablishedGpuChannel(
       begin_frame_source, std::move(display_output_surface),
       std::move(scheduler), base::MakeUnique<cc::TextureMailboxDeleter>(
                                 compositor->task_runner().get()));
-  // Note that we are careful not to destroy a prior |data->begin_frame_source|
+  // Note that we are careful not to destroy prior begin frame sources

[danakj, 2017/02/16 15:56:02]

BeginFrameSources will be more greppable and search/replaceable

[stanisc, 2017/02/16 21:43:18]

Done.

   // until we have reset |data->display|.
-  data->begin_frame_source = std::move(synthetic_begin_frame_source);
+  data->synthetic_begin_frame_source = std::move(synthetic_begin_frame_source);
+  data->gpu_vsync_begin_frame_source = std::move(gpu_vsync_begin_frame_source);
 
   // The |delegated_output_surface| is given back to the compositor, it
   // delegates to the Display as its root surface. Importantly, it shares the
@@ -743,8 +773,8 @@ void GpuProcessTransportFactory::SetAuthoritativeVSyncInterval(
     return;
   PerCompositorData* data = it->second.get();
   DCHECK(data);
-  if (data->begin_frame_source)
-    data->begin_frame_source->SetAuthoritativeVSyncInterval(interval);
+  if (data->synthetic_begin_frame_source)
+    data->synthetic_begin_frame_source->SetAuthoritativeVSyncInterval(interval);
 }
 
 void GpuProcessTransportFactory::SetDisplayVSyncParameters(
@@ -756,8 +786,11 @@ void GpuProcessTransportFactory::SetDisplayVSyncParameters(
     return;
   PerCompositorData* data = it->second.get();
   DCHECK(data);
-  if (data->begin_frame_source)
-    data->begin_frame_source->OnUpdateVSyncParameters(timebase, interval);
+  if (data->synthetic_begin_frame_source) {
+    data->synthetic_begin_frame_source->OnUpdateVSyncParameters(timebase,
+                                                                interval);
+  } else if (data->gpu_vsync_begin_frame_source)

[danakj, 2017/02/16 15:56:02]

if one block of if/else has {} then they all do

https://google.github.io/styleguide/cppguide.html#Conditionals

[stanisc, 2017/02/16 21:43:18]

Done.

+    data->gpu_vsync_begin_frame_source->OnVSync(timebase, interval);
 }
 
 void GpuProcessTransportFactory::SetOutputIsSecure(ui::Compositor* compositor,