[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..867fb7ee01c30847db225cf63b80a29d80c56af2
--- /dev/null
+++ b/content/browser/renderer_host/media/buildable_video_capture_device.h
@@ -0,0 +1,86 @@
+// Copyright 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 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/browser/renderer_host/media/ownership.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 VideoCaptureDeviceEntry;
+
+class CONTENT_EXPORT BuildableDeviceCallbacks {
+ public:
+  virtual ~BuildableDeviceCallbacks() {}
+  // Returns false if no descriptor was found.
+  virtual bool LookupDeviceDescriptor(
+      const std::string& id,
+      media::VideoCaptureDeviceDescriptor* descriptor) = 0;
+  virtual void OnDeviceAboutToStart(media::VideoFacingMode facing_mode) = 0;

[miu, 2017/03/11 01:13:21]

I've seen a lot of similar code throughout Chromium use the "will" and "did"
naming in situations like this. So consider naming these:

  WillStartDevice(facing_mode);
  DidStartDevice(entry);
  OnDeviceStartFailed(entry);

[chfremer, 2017/03/13 22:02:02]

I like those names when reading the calling code. But I don't like them when
reading the implementing class, in this case VideoCaptureManager. When seeing a
class expose methods WillDoXYZ() or DidXYZ(), I would intuitively expect them to
return bool and tell the caller whether or not the class did or will be doing
XYZ.

Let's go with the "will" and "did" convention for now. The "WillStartDevice" is
hopefully going away in a few CLs, so we may be able to simplify then.

Done.

+  virtual void OnDeviceStarted(VideoCaptureDeviceEntry* entry) = 0;
+  virtual void OnDeviceStartFailed(VideoCaptureDeviceEntry* entry) = 0;
+};
+
+// 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 HasDevice() == 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 {

[miu, 2017/03/11 01:13:21]

naming/description suggestion: It looks like this would more-accurately be
called a "launcher" (VideoCaptureDeviceLauncher), since this code is all about
creating and starting a VideoCaptureDevice, and then owning its lifecycle from
there, and notifying other entities of when it started. Your call...

[chfremer, 2017/03/13 22:02:03]

Hmm, now that I think about it with a fresh mind and some distance (it's been
many months since I wrote this code), my impression is as follows. The
abstraction "BuildableVideoCaptureDevice" has two parts. One part is about
creating and releasing a device. The other part is about operating the device. I
agree that "launcher" would be a good name for the first part. But with the
second part being present, I feel "BuildableVideoCaptureDevice" is a more
suitable name.

To make things cleaner, I think we should split the interface in two, one for
each part. The first interface should be a "factory" that creates an instance of
the second interface, which would be for the device operation. I am not sure why
I did not do it this way a few months back, but I was probably just trying to
stay conceptually close to the existing code and keep this a pure and mechanical
"cut-some-existing-part-out-of-a-large-class" refactoring.

Would it be okay if I apply the splitting of this interface after the end of the
existing CL range 15-20 that is about refactoring VideoCaptureManager? I am
afraid doing it here at 15 would require a lot of merging work for 16-20.

[miu, 2017/03/16 00:01:35]

Ack.
IIUC, by "factory," you mean "launcher," right?
SGTM.

[chfremer, 2017/03/16 17:39:39]

Sorry, yes, I meant the "launcher" part. I got ahead of myself thinking about
what we might call it after splitting the interface. When split out, it may make
sense to call it a "factory", because it would be creating an object
(representing a launched device) that can then be operated. Let's revisit that
idea when we do the splitting.

+ public:
+  virtual ~BuildableVideoCaptureDevice() {}
+
+  // Device management methods.
+  // The passed-in |context_reference| must guarantee that the context relevant

[miu, 2017/03/11 01:13:21]

I'm a little confused by this. If the
BuildableVideoCaptureDevice::CreateAndStartDeviceAsync() method is taking
owership of |context_reference|, isn't it the responsibility of the impl to not
destroy |context_reference| until after it is no longer needed?

Maybe the comment is redundant? Or, am I misunderstanding something?

[chfremer, 2017/03/13 22:02:02]

Your understanding is correct. The comment is confusing.
The comment was meant for the caller, not the implementor of the method.
I felt that the purpose of |context_reference| may not be understood without
this information, but my attempt to explain it apparently backfired :-). Could
you give a suggestion, what kind of comment (including no comment at all) would
help you as a reader understand the purpose of |context_reference|?

[chfremer, 2017/03/16 17:39:39]

