VideoCaptureManager opens/closes, starts/stops and enumerates video capture devices. VideoCapture...

content/browser/media_stream/video_capture_manager.h

+// Copyright (c) 2011 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.
+
+// VideoCaptureManager is used to open/close, start/stop as well as enumerate
+// available video capture devices. All functions are expected to be called from
+// the Browser::IO thread. VideoCaptureManager will open OS dependent instances
+// of VideoCaptureDevice. A device can only be opened once.
+
+#ifndef CONTENT_BROWSER_MEDIA_STREAM_VIDEO_CAPTURE_MANAGER_H_
+#define CONTENT_BROWSER_MEDIA_STREAM_VIDEO_CAPTURE_MANAGER_H_
+
+#include <map>
+
+#include "base/lazy_instance.h"
+#include "base/threading/thread.h"
+#include "content/browser/media_stream/media_stream_provider.h"
+#include "media/video/capture/video_capture_device.h"
+#include "media/video/capture/video_capture_types.h"
+
+namespace media_stream {
+
+class VideoCaptureManager

[John Knottenbelt, 2011/05/11 11:36:35]

Please provide a class level comment describing the purpose of this class.

[mflodman1, 2011/05/15 20:24:00]

I wrote a description at the top of this file. Do you think that one should be
moved to here, shall the explanation be extended and more detailed or shall
there just be a small explanatino as added?

+    : public MediaStreamProvider {

[John Knottenbelt, 2011/05/11 11:36:35]

Join with the previous line.

[mflodman1, 2011/05/15 20:24:00]

Done.

+  public:
+  // Calling Start of this id will open the first device, even though open has
+  // not been called.
+  enum { kStartOpenSessionId = 1 };
+
+  // Called to get a pointer to the singleton
+  static VideoCaptureManager* Get();
+
+  // Implements MediaStreamProvider.
+  virtual bool Register(MediaStreamType service_type,
+                       MediaStreamProviderListener* listener);
+
+  virtual void Unregister(MediaStreamType stream_type,
+                          MediaStreamProviderListener* listener);
+
+  virtual void EnumerateDevices(MediaStreamType stream_type);
+
+  virtual MediaCaptureSessionId Open(MediaStreamType stream_type,
+                                     const MediaCaptureDeviceInfo& device);
+  virtual void Close(MediaStreamType stream_type,
+                     MediaCaptureSessionId capture_session_id);
+
+  // Functions used to start and stop media flow.
+  // Start allocates the device and no other application can use the device
+  // before Stop is called.
+  void Start(const media::VideoCaptureParams& capture_params,

[John Knottenbelt, 2011/05/11 11:36:35]

Question: Can the VideoCaptureSessionId specified in capture_params (and also as
the first argument of Stop()) ever be a value other than kStartOpenSessionId?

Suggestion: Move session_id out of VideoCaptureParams. Perhaps
VideoCaptureSessionId type in video_capture_types is then not necessary?

[mflodman1, 2011/05/15 20:24:00]

Yes. kStartOpenSessionId will only be used when MediaStreamManager is not used.
As a start this will be used in our tests to be able to capture video before
MediaStream has been commited to Chrome and WebKit.

We're looking at this at another patch and we're not sure how to solve this yet.
The first step is to merge the two typedefs into one:
http://codereview.chromium.org/7001017/

+             media::VideoCaptureDevice::EventHandler* video_capture_receiver);
+
+  // Stops capture device referenced by |capture_session_id|. No more frames
+  // will be delivered to the frame receiver, and |stopped_task| will be called.
+  void Stop(const media::VideoCaptureSessionId capture_session_id,
+            Task* stopped_task);
+
+  virtual ~VideoCaptureManager();
+
+  // Used by unit test to make sure a fake device is used instead of a real
+  // video capture device, due to time requirements. The function must be called

[John Knottenbelt, 2011/05/11 11:36:35]

Nit: 
device, due => device. Due
time => timing
requirements. The => requirements, the

[mflodman1, 2011/05/15 20:24:00]

Done.

+  // before EnumerateDevices and Open.
+  static void CreateTestManager();

[John Knottenbelt, 2011/05/11 11:36:35]

Nit: Move to just below static VideoCaptureManager* Get() to match order of
implementation in .cc file.

In general, the text order of the methods in the .cc file should match the text
order of the methods in the .h.

[mflodman1, 2011/05/15 20:24:00]

Done.

+  MessageLoop* GetMessageLoop();
+
+ protected:
+  // Used for testing to mock platform dependent device code.

[John Knottenbelt, 2011/05/11 11:36:35]

Nit: to => with

[mflodman1, 2011/05/15 20:24:00]

Done.

+  void UseFakeDevice();
+  static bool use_fake_device_;
+
+ private:
+  friend struct base::DefaultLazyInstanceTraits<VideoCaptureManager>;
+
+  VideoCaptureManager();
+
+  // Called by the public functions, executed on vc_device_thread_.
+  void OnEnumerateDevices();
+  void OnOpen(MediaCaptureSessionId capture_session_id,
+              const MediaCaptureDeviceInfo device);
+  void OnClose(MediaCaptureSessionId capture_session_id);
+  void OnStart(const media::VideoCaptureParams capture_params,
+               media::VideoCaptureDevice::EventHandler* video_capture_receiver);
+  void OnStop(const media::VideoCaptureSessionId capture_session_id,
+              Task* stopped_task);
+
+
+  // Executed on Browser::IO thread to call Listener.
+  void OnOpened(MediaCaptureSessionId capture_session_id);
+  void OnClosed(MediaCaptureSessionId capture_session_id);
+  void OnDevicesEnumerated(const MediaCaptureDevices& devices);
+  void OnError(MediaCaptureSessionId capture_session_id,
+               MediaStreamProviderError error);
+
+  // Executed on vc_device_thread_ to make sure Listener is called from
+  // Browser::IO thread.
+  void PostOnOpened(MediaCaptureSessionId capture_session_id);
+  void PostOnClosed(MediaCaptureSessionId capture_session_id);
+  void PostOnDevicesEnumerated(MediaCaptureDevices devices);
+  void PostOnError(MediaCaptureSessionId capture_session_id,
+                   MediaStreamProviderError error);
+
+  // Helpers
+  void GetAvailableDevices(media::VideoCaptureDevice::Names* device_names);
+  bool DeviceOpened(const media::VideoCaptureDevice::Name& device_name);
+  bool DeviceOpened(const MediaCaptureDeviceInfo& device_info);
+  bool IsOnCaptureDeviceThread() const;
+
+  // Thread for all calls to VideoCaptureDevice
+  base::Thread vc_device_thread_;
+
+  // Only accessed on IO thread

[John Knottenbelt, 2011/05/11 11:36:35]

Nit: IO => Browser::IO
(for consistency with above comments)

[mflodman1, 2011/05/15 20:24:00]

Done.

+  MediaStreamProviderListener* listener_;
+  MediaCaptureSessionId new_capture_session_id_;
+
+  // Only accessed from vc_device_thread_
+  typedef std::map<int, media::VideoCaptureDevice*> VideoCaptureDevices;

[John Knottenbelt, 2011/05/11 11:36:35]

Question: Could we use MediaCaptureSessionId instead of int for the key type of
map? I think this would be clearer.

In the implementation, I see that the devices_ is keyed on both
media::VideoCaptureSessionId and MediaCaptureSessionId, which would indicate
that they are the same concept and perhaps should be merged.

[mflodman1, 2011/05/15 20:24:00]

I've added a TODO(mflodman) for changing key type in the map when the new patch
is in and we have one type instead of two different as of today.

+  VideoCaptureDevices devices_;
+
+  DISALLOW_COPY_AND_ASSIGN(VideoCaptureManager);
+};
+
+}  // namespace media_stream
+
+DISABLE_RUNNABLE_METHOD_REFCOUNT(media_stream::VideoCaptureManager);
+
+#endif  // CONTENT_BROWSER_MEDIA_STREAM_VIDEO_CAPTURE_MANAGER_H_