[Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice

content/browser/renderer_host/media/buildable_video_capture_device.h

Index: content/browser/renderer_host/media/buildable_video_capture_device.h
diff --git a/content/browser/renderer_host/media/buildable_video_capture_device.h b/content/browser/renderer_host/media/buildable_video_capture_device.h
new file mode 100644
index 0000000000000000000000000000000000000000..ef71b18350d61686bb0972b2ad545b7f7efb2fb9
--- /dev/null
+++ b/content/browser/renderer_host/media/buildable_video_capture_device.h
@@ -0,0 +1,127 @@
+// Copyright 2017 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 CONTENT_BROWSER_RENDERER_HOST_MEDIA_BUILDABLE_VIDEO_CAPTURE_DEVICE_H_
+#define CONTENT_BROWSER_RENDERER_HOST_MEDIA_BUILDABLE_VIDEO_CAPTURE_DEVICE_H_
+
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "content/public/common/media_stream_request.h"
+#include "media/capture/video/video_capture_device.h"
+#include "media/capture/video_capture_types.h"
+
+namespace content {
+
+class VideoCaptureController;
+
+class BuildableDeviceCallbacks {
+ public:
+  virtual ~BuildableDeviceCallbacks() {}
+  // Returns false if no descriptor was found.
+  virtual const media::VideoCaptureDeviceDescriptor* LookupDeviceDescriptor(
+      const std::string& id) = 0;
+  virtual void WillStartDevice(media::VideoFacingMode facing_mode) = 0;
+  virtual void DidStartDevice(VideoCaptureController* controller) = 0;
+  virtual void OnDeviceStartFailed(VideoCaptureController* controller) = 0;
+};
+
+#define FAIL_COMPILE_REQUIRE_NON_CONST_RVALUE                           \
+  static_assert(!FailCompileOnWrongUsage,                               \
+                "This method may only be invoked on a non-const rvalue" \
+                ", i.e. std::move(object).Method().")
+
+// Encapsulates ownership of |context_reference| and enforces that the instance
+// is released when certain methods are invoked.
+template <bool FailCompileOnWrongUsage = true>
+class DeviceBuildContextT {
+ public:
+  DeviceBuildContextT(base::OnceClosure context_reference,
+                      BuildableDeviceCallbacks* callbacks)
+      : context_reference_(std::move(context_reference)),
+        callbacks_(callbacks) {}
+
+  const media::VideoCaptureDeviceDescriptor* LookupDeviceDescriptor(
+      const std::string& id) {
+    return callbacks_->LookupDeviceDescriptor(id);
+  }
+
+  void WillStartDevice(media::VideoFacingMode facing_mode) {
+    callbacks_->WillStartDevice(facing_mode);
+  }
+
+  void DidStartDevice(VideoCaptureController* controller) const & {
+    FAIL_COMPILE_REQUIRE_NON_CONST_RVALUE;
+  }
+
+  void DidStartDevice(VideoCaptureController* controller) && {
+    callbacks_->DidStartDevice(controller);
+  }
+
+  void OnDeviceStartFailed(VideoCaptureController* controller) const & {
+    FAIL_COMPILE_REQUIRE_NON_CONST_RVALUE;
+  }
+
+  void OnDeviceStartFailed(VideoCaptureController* controller) && {
+    callbacks_->OnDeviceStartFailed(controller);

[miu, 2017/03/20 23:02:15]

This is an interesting way ensure the methods are called on an object "just
about to be destroyed." There are probably super-fancy C++ ways to subvert this,
but that would set off red flags with any reasonable future code reviewer. So,
it seems to work well. :)

[chfremer, 2017/03/20 23:58:26]

Agreed. I had no idea this was possible until I checked how base::OnceCallback
is implemented.

+  }
+
+ private:
+  // For pure move-only ownership. Not meant to be called.
+  base::OnceClosure context_reference_;
+
+  BuildableDeviceCallbacks* callbacks_;
+};
+using DeviceBuildContext = DeviceBuildContextT<>;

[miu, 2017/03/20 23:02:15]

Suggestion: Taking your idea, but re-stating it with IMHO much simpler code.
Instead of a separate BuildableDeviceCallbacks and DeviceBuildContextT<T>, what
if we just define a single abstract class that acts as the interface, with the
special "done" semantics baked-in:

class DeviceBuildContext {
 protected:
  DeviceBuildContext() = default;
  DeviceBuildContext(const base::OnceClosure& done_cb) : done_cb_(done_cb) {}
  DeviceBuildContext(DeviceBulidContext&& context)
    : done_cb_(std::move(context.done_cb_)) {}

  virtual ~DeviceBuildContext() {
    if (!done_cb_.is_null())
      done_cb_.Run();
  }

  // Returns false if no descriptor was found.
  virtual const media::VideoCaptureDeviceDescriptor* LookupDeviceDescriptor(
      const std::string& id) = 0;

  virtual void WillStartDevice(media::VideoFacingMode facing_mode) = 0;
  virtual void DidStartDevice(VideoCaptureController* controller) && = 0;
  virtual void OnDeviceStartFailed(VideoCaptureController* controller) && = 0;

 private:
  // Disallow calling done-condition methods on an L-value instance. This
  // R-value requirement is to ensure the DeviceBuildContext is about to be
  // destroyed right after one of the done-condition methods is called.
  void DidStartDevice(VideoCaptureController* controller) const &;
  void OnDeviceStartFailed(VideoCaptureController* controller) const &;

  // Run immediately after one of the done-condition methods is called.
  base::OnceClosure done_cb_;
};

Because they are private, no subclass can override the L-value done methods, and
they are invisible to callers. Also, you would not define these "disallowed"
methods anywhere: since there cannot be any callers, the linker should not
require them to be defined.

Also interesting is the |done_cb|: Since DeviceBuildContext is a move-only
class, it is guaranteed to be run once at the end, automatically, when the final
DeviceBuildContext is destroyed after the DidStartDevice() call.

WDYT?

[chfremer, 2017/03/20 23:58:26]

That is a very nice idea. I tried something similar when I was experimenting but
ran into some issue that may prevent this from working. It seems that the &&
method qualifier does not play nice with abstract classes. IIUC, an instance of
an abstract class has to be a pointer, and when calling a method on a pointer,
it is impossible to call on an R-value, because dereferencing a pointer always
delivers an L-value, see [1].

[1] http://stackoverflow.com/questions/30093923/function-qualifier-behaviour 
If I read correctly, this is not because DeviceBuildContext is a move-only
class, but because we are explicitly calling |done_cb_.Run()| in the destructor.
Even though it is nice that we can do that, I don't see why we need to do that
if the only job of |done_cb_| is to carry ownership. The owned resources would
be freed on destruction of |done_cb_|, and therefore on destruction of
DeviceBuildContext, even if we don't call |done_cb_.Run()|.
 

+
+// Abstraction for a video capture device that must be "built" before it can be
+// operated and must be "stopped", before it can be released. Typical operation
+// is that a newly created instance initially reports IsDeviceAlive() == false.
+// Clients call CreateAndStartDeviceAsync(), which kicks off the asynchronous
+// building of the device. The outcome of the device building is typically
+// reported to an instance of BuildableDeviceCallbacks (see above). Once the
+// device has been built successfully, the "Device operation methods", are
+// allowed to be called. ReleaseDeviceAsync() must be called in order to
+// release the device if it has before been built successfully. After calling
+// ReleaseDeviceAsync(), it is legal to call CreateAndStartDeviceAsync() to
+// rebuild and start the device again.
+class BuildableVideoCaptureDevice {
+ public:
+  virtual ~BuildableVideoCaptureDevice() {}
+
+  // Device management methods.
+  // The passed-in |context_reference| must guarantee that the context relevant
+  // during the asynchronous processing stays alive.
+  virtual void CreateAndStartDeviceAsync(
+      VideoCaptureController* controller,
+      const media::VideoCaptureParams& params,
+      DeviceBuildContext context) = 0;
+  // The passed-in |context_reference| must guarantee that the context relevant
+  // during the asynchronous processing stays alive.
+  virtual void ReleaseDeviceAsync(VideoCaptureController* controller,
+                                  base::OnceClosure done_cb) = 0;
+  virtual bool IsDeviceAlive() const = 0;
+
+  // Device operation methods.
+  virtual void GetPhotoCapabilities(
+      media::VideoCaptureDevice::GetPhotoCapabilitiesCallback callback)
+      const = 0;
+  virtual void SetPhotoOptions(
+      media::mojom::PhotoSettingsPtr settings,
+      media::VideoCaptureDevice::SetPhotoOptionsCallback callback) = 0;
+  virtual void TakePhoto(
+      media::VideoCaptureDevice::TakePhotoCallback callback) = 0;
+  virtual void MaybeSuspendDevice() = 0;
+  virtual void ResumeDevice() = 0;
+  virtual void RequestRefreshFrame() = 0;
+
+  // Methods for specific types of devices.
+  // The passed-in |context_reference| must guarantee that the context relevant
+  // during the asynchronous processing stays alive.
+  virtual void SetDesktopCaptureWindowIdAsync(gfx::NativeViewId window_id,
+                                              base::OnceClosure done_cb) = 0;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_BUILDABLE_VIDEO_CAPTURE_DEVICE_H_