Video Capture: Allow suspension of individual devices.

content/renderer/media/video_capture_impl_manager.h

 // Copyright (c) 2012 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_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"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
 #include "content/common/content_export.h"
 #include "content/common/media/video_capture.h"
@@ skipping 50 matching lines @@
   base::Closure StartCapture(
       media::VideoCaptureSessionId id,
       const media::VideoCaptureParams& params,
       const VideoCaptureStateUpdateCB& state_update_cb,
       const VideoCaptureDeliverFrameCB& deliver_frame_cb);
 
   // Requests that the video capturer send a frame "soon" (e.g., to resolve
   // 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,
                                  const VideoCaptureDeviceFormatsCB& callback);
 
   // Get supported formats currently in use for the given session ID.
   // |callback| will be called on the IO thread.
   void GetDeviceFormatsInUse(media::VideoCaptureSessionId id,
                              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 {
     return filter_.get();
   }
 
  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.
   int next_client_id_;
 
   const scoped_refptr<VideoCaptureMessageFilter> filter_;
 
   // Hold a pointer to the Render Main message loop to check we operate on the
   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoCaptureImplManager);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_MANAGER_H_