GpuVSyncProvider with unit test

gpu/ipc/service/gpu_vsync_provider.h

+// Copyright (c) 2016 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.
+
+#ifndef GPU_IPC_SERVICE_GPU_VSYNC_PROVIDER_H_
+#define GPU_IPC_SERVICE_GPU_VSYNC_PROVIDER_H_
+
+#include <memory>
+
+#include "base/threading/thread.h"
+#include "base/time/time.h"
+#include "gpu/gpu_export.h"
+#include "gpu/ipc/common/surface_handle.h"
+
+namespace gpu {
+
+// Implements waiting for VSync signal on background thread.
+class GPU_EXPORT GpuVSyncProvider : public base::Thread {

[stanisc, 2016/12/22 21:35:08]

Not sure about the class name because it is similar to VSyncProvider which is
related but provides slightly different kind of data. Feel free to suggest a
better name.

[brucedawson, 2016/12/22 21:49:16]

Well I like the name...

+ public:
+  ~GpuVSyncProvider() override;
+
+  using VSyncCallback = base::Callback<void(base::TimeTicks timestamp)>;
+
+  // Create an instance of GpuVSyncProvider and start background thread
+  // Note that callback will be invoked on the worker thread.
+  static std::unique_ptr<GpuVSyncProvider> Create(const VSyncCallback& callback,
+                                                  SurfaceHandle surface_handle);
+
+  // Enable or disable VSync production.
+  void EnableVSync(bool enabled);
+
+ private:
+#if defined(OS_WIN)
+  GpuVSyncProvider(const VSyncCallback& callback, SurfaceHandle surface_handle);
+
+  void StartRunningVSyncOnThread();
+  void WaitForVSyncOnThread();
+
+  void SendVSyncUpdate(base::TimeTicks now);
+
+  // Specifies whether VSync generation is enabled.
+  // This can be set of main thread only and accessed from both threads.

[stanisc, 2016/12/22 21:35:08]

Need to replace of -> on.

[stanisc, 2017/01/04 22:33:04]

Done.

+  volatile bool enabled_ = false;

[brucedawson, 2016/12/22 21:49:16]

Shouldn't need volatile. volatile does tend to 'help' but it actually has no
defined meaning in multi-threaded code so it should usually be avoided. In
general you should prefer locks. Any data that is referenced on multiple threads
should be protected by a lock even if it "seems" safe. Otherwise you are hitting
C++ undefined behavior and it's very difficult to reason about what guarantees
you have.

[stanisc, 2016/12/22 22:59:31]

I had a lock initially but was concerned it could result in priority inversion,
although the earlier version of the code was more complex with more cross-thread
interaction. This is the pattern where enabled in flipped only on main thread
but checked on background thread. It doesn't need to protect any data and is
used just to control execution. Perhaps I should replace it with Interlocked
functions.

[brucedawson, 2016/12/23 00:37:30]

A few possibilities...

1) Use a lock, see if it works. Because you are locking very briefly and with
very little contention the overhead should be trivial.

2) Look in base\atomicops.h, and use its types and functions. There are some
comments their that discourage use of these functions but those comments apply
equally to using Interlocked* directly.

Option #2 is not unreasonable, but you don't need to mark your variables as
volatile. Just always use the functions to read/write the data. Acquire_Load or
NoBarrier_Load, for instance, instead of a raw read. Mixing normal reads/writes
of a variable with atomics reads/writes is when things get ill-defined.

[stanisc, 2017/01/04 22:33:04]

Done.

+  // Specifies whether background task is running.
+  // This can be set on background thread only.

[brucedawson, 2016/12/22 21:49:16]

Can it be read on other threads? If so then it needs to be behind a lock.

If it can only be referenced on the background thread then change the comment to
say that, to make it clear that no lock is needed.

[stanisc, 2016/12/22 22:59:31]

No, this is accessed on background thread only. I'll change the comment to
reflect that.

[stanisc, 2017/01/04 22:33:04]

Done.

+  bool running_ = false;
+
+  const VSyncCallback callback_;
+  const SurfaceHandle surface_handle_;
+#endif  // defined(OS_WIN)
+
+  DISALLOW_COPY_AND_ASSIGN(GpuVSyncProvider);
+};
+
+}  // namespace gpu
+
+#endif  // GPU_IPC_SERVICE_GPU_VSYNC_PROVIDER_H_