refactor video capture in browser process to support device sharing across multiple renderer proc...

content/browser/renderer_host/media/video_capture_controller.h

Index: content/browser/renderer_host/media/video_capture_controller.h
===================================================================
--- content/browser/renderer_host/media/video_capture_controller.h	(revision 107671)
+++ content/browser/renderer_host/media/video_capture_controller.h	(working copy)
@@ -5,12 +5,13 @@
 // VideoCaptureController is the glue between VideoCaptureHost,
 // VideoCaptureManager and VideoCaptureDevice.
 // It provides functions for VideoCaptureHost to start a VideoCaptureDevice and
-// is responsible for keeping track of TransportDIBs and filling them with I420
+// is responsible for keeping track of shared DIBs and filling them with I420
 // video frames for IPC communication between VideoCaptureHost and
 // VideoCaptureMessageFilter.
 // It implements media::VideoCaptureDevice::EventHandler to get video frames
 // from a VideoCaptureDevice object and do color conversion straight into the
-// TransportDIBs to avoid a memory copy.
+// shared DIBs to avoid a memory copy.
+// It serves multiple VideoCaptureControllerEventHandlers.
 
 #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_CONTROLLER_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_CONTROLLER_H_
@@ -18,14 +19,15 @@
 #include <list>
 #include <map>
 
+#include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/process.h"
 #include "base/synchronization/lock.h"
 #include "base/task.h"
 #include "content/browser/renderer_host/media/video_capture_controller_event_handler.h"
+#include "media/video/capture/video_capture.h"
 #include "media/video/capture/video_capture_device.h"
 #include "media/video/capture/video_capture_types.h"
