Changed GpuVSyncProvider to implement gfx::VSyncProvider

gpu/ipc/service/gpu_vsync_provider_win.cc

Index: gpu/ipc/service/gpu_vsync_provider_win.cc
diff --git a/gpu/ipc/service/gpu_vsync_provider_win.cc b/gpu/ipc/service/gpu_vsync_provider_win.cc
index a996e6b6b027a7b6831cd05438ed80647b029cbc..f99bf2277540b0eff209b275f72f8b4824b8bdcf 100644
--- a/gpu/ipc/service/gpu_vsync_provider_win.cc
+++ b/gpu/ipc/service/gpu_vsync_provider_win.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "gpu/ipc/service/gpu_vsync_provider.h"
+#include "gpu/ipc/service/gpu_vsync_provider_win.h"
 
 #include <string>
 
@@ -10,6 +10,10 @@
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread.h"
 #include "base/trace_event/trace_event.h"
+#include "gpu/ipc/common/gpu_messages.h"
+#include "ipc/ipc_message_macros.h"
+#include "ipc/message_filter.h"
+#include "ui/gl/vsync_provider_win.h"
 
 #include <windows.h>
 
@@ -49,18 +53,25 @@ typedef NTSTATUS(APIENTRY* PFND3DKMTWAITFORVERTICALBLANKEVENT)(
 
 // The actual implementation of background tasks plus any state that might be
 // needed on the worker thread.
-class GpuVSyncWorker : public base::Thread {
+class GpuVSyncWorker : public base::Thread,
+                       public base::RefCountedThreadSafe<GpuVSyncWorker> {
  public:
-  GpuVSyncWorker(const GpuVSyncProvider::VSyncCallback& callback,
+  GpuVSyncWorker(const gfx::VSyncProvider::UpdateVSyncCallback& callback,
                  SurfaceHandle surface_handle);
-  ~GpuVSyncWorker() override;
 
+  void CleanupAndStop();
   void Enable(bool enabled);
   void StartRunningVSyncOnThread();
   void WaitForVSyncOnThread();
-  void SendVSyncUpdate(base::TimeTicks timestamp);
+  void SendVSyncUpdate(base::TimeTicks now,
+                       base::TimeTicks timestamp,
+                       base::TimeDelta interval);
+  bool BelongsToWorkerThread();
 
  private:
+  friend class base::RefCountedThreadSafe<GpuVSyncWorker>;
+  ~GpuVSyncWorker() override;
+
   void Reschedule();
   void OpenAdapter(const wchar_t* device_name);
   void CloseAdapter();
@@ -74,9 +85,12 @@ class GpuVSyncWorker : public base::Thread {
   // threads but can be changed on the main thread only.
   base::subtle::AtomicWord enabled_ = false;
 
-  const GpuVSyncProvider::VSyncCallback callback_;
+  const gfx::VSyncProvider::UpdateVSyncCallback callback_;
   const SurfaceHandle surface_handle_;
 
+  // The actual timing and interval comes from the nested provider.
+  std::unique_ptr<gl::VSyncProviderWin> vsync_provider_;
+
   PFND3DKMTOPENADAPTERFROMHDC open_adapter_from_hdc_ptr_;
   PFND3DKMTCLOSEADAPTER close_adapter_ptr_;
   PFND3DKMTWAITFORVERTICALBLANKEVENT wait_for_vertical_blank_event_ptr_;
@@ -86,11 +100,13 @@ class GpuVSyncWorker : public base::Thread {
   D3DDDI_VIDEO_PRESENT_SOURCE_ID current_source_id_ = 0;
 };
 
-GpuVSyncWorker::GpuVSyncWorker(const GpuVSyncProvider::VSyncCallback& callback,
-                               SurfaceHandle surface_handle)
+GpuVSyncWorker::GpuVSyncWorker(
+    const gfx::VSyncProvider::UpdateVSyncCallback& callback,
+    SurfaceHandle surface_handle)
     : base::Thread(base::StringPrintf("VSync-%d", surface_handle)),
       callback_(callback),
-      surface_handle_(surface_handle) {
+      surface_handle_(surface_handle),
+      vsync_provider_(new gl::VSyncProviderWin(surface_handle)) {
   HMODULE gdi32 = GetModuleHandle(L"gdi32");
   if (!gdi32) {
     NOTREACHED() << "Can't open gdi32.dll";
@@ -120,7 +136,9 @@ GpuVSyncWorker::GpuVSyncWorker(const GpuVSyncProvider::VSyncCallback& callback,
   }
 }
 
-GpuVSyncWorker::~GpuVSyncWorker() {
+GpuVSyncWorker::~GpuVSyncWorker() = default;
+
+void GpuVSyncWorker::CleanupAndStop() {
   // Thread::Close() call below will block until this task has finished running
   // so it is safe to post it here and pass unretained pointer.
   task_runner()->PostTask(FROM_HERE, base::Bind(&GpuVSyncWorker::CloseAdapter,
@@ -140,8 +158,12 @@ void GpuVSyncWorker::Enable(bool enabled) {
                               base::Unretained(this)));
 }
 
+bool GpuVSyncWorker::BelongsToWorkerThread() {
+  return base::PlatformThread::CurrentId() == GetThreadId();
+}
+
 void GpuVSyncWorker::StartRunningVSyncOnThread() {
-  DCHECK(base::PlatformThread::CurrentId() == GetThreadId());
+  DCHECK(BelongsToWorkerThread());
 
   if (!running_) {
     running_ = true;
@@ -150,7 +172,7 @@ void GpuVSyncWorker::StartRunningVSyncOnThread() {
 }
 
 void GpuVSyncWorker::WaitForVSyncOnThread() {
-  DCHECK(base::PlatformThread::CurrentId() == GetThreadId());
+  DCHECK(BelongsToWorkerThread());
 
   TRACE_EVENT0("gpu", "GpuVSyncWorker::WaitForVSyncOnThread");
 
@@ -168,18 +190,38 @@ void GpuVSyncWorker::WaitForVSyncOnThread() {
   }
 
   if (WaitForVBlankEvent()) {
-    // Note: this sends update on background thread which the callback is
-    // expected to handle.
-    SendVSyncUpdate(base::TimeTicks::Now());
+    vsync_provider_->GetVSyncParameters(
+        base::Bind(&GpuVSyncWorker::SendVSyncUpdate, base::Unretained(this),
+                   base::TimeTicks::Now()));
   }
 
   Reschedule();
 }
 
-void GpuVSyncWorker::SendVSyncUpdate(base::TimeTicks timestamp) {
+void GpuVSyncWorker::SendVSyncUpdate(base::TimeTicks now,
+                                     base::TimeTicks timestamp,
+                                     base::TimeDelta interval) {
+  base::TimeDelta adjustment;
+
+  if (!timestamp.is_null()) {
+    // Timestamp comes from DwmGetCompositionTimingInfo and apparently it might
+    // be up to 2-3 vsync cycles in the past or in the future.
+    // The adjustment formula was suggested here:
+    // http://www.vsynctester.com/firefoxisbroken.html
+    base::TimeDelta adjustment =
+        ((now - timestamp + interval / 8) % interval + interval) % interval -

[jbauman, 2017/02/14 02:59:22]

Can interval be 0 here, due to buggy drivers?

[stanisc, 2017/02/14 19:56:13]

Good question! Compositor code deals with zero vsync interval replacing it with
default 16.7 ms interval which means it is possible to receive zero interval
here. I am going to treat that the same as zero timestamp and just use "now"
time which in my experience is usually within 0.03-0.1 ms of the timestamp. That
should probably be good enough for that corner case.

+        interval / 8;
+    timestamp = now - adjustment;
+  } else {
+    // DWM must be disabled.
+    timestamp = now;
+  }
+
+  TRACE_EVENT1("gpu", "GpuVSyncWorker::SendVSyncUpdate", "adjustment",
+               adjustment.ToInternalValue());
+
   if (base::subtle::NoBarrier_Load(&enabled_)) {
-    TRACE_EVENT0("gpu", "GpuVSyncWorker::SendVSyncUpdate");
-    callback_.Run(timestamp);
+    callback_.Run(timestamp, interval);
   }
 }
 
@@ -237,17 +279,88 @@ bool GpuVSyncWorker::WaitForVBlankEvent() {
   return result == STATUS_SUCCESS;
 }
 
-/* static */
-std::unique_ptr<GpuVSyncProvider> GpuVSyncProvider::Create(
-    const VSyncCallback& callback,
-    SurfaceHandle surface_handle) {
-  return std::unique_ptr<GpuVSyncProvider>(
-      new GpuVSyncProvider(callback, surface_handle));
+// MessageFilter class for sending and receiving IPC messages
+// directly, avoiding routing them through the main GPU thread.
+class GpuVSyncMessageFilter : public IPC::MessageFilter {
+ public:
+  explicit GpuVSyncMessageFilter(
+      const scoped_refptr<GpuVSyncWorker>& vsync_worker,
+      int32_t route_id)
+      : vsync_worker_(vsync_worker), route_id_(route_id) {}
+
+  // IPC::MessageFilter overrides.
+  void OnChannelError() override { Reset(); }
+  void OnChannelClosing() override { Reset(); }
+  void OnFilterAdded(IPC::Channel* channel) override;
+  void OnFilterRemoved() override { Reset(); }
+  bool OnMessageReceived(const IPC::Message& msg) override;
+
+  // Send can be called from GpuVSyncWorker thread.
+  void Send(std::unique_ptr<IPC::Message> message);
+
+  int32_t route_id() const { return route_id_; }
+
+ private:
+  ~GpuVSyncMessageFilter() override = default;
+  void SendOnIOThread(std::unique_ptr<IPC::Message> message);
+  void Reset();
+
+  scoped_refptr<GpuVSyncWorker> vsync_worker_;
+  // The sender to which this filter was added.
+  IPC::Sender* sender_ = nullptr;
+  // The sender must be invoked on IO thread.
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+  int32_t route_id_;

[jbauman, 2017/02/14 02:59:22]

nit: const int32_t route_id_;

[stanisc, 2017/02/14 19:56:13]

Done.

+};
+
+void GpuVSyncMessageFilter::OnFilterAdded(IPC::Channel* channel) {
+  io_task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  sender_ = channel;
+}
+
+void GpuVSyncMessageFilter::Reset() {
+  sender_ = nullptr;
+  vsync_worker_->Enable(false);
+}
+
+bool GpuVSyncMessageFilter::OnMessageReceived(const IPC::Message& msg) {
+  if (msg.routing_id() != route_id_)
+    return false;
+
+  IPC_BEGIN_MESSAGE_MAP(GpuVSyncMessageFilter, msg)
+    IPC_MESSAGE_FORWARD(GpuCommandBufferMsg_SetNeedsVSync, vsync_worker_.get(),
+                        GpuVSyncWorker::Enable);
+    IPC_MESSAGE_UNHANDLED(return false)
+  IPC_END_MESSAGE_MAP()
+  return true;
+}
+
+void GpuVSyncMessageFilter::Send(std::unique_ptr<IPC::Message> message) {
+  CHECK(io_task_runner_->PostTask(

[dcheng, 2017/02/14 09:03:17]

Is it necessary to CHECK() this? CHECK() should be pretty rare, per the logging
guidance in
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[stanisc, 2017/02/14 19:56:13]

Not necessary. I've copied this as an example from other IPC filter
implementation. I've remove the CHECK.

+      FROM_HERE, base::Bind(&GpuVSyncMessageFilter::SendOnIOThread, this,
+                            base::Passed(&message))));
+}
+
+void GpuVSyncMessageFilter::SendOnIOThread(
+    std::unique_ptr<IPC::Message> message) {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  if (!sender_ || message->is_sync()) {
+    DCHECK(!message->is_sync());

[jbauman, 2017/02/14 02:59:22]

Nit: move this dcheck up and remove the "|| message->is_sync()" from the if.

[stanisc, 2017/02/14 19:56:13]

Done.

+    return;
+  }
+  sender_->Send(message.release());
 }
 
-GpuVSyncProvider::GpuVSyncProvider(const VSyncCallback& callback,
-                                   SurfaceHandle surface_handle)
-    : vsync_worker_(new GpuVSyncWorker(callback, surface_handle)) {
+GpuVSyncProviderWin::GpuVSyncProviderWin(
+    base::WeakPtr<ImageTransportSurfaceDelegate> delegate,

[dcheng, 2017/02/14 09:03:17]

I'm curious why this needs to be a WeakPtr: we dereference it without checking,
and we don't store it. Maybe it can just be a raw pointer?

[stanisc, 2017/02/14 19:56:13]

Good question. This pointer already comes from GpuCommandBufferStub as WeakPtr -
see ImageTransportSurface::CreateNativeSurface. Here is how I plan to integrate
GpuVSyncProviderWin -
https://codereview.chromium.org/2626413002/diff/60001/gpu/ipc/service/image_t....

It is true that CreateNativeSurface could resolve the weak pointer and pass a
raw pointer here. I decided to keep the weak pointer in case a future iteration
of this code would need to store the delegate. I feels more future-proof this
way. 

+    SurfaceHandle surface_handle) {
+  vsync_worker_ = new GpuVSyncWorker(
+      base::Bind(&GpuVSyncProviderWin::OnVSync, base::Unretained(this)),
+      surface_handle);
+  message_filter_ =
+      new GpuVSyncMessageFilter(vsync_worker_, delegate->GetRouteID());
+  delegate->AddFilter(message_filter_.get());
+
   // Start the thread.
   base::Thread::Options options;
   // TODO(stanisc): might consider even higher priority - REALTIME_AUDIO.
@@ -255,10 +368,25 @@ GpuVSyncProvider::GpuVSyncProvider(const VSyncCallback& callback,
   vsync_worker_->StartWithOptions(options);
 }
 
-GpuVSyncProvider::~GpuVSyncProvider() = default;
+GpuVSyncProviderWin::~GpuVSyncProviderWin() {
+  vsync_worker_->CleanupAndStop();
+}
+
+void GpuVSyncProviderWin::GetVSyncParameters(
+    const UpdateVSyncCallback& callback) {
+  // This is ignored and the |callback| is never called back.  The timestamp
+  // and interval are posted directly via
+  // GpuCommandBufferMsg_UpdateVSyncParameters message sent from the worker
+  // thread.
+}
+
+void GpuVSyncProviderWin::OnVSync(base::TimeTicks timestamp,
+                                  base::TimeDelta interval) {
+  DCHECK(vsync_worker_->BelongsToWorkerThread());
 
-void GpuVSyncProvider::EnableVSync(bool enabled) {
-  vsync_worker_->Enable(enabled);
+  message_filter_->Send(
+      base::MakeUnique<GpuCommandBufferMsg_UpdateVSyncParameters>(
+          message_filter_->route_id(), timestamp, interval));
 }
 
 }  // namespace gpu