[Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice

content/browser/renderer_host/media/video_capture_controller.h

Index: content/browser/renderer_host/media/video_capture_controller.h
diff --git a/content/browser/renderer_host/media/video_capture_controller.h b/content/browser/renderer_host/media/video_capture_controller.h
index 0ba1cfb95041653499060596087836132428d2ec..d76bfa2b4f70fbdb522f5c853c7e94e3d772236c 100644
--- a/content/browser/renderer_host/media/video_capture_controller.h
+++ b/content/browser/renderer_host/media/video_capture_controller.h
@@ -13,9 +13,11 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/process/process.h"
+#include "content/browser/renderer_host/media/buildable_video_capture_device.h"
 #include "content/browser/renderer_host/media/video_capture_controller_event_handler.h"
 #include "content/common/content_export.h"
 #include "content/common/media/video_capture.h"
+#include "content/public/common/media_stream_request.h"
 #include "media/capture/video/video_frame_receiver.h"
 #include "media/capture/video_capture_types.h"
 
@@ -23,10 +25,23 @@ namespace content {
 
 // Implementation of media::VideoFrameReceiver that distributes received frames
 // to potentially multiple connected clients.
-class CONTENT_EXPORT VideoCaptureController : public media::VideoFrameReceiver {
+// A call to CreateAndStartDeviceAsync() asynchronously brings up the device. If
+// CreateAndStartDeviceAsync() has been called, ReleaseDeviceAsync() must be
+// called before releasing the instance.
+// Instances must be RefCountedThreadSafe, because an owner
+// (VideoCaptureManager) wants to be able to release its reference during an
+// (asynchronously executing) run of CreateAndStartDeviceAsync(). To this end,
+// the owner passes in the shared ownership as part of |context_reference| into
+// CreateAndStartDeviceAsync().
+class CONTENT_EXPORT VideoCaptureController
+    : public media::VideoFrameReceiver,
+      public base::RefCountedThreadSafe<VideoCaptureController> {
  public:
-  VideoCaptureController();
-  ~VideoCaptureController() override;
+  VideoCaptureController(
+      const std::string& device_id,
+      MediaStreamType stream_type,
+      const media::VideoCaptureParams& params,
+      std::unique_ptr<BuildableVideoCaptureDevice> buildable_device);
 
   base::WeakPtr<VideoCaptureController> GetWeakPtrForIOThread();
 
@@ -104,7 +119,30 @@ class CONTENT_EXPORT VideoCaptureController : public media::VideoFrameReceiver {
   void OnStarted() override;
   void OnStartedUsingGpuDecode() override;
 
+  void CreateAndStartDeviceAsync(const media::VideoCaptureParams& params,
+                                 BuildableDeviceCallbacks* callbacks,
+                                 base::OnceClosure done_cb);
+  void ReleaseDeviceAsync(base::OnceClosure done_cb);
+  bool IsDeviceAlive() const;
+  bool CorrespondsToController(const VideoCaptureController* controller) const;
+  void GetPhotoCapabilities(
+      media::VideoCaptureDevice::GetPhotoCapabilitiesCallback callback) const;
+  void SetPhotoOptions(
+      media::mojom::PhotoSettingsPtr settings,
+      media::VideoCaptureDevice::SetPhotoOptionsCallback callback);
+  void TakePhoto(media::VideoCaptureDevice::TakePhotoCallback callback);
+  void MaybeSuspend();
+  void Resume();
+  void RequestRefreshFrame();
+  void SetDesktopCaptureWindowIdAsync(gfx::NativeViewId window_id,
+                                      base::OnceClosure done_cb);
+  int serial_id() const { return serial_id_; }
+  const std::string& device_id() const { return device_id_; }
+  MediaStreamType stream_type() const { return stream_type_; }
+  const media::VideoCaptureParams& parameters() const { return parameters_; }

[mcasas, 2017/03/22 00:33:42]

Heh two things: this class is starting to have way too
many methods, and it's a code smell to me that has
accessors to so many private methods: it feels like it
exposes too much internals.

[chfremer, 2017/03/22 17:26:52]

I agree with both observations.
However, I would argue that it is still better than what we had before this CL.
We got here by first cutting out a large chunk of functionality from the
monolithic VideoCaptureManager class and subsequently (in PatchSet 3) merging
the class VideoCaptureDeviceEntry into VideoCaptureController, because we felt
it would make things simpler.

Do you see any way of improving this without doing a full redesign?
If not, I would be okay with leaving things as is and consider it as "design
issue that has shifted from VideoCaptureManager to VideoCaptureController".

+
  private:
+  friend class base::RefCountedThreadSafe<VideoCaptureController>;
   struct ControllerClient;
   typedef std::list<std::unique_ptr<ControllerClient>> ControllerClients;
 
@@ -153,6 +191,8 @@ class CONTENT_EXPORT VideoCaptureController : public media::VideoFrameReceiver {
         buffer_read_permission_;
   };
 
+  ~VideoCaptureController() override;
+
   // Find a client of |id| and |handler| in |clients|.
   ControllerClient* FindClient(VideoCaptureControllerID id,
                                VideoCaptureControllerEventHandler* handler,
@@ -178,6 +218,12 @@ class CONTENT_EXPORT VideoCaptureController : public media::VideoFrameReceiver {
                           VideoCaptureControllerID id)>;
   void PerformForClientsWithOpenSession(EventHandlerAction action);
 
+  const int serial_id_;
+  const std::string device_id_;
+  const MediaStreamType stream_type_;
+  const media::VideoCaptureParams parameters_;
+  std::unique_ptr<BuildableVideoCaptureDevice> buildable_device_;
+
   std::unique_ptr<media::VideoFrameConsumerFeedbackObserver>
       consumer_feedback_observer_;