Video Capture: Allow suspension of individual devices.

content/renderer/media/video_capture_impl_manager.h

Index: content/renderer/media/video_capture_impl_manager.h
diff --git a/content/renderer/media/video_capture_impl_manager.h b/content/renderer/media/video_capture_impl_manager.h
index 1b7c28b3fb7218017e03aa6b80eb3e57c062f4cc..8a5548e282988eb8d5db24ceacfebd83daeac4d2 100644
--- a/content/renderer/media/video_capture_impl_manager.h
+++ b/content/renderer/media/video_capture_impl_manager.h
@@ -5,8 +5,8 @@
 #ifndef CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_MANAGER_H_
 #define CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_MANAGER_H_
 
-#include <map>
 #include <memory>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
@@ -77,6 +77,10 @@ class CONTENT_EXPORT VideoCaptureImplManager {
   // picture loss or quality issues).
   void RequestRefreshFrame(media::VideoCaptureSessionId id);
 
+  // Requests frame delivery be suspended/resumed for a given capture session.
+  void Suspend(media::VideoCaptureSessionId id);
+  void Resume(media::VideoCaptureSessionId id);
+
   // Get supported formats supported by the device for the given session
   // ID. |callback| will be called on the IO thread.
   void GetDeviceSupportedFormats(media::VideoCaptureSessionId id,
@@ -88,7 +92,9 @@ class CONTENT_EXPORT VideoCaptureImplManager {
                              const VideoCaptureDeviceFormatsCB& callback);
 
   // Make all existing VideoCaptureImpl instances stop/resume delivering
-  // video frames to their clients, depends on flag |suspend|.
+  // video frames to their clients, depends on flag |suspend|. This is called in
+  // response to a RenderView-wide PageHidden/Shown() event. To suspend/resume
+  // an individual session, please call Suspend(id) or Resume(id).
   void SuspendDevices(bool suspend);

[chfremer, 2016/09/27 00:54:59]

Deciding which devices need to be suspended/resumed based on state for both
individual devices plus a global flag seems to be enough responsibility for a
class of its own. Furthermore it seems unrelated to the responsibility this
class has had so far, which is to communicate with the Browser process through
IPC.

Adding this responsibility to this existing class makes this class quite large
and mixes responsibilities. Can we split the suspend/resume functionality out
into a new class and test it in isolation?

[miu, 2016/09/27 23:42:23]

No, that's not the scoping/responsibility of this class. It doesn't make nor
process any IPC calls. Its purpose is to control an individual VideoCaptureImpl
and its lifetime based on multiple clients needing shared access to a single
instance.

There's also the threading concern: This is the main thread's interface to the
VideoCaptureImpls that run on the IO thread.

Granted, there's certainly room for clean-up here. These classes have evolved a
lot, and the comments, naming, and code factoring need a lot of improvement.
For reasons stated above, I believe mitigating the global versus individual
suspend/resume should be a responsibility of this class. It would
over-complexify things to split-out the functionality into a separate class,
because now you'd have two classes that have to synchronize with each other, and
with each maintaining similar state. If you disagree, feel free to show me the
code. ;-)

[chfremer, 2016/09/28 00:27:36]

Oops, I have misunderstood the responsibilities of this class then.
I feel somewhat mislead by the class description stating: 

// This class provides access to a video capture device in the browser
// process through IPC. The main function is to deliver video frames
// to a client.

Could we maybe update the class description to better convey the actual
responsibilities? Also, on second look, this class seems to manage a whole map
of VideoCaptureImpl instances, not just an individual one.
 
:-) Challenge accepted. Please go ahead with your proposed change as is. When I
find some quiet minutes, I'd love to make an attempt at separating the
responsibilities as I see them and then see what complexities I end up with.

 
   VideoCaptureMessageFilter* video_capture_message_filter() const {
@@ -96,21 +102,36 @@ class CONTENT_EXPORT VideoCaptureImplManager {
   }
 
  protected:
-  virtual VideoCaptureImpl* CreateVideoCaptureImplForTesting(
+  virtual std::unique_ptr<VideoCaptureImpl> CreateVideoCaptureImplForTesting(
       media::VideoCaptureSessionId id,
       VideoCaptureMessageFilter* filter) const;
 
  private:
+  struct DeviceEntry {
+    media::VideoCaptureSessionId session_id;
+
+    // To be used and destroyed only on the IO thread.
+    std::unique_ptr<VideoCaptureImpl> impl;
+
+    // Number of clients using |impl|.
+    int client_count;
+
+    // This is set to true if this device is being suspended, via
+    // VideoCaptureImplManager::Suspend().
+    // See also: VideoCaptureImplManager::is_suspending_all_.
+    bool is_individually_suspended;
+
+    DeviceEntry();
+    DeviceEntry(DeviceEntry&& other);
+    DeviceEntry& operator=(DeviceEntry&& other);
+    ~DeviceEntry();
+  };
+
   void StopCapture(int client_id, media::VideoCaptureSessionId id);
   void UnrefDevice(media::VideoCaptureSessionId id);
 
-  // The int is used to count clients of the corresponding VideoCaptureImpl.
-  // VideoCaptureImpl objects are owned by this object. But they are
-  // destroyed on the IO thread. These are raw pointers because we destroy
-  // them manually.
-  typedef std::map<media::VideoCaptureSessionId,
-                   std::pair<int, VideoCaptureImpl*>> VideoCaptureDeviceMap;
-  VideoCaptureDeviceMap devices_;
+  // Devices currently in use.
+  std::vector<DeviceEntry> devices_;
 
   // This is an internal ID for identifying clients of VideoCaptureImpl.
   // The ID is global for the render process.
@@ -122,6 +143,11 @@ class CONTENT_EXPORT VideoCaptureImplManager {
   // right thread.
   const scoped_refptr<base::SingleThreadTaskRunner> render_main_task_runner_;
 
+  // Set to true if SuspendDevices(true) was called. This, along with
+  // DeviceEntry::is_individually_suspended, is used to determine whether to
+  // take action when suspending/resuming each device.
+  bool is_suspending_all_;
+
   // Bound to the render thread.
   // NOTE: Weak pointers must be invalidated before all other member variables.
   base::WeakPtrFactory<VideoCaptureImplManager> weak_factory_;