I still haven't found a better way of explaining |context_reference| with a
comment. Please let me know if you have any ideas what we could write here. I am
also okay with removing the comment altogether if it more confusing than
helpful.

[miu, 2017/03/17 21:18:15]

That's because it should be a "done callback" to release the references that
need to be held until the operation is complete. ;-)

So, I don't have a strong opinion, but you seemed to, which is why I didn't push
back more earlier. FWIW, if I was writing this code, I'd have used Closures
rather than add a custom reference-holding scheme. Another way to look at this:
If you have to write the code in ownership.h/.cc, why is it not being committed
in src/base?

Let's revisit the earlier thread, where you discuss some disadvantages of using
Closures. CIL:
Actually, this isn't a problem at all. The Closure takes ownership of all
objects bound to it. So, if your code didn't Run() it, the Closure would still
eventually be destroyed anyway, which would chain to the destruction to those
objects.
Wait. Sure it does. A Closure is, by definition, a binding of a function with a
set of owned objects that will be passed as arguments.

If you name it a "done callback," then it's also clear that the client is using
this to execute something after the operation completes. That "something" could
be anything, and is not important to the method implementation, only that it is
Run() after everything the impl does. The client is the entity having the
knowledge that some objects must remain alive until the operation completes; so
I don't see how this isn't a reasonable interface contract.

FWIW, there are plenty of examples of code throughout Chromium that use "done
callbacks" for nothing other than destroying objects after an async operation
completes. Also, there are lots of examples where people have written code
comments on the impl side, like "Warning: Running this callback may delete
|this|!"
Let's compare code written both ways. First, here's an example where you use
Ownership:

+  auto context_reference = base::MakeUnique<OwnershipCollection>();
+  context_reference->AttachOwnership(
+      MakeScopedRefptrOwnership(scoped_refptr<VideoCaptureManager>(this)));
+  context_reference->AttachOwnership(
+     
MakeScopedRefptrOwnership(GetControllerSharedRefFromSerialId(serial_id)));
+  // TODO(chfremer): Check if request->params() can actually be different from
+  // controller->parameters, and simplify if this is not the case.
+  controller->CreateAndStartDeviceAsync(
+      request->params(), static_cast<BuildableDeviceCallbacks*>(this),
+      std::move(context_reference));

Here's how this would look using a Closure:

+  // TODO(chfremer): Check if request->params() can actually be different from
+  // controller->parameters, and simplify if this is not the case.
+  controller->CreateAndStartDeviceAsync(
+      request->params(), static_cast<BuildableDeviceCallbacks*>(this),
+      base::Bind(&OnStartDeviceCompleted,
+                 this, GetControllerSharedRefFromSerialId(serial_id));

Of course, you'd also need to write the trivial OnStartDeviceCompleted()
function:

+  void OnStartDeviceCompleted(
+      const scoped_refptr<VideoCaptureManager>& manager,
+      const scoped_refptr<VideoCaptureController>& controller) {}

But where you would be adding 3 LOC for this helper function, the 69 LOC in
ownership.h and ownership.cc could be deleted.

In addition, OwnershipCollection is maintaining a vector w/ heap allocation:
This cost in CPU and memory is incurred at run time, not at compile time.
If you're still not convinced of my position, you may want to get second
opinions and ask this question on chromium-dev@.

+  // during the asynchronous processing stays alive.
+  virtual void CreateAndStartDeviceAsync(
+      VideoCaptureDeviceEntry* entry,
+      VideoCaptureController* controller,
+      const media::VideoCaptureParams& params,
+      BuildableDeviceCallbacks* callbacks,
+      std::unique_ptr<Ownership> context_reference) = 0;
+  // The passed-in |context_reference| must guarantee that the context relevant
+  // during the asynchronous processing stays alive.
+  virtual void ReleaseDeviceAsync(
+      VideoCaptureController* controller,
+      std::unique_ptr<Ownership> context_reference) = 0;
+  virtual bool HasDevice() const = 0;

[miu, 2017/03/11 01:13:21]

naming suggestion: s/HasDevice/IsDeviceAlive/ They're mostly equivalent except
that my suggestion would hint to readers of the code that something interesting
has to happen before we have a device, and something interesting could happen to
make it go away later.

[chfremer, 2017/03/13 22:02:02]

Done.

+
+  // 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,
+      std::unique_ptr<Ownership> context_reference) = 0;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_BUILDABLE_VIDEO_CAPTURE_DEVICE_H_