-#include "ui/gfx/surface/transport_dib.h"
 
 namespace media_stream {
 class VideoCaptureManager;
@@ -36,66 +38,108 @@
       public media::VideoCaptureDevice::EventHandler {
  public:
   VideoCaptureController(
-      const VideoCaptureControllerID& id,
-      base::ProcessHandle render_process,
-      VideoCaptureControllerEventHandler* event_handler,
       media_stream::VideoCaptureManager* video_capture_manager);
   virtual ~VideoCaptureController();
 
-  // Starts video capturing and tries to use the resolution specified in
-  // params.
-  // When capturing has started VideoCaptureControllerEventHandler::OnFrameInfo
-  // is called with resolution that best matches the requested that the video
+  // Start video capturing and try to use the resolution specified in
+  // |params|.
+  // When capturing has started, the |event_handler| receives a call OnFrameInfo
+  // with resolution that best matches the requested that the video
   // capture device support.
-  void StartCapture(const media::VideoCaptureParams& params);
+  void StartCapture(const VideoCaptureControllerID& id,
+                    VideoCaptureControllerEventHandler* event_handler,
+                    base::ProcessHandle render_process,
+                    const media::VideoCaptureParams& params);
 
   // Stop video capture.
-  // When the capture is stopped and all TransportDIBS have been returned
+  // When the capture is stopped and all DIBs have been returned,
   // VideoCaptureControllerEventHandler::OnReadyToDelete will be called.
-  // |stopped_cb| may be NULL. But a non-NULL Closure can be used to get
-  // a notification when the device is stopped, regardless of
-  // VideoCaptureController's state.
-  void StopCapture(base::Closure stopped_cb);
+  // |force_buffer_return| allows controller to take back all buffers used
+  // by |event_handler|.
+  void StopCapture(const VideoCaptureControllerID& id,
+                   VideoCaptureControllerEventHandler* event_handler,
+                   bool force_buffer_return);
 
   // Return a buffer previously given in
   // VideoCaptureControllerEventHandler::OnBufferReady.
-  void ReturnBuffer(int buffer_id);
+  void ReturnBuffer(const VideoCaptureControllerID& id,
+                    VideoCaptureControllerEventHandler* event_handler,
+                    int buffer_id);
 
   // Implement media::VideoCaptureDevice::EventHandler.
   virtual void OnIncomingCapturedFrame(const uint8* data,
                                        int length,
-                                       base::Time timestamp);
-  virtual void OnError();
-  virtual void OnFrameInfo(const media::VideoCaptureDevice::Capability& info);
+                                       base::Time timestamp) OVERRIDE;
+  virtual void OnError() OVERRIDE;
+  virtual void OnFrameInfo(
+      const media::VideoCaptureDevice::Capability& info) OVERRIDE;
 
  private:
-  // Called by VideoCaptureManager when a device have been stopped.
-  void OnDeviceStopped(base::Closure stopped_cb);
+  struct ControllerClient;
+  typedef std::list<ControllerClient*> ControllerClients;
 
+  // Callback when manager has stopped device.
+  void OnDeviceStopped();
+
+  // Worker functions on IO thread.
+  void DoIncomingCapturedFrameOnIOThread(int buffer_id, base::Time timestamp);
+  void DoFrameInfoOnIOThread(const media::VideoCaptureDevice::Capability info);
+  void DoErrorOnIOThread();
+  void DoDeviceStateOnIOThread(bool in_use);
+  void DoDeviceStoppedOnIOThread();
+
+  // Send frame info and init buffers to |client|.
+  void SendFrameInfoAndBuffers(ControllerClient* client);
+
+  // Helpers.
+  // Find a client of |id| and |handler| in |clients|.
+  ControllerClients::iterator FindClient(
+      const VideoCaptureControllerID& id,
+      VideoCaptureControllerEventHandler* handler,
+      ControllerClients& clients);
+  // Decide what to do after kStopping state. Dependent on events, controller
+  // can stay in kStopping state, or go to kStopped, or restart capture.
+  void PostStopping();
+  // Check if any DIB is used by client.
+  bool ClientHasDIB();
+  void SendFrameInfoAndBuffersWithLock(
+      ControllerClient* client, int needed_size);
+
   // Lock to protect free_dibs_ and owned_dibs_.
   base::Lock lock_;
-  // Handle to the render process that will receive the DIBs.
-  base::ProcessHandle render_handle_;
-  bool report_ready_to_delete_;
-  typedef std::list<int> DIBHandleList;
-  typedef std::map<int, base::SharedMemory*> DIBMap;
 
-  // Free DIBS that can be filled with video frames.
+  typedef std::list<int /*buffer_id*/> DIBHandleList;
+  // Free DIBs that can be filled with video frames.
+  // It can be modified on both IO and device threads.
   DIBHandleList free_dibs_;
 
-  // All DIBS created by this object.
+  typedef std::map<int /*buffer_id*/, base::SharedMemory*> DIBMap;
+  // All DIBs created by this object.
+  // It's modified only on IO thread.
   DIBMap owned_dibs_;
-  VideoCaptureControllerEventHandler* event_handler_;
 
-  // The parameter that was requested when starting the capture device.
-  media::VideoCaptureParams params_;
+  typedef std::map<int /*buffer_id*/, int /*count*/> ClientSideDIBCount;
+  // Keep count of clients to which a DIB has been sent.
+  // It's modified only on IO thread.
+  ClientSideDIBCount client_side_dib_count_;

[scherkus (not reviewing), 2011/10/31 03:20:18]

I find the buffer_id system awfully confusing

why go through the trouble with multiple lists/maps and buffer_ids when
kNoOfDIBS is 3? I doubt you gaining any sort of performance advantage using a
map when you only have three elements.

what about the following:
struct SharedDIB {
  base::SharedMemory* shared_memory;  
  int references;
};
SharedDIB shared_dibs[kNoOfDIBS];

Then inside of functions like OnIncomingCapturedFrame() you'd have much simpler
code, such as:

for (int i = 0; i < kNoOfDIBS; ++i) {
  if (shared_dibs[i].references == 0) {
    // found an available DIB
  }
}

It'd also do away with some of the hard to read iterator/map-based code, such as
the following:
if (--dit->second > 0)
  continue;

[wjia(left Chromium), 2011/10/31 22:02:44]

Added SharedDIB and removed ClientSideDIBCount.

Using map for owned_dibs_ allows controller to use any numbers for buffer_id,
instead of just 0, 1, 2.

Let me know if code clarity/simplicity is more important than flexibility here.

[scherkus (not reviewing), 2011/11/01 02:18:38]

I guess I don't fully understand. What is the flexibility giving you? Do you
plan on switching to opaque handles or the pointer value of SharedDIB?

It's your call -- using a map makes some things easier (i.e., bounds checking is
replaced by map lookup), but it's also a bit more verbose.

[wjia(left Chromium), 2011/11/01 21:34:23]

By "flexibility", I mean we can improve the capture later. For example, current
design doesn't allow controller to restart device till it has received all
shared buffers and the device is fully stopped. This can be improved in such a
way that controller doesn't have to wait for all shared buffers returned. As
long as device has been fully stopped, controller can restart capturing with
another series of buffer_id's. The new buffer_id's don't overlap with previous
ones. This new design can speed up resolution change by allowing buffer return
and new capturing in parallel. 

Another advantage of using map is that buffer_id can be other values, maybe
scrambled ones.

 
-  // ID used for identifying this object.
-  VideoCaptureControllerID id_;
+  // All clients served by this controller.
+  ControllerClients controller_clients_;
+
+  // All clients waiting for service.
+  ControllerClients pending_clients_;
+
+  // The parameter that currently used for the capturing.
+  media::VideoCaptureParams current_params_;
+
   media::VideoCaptureDevice::Capability frame_info_;
+  bool frame_info_available_;
 
   media_stream::VideoCaptureManager* video_capture_manager_;
 
+  bool device_in_use_;
+  media::VideoCapture::State state_;
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(VideoCaptureController);
 };