Adding error signalling from device to VideocaptureManager to relay up to MediaStream and WebKit.

content/browser/renderer_host/media/video_capture_manager.cc

 // 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.
 
 #include "content/browser/renderer_host/media/video_capture_manager.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "content/browser/browser_thread.h"
 #include "media/video/capture/fake_video_capture_device.h"
 #include "media/video/capture/video_capture_device.h"
 
 namespace media_stream {
 
 // Starting id for the first capture session.
 // VideoCaptureManager::kStartOpenSessionId is used as default id without
 // explicitly calling open device.
 enum { kFirstSessionId = VideoCaptureManager::kStartOpenSessionId + 1 };
 
 static ::base::LazyInstance<VideoCaptureManager> g_video_capture_manager(
     base::LINKER_INITIALIZED);
 
 VideoCaptureManager* VideoCaptureManager::Get() {
   return g_video_capture_manager.Pointer();
 }
 
 VideoCaptureManager::VideoCaptureManager()
   : vc_device_thread_("VideoCaptureManagerThread"),
     listener_(NULL),
     new_capture_session_id_(kFirstSessionId),
     devices_(),

[Leandro Graciá Gil, 2011/06/16 17:39:53]

The default constructor will be automatically called. I don't think this is
needed.

[mflodman1, 2011/06/20 19:48:03]

Done.
This was mainly to point out it's initialized and not forgotten. I'm used to
this, but seems to not be the Chrome style :)

On 2011/06/16 17:39:53, Leandro Graciá Gil wrote:

     use_fake_device_(false) {
   vc_device_thread_.Start();
 }
 
 VideoCaptureManager::~VideoCaptureManager() {

[Leandro Graciá Gil, 2011/06/16 17:39:53]

You should delete the entries of your devices_ map, if any. Or switch to
scoped_refptr (so that they can be copied) instead of raw pointers which may
make the code simpler.

[mflodman1, 2011/06/20 19:48:03]

This should never happen, classes "above" should take care of this. I added a
DCHECK to show that is the case. OK?

On 2011/06/16 17:39:53, Leandro Graciá Gil wrote:

   vc_device_thread_.Stop();
 }
 
-bool VideoCaptureManager::Register(MediaStreamProviderListener* listener) {
+void VideoCaptureManager::Register(MediaStreamProviderListener* listener) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DCHECK(!listener_);

[John Knottenbelt, 2011/06/16 15:20:43]

Why is this check is no longer appropriate?
If it is still appropriate, would it also be appropriate to add a DCHECK in
VideoCaptureManager::Unregister?

[mflodman1, 2011/06/20 19:48:03]

Added DCHECK in both places to track possible user errors.

   listener_ = listener;
-  return true;
 }
 
 void VideoCaptureManager::Unregister() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   listener_ = NULL;
 }
 
 void VideoCaptureManager::EnumerateDevices() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(listener_);
 
   vc_device_thread_.message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this,
                         &VideoCaptureManager::OnEnumerateDevices));
 }
 
-MediaCaptureSessionId VideoCaptureManager::Open(
-    const MediaCaptureDeviceInfo& device) {
+int VideoCaptureManager::Open(const StreamDeviceInfo& device) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(listener_);
 
   // Generate a new id for this device
-  MediaCaptureSessionId video_capture_session_id = new_capture_session_id_++;
+  int video_capture_session_id = new_capture_session_id_++;
 
   vc_device_thread_.message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this,
                         &VideoCaptureManager::OnOpen,
                         video_capture_session_id,
                         device));
 
   return video_capture_session_id;
 }
 
-void VideoCaptureManager::Close(MediaCaptureSessionId capture_session_id) {
+void VideoCaptureManager::Close(int capture_session_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(listener_);
 
   vc_device_thread_.message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this,
                         &VideoCaptureManager::OnClose,
                         capture_session_id));
 }
 
@@ skipping 15 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   vc_device_thread_.message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this,
                         &VideoCaptureManager::OnStop,
                         capture_session_id,
                         stopped_task));
 }
 
+void VideoCaptureManager::Error(
+    const media::VideoCaptureSessionId capture_session_id) {
+  PostOnError(capture_session_id, kDeviceNotAvailable);
+}
+
 void VideoCaptureManager::UseFakeDevice() {
   use_fake_device_ = true;
 }
 
 MessageLoop* VideoCaptureManager::GetMessageLoop() {
   return vc_device_thread_.message_loop();
 }
 
 void VideoCaptureManager::OnEnumerateDevices() {
   DCHECK(IsOnCaptureDeviceThread());
 
   scoped_ptr<media::VideoCaptureDevice::Names> device_names(

[Leandro Graciá Gil, 2011/06/16 17:39:53]

If I'm not wrong this is a scoped_ptr to a pointer of an STL list of structures
containing strings. Are you sure you need to do this? Since you're not passing
the device_names list anywhere else and you're not allocating any initial list
size I think maybe this would be simpler and enough:

media::VideoCaptureDevice::Names device_names;
GetAvailableDevices(&device_names); // The only reason you need a pointer is
because of style conventions we pass pointers instead of non-const references.
This is just to fill the list as you know.

It should also simplify the code of the iterators and avoid cleaning up it
later.

One more thing: is there any special reason you're using STL lists instead of
STL vectors? Unless you're expecting to insert elements frequently or in
positions different to the end I think a vector would be a better choice.

[mflodman1, 2011/06/20 19:48:03]

Changed to not using scoped_ptr. This was left after some other rewriting,
thanks!


On 2011/06/16 17:39:53, Leandro Graciá Gil wrote:

       new media::VideoCaptureDevice::Names());
   GetAvailableDevices(device_names.get());
 
-  MediaCaptureDevices devices;
+  StreamDeviceInfoArray devices;
   for (media::VideoCaptureDevice::Names::iterator it =
            device_names.get()->begin(); it != device_names.get()->end(); ++it) {
     bool opened = DeviceOpened(*it);
-    devices.push_back(MediaCaptureDeviceInfo(kVideoCapture, it->device_name,
-                                             it->unique_id, opened));
+    devices.push_back(StreamDeviceInfo(kVideoCapture, it->device_name,
+                                       it->unique_id, opened));
   }
 
   PostOnDevicesEnumerated(devices);
 
   // Clean-up
   devices.clear();

[Leandro Graciá Gil, 2011/06/16 17:39:53]

I think this happens automatically with the destructor. Am I wrong?

[mflodman1, 2011/06/20 19:48:03]

Done.

You're of course right about elements being deleted by the destructor.

   device_names.get()->clear();

[Leandro Graciá Gil, 2011/06/16 17:39:53]

Independently of the comment about the type, if you use scoped_ptr objects you
should be able to use the overloaded -> operator without calling .get().

[mflodman1, 2011/06/20 19:48:03]

Removed.

 }
 
-void VideoCaptureManager::OnOpen(MediaCaptureSessionId capture_session_id,
-                                 const MediaCaptureDeviceInfo device) {
+void VideoCaptureManager::OnOpen(int capture_session_id,
+                                 const StreamDeviceInfo device) {
   DCHECK(IsOnCaptureDeviceThread());
   DCHECK(devices_.find(capture_session_id) == devices_.end());
 
   // Check if another session has already opened this device, only one user per
   // device is supported.
   if (DeviceOpened(device)) {
     PostOnError(capture_session_id, kDeviceAlreadyInUse);
     return;
   }
 
@@ skipping 11 matching lines @@
   }
   if (video_capture_device == NULL) {
     PostOnError(capture_session_id, kDeviceNotAvailable);
     return;
   }
 
   devices_[capture_session_id] = video_capture_device;
   PostOnOpened(capture_session_id);
 }
 
-void VideoCaptureManager::OnClose(
-    MediaCaptureSessionId capture_session_id) {
+void VideoCaptureManager::OnClose(int capture_session_id) {
   DCHECK(IsOnCaptureDeviceThread());
 
   VideoCaptureDevices::iterator it = devices_.find(capture_session_id);
   if (it != devices_.end()) {
     // Deallocate (if not done already) and delete the device
     media::VideoCaptureDevice* video_capture_device = it->second;
     video_capture_device->DeAllocate();
     delete video_capture_device;
     devices_.erase(it);
   }
 
   PostOnClosed(capture_session_id);
 }
 
 void VideoCaptureManager::OnStart(
     const media::VideoCaptureParams capture_params,
     media::VideoCaptureDevice::EventHandler* video_capture_receiver) {
   DCHECK(IsOnCaptureDeviceThread());
 
   // Solution for not using MediaStreamManager
   // This session id won't be returned by Open()
   if (capture_params.session_id == kStartOpenSessionId) {
     // Start() is called without using Open(), we need to open a device
     scoped_ptr<media::VideoCaptureDevice::Names> device_names(
         new media::VideoCaptureDevice::Names());
     GetAvailableDevices(device_names.get());

[John Knottenbelt, 2011/06/16 15:20:43]

Is it possible for device_names to be empty after this call?

[mflodman1, 2011/06/20 19:48:03]

Good point. Rewritten based on feedback above and calling error function if the
device list is empty.

 
-    MediaCaptureDeviceInfo device(kVideoCapture,
-                                  device_names.get()->front().device_name,
-                                  device_names.get()->front().unique_id, false);
+    StreamDeviceInfo device(kVideoCapture,
+                            device_names.get()->front().device_name,
+                            device_names.get()->front().unique_id, false);
 
     // Call OnOpen to open using the first device in the list
     OnOpen(capture_params.session_id, device);
   }
 
   VideoCaptureDevices::iterator it = devices_.find(capture_params.session_id);
   if (it == devices_.end()) {
     // Invalid session id
     video_capture_receiver->OnError();

[Leandro Graciá Gil, 2011/06/16 17:39:53]

Since you're using pointers for this a DCHECK would not harm.

[mflodman1, 2011/06/20 19:48:03]

Done, but at top of functions since the pointer is used at other places as well.

     return;
   }
   media::VideoCaptureDevice* video_capture_device = it->second;
 
   // Possible errors are signaled to video_capture_receiver by
   // video_capture_device. video_capture_receiver to perform actions.
   video_capture_device->Allocate(capture_params.width, capture_params.height,
                                  capture_params.frame_per_second,
                                  video_capture_receiver);
   video_capture_device->Start();
 }
 
 void VideoCaptureManager::OnStop(
     const media::VideoCaptureSessionId capture_session_id,
     Task* stopped_task) {
   DCHECK(IsOnCaptureDeviceThread());
 
   VideoCaptureDevices::iterator it = devices_.find(capture_session_id);
   if (it != devices_.end()) {
     media::VideoCaptureDevice* video_capture_device = it->second;
     // Possible errors are signaled to video_capture_receiver by
     // video_capture_device. video_capture_receiver to perform actions.
     video_capture_device->Stop();
     video_capture_device->DeAllocate();
   }
 
   if (stopped_task) {
     stopped_task->Run();
     delete stopped_task;

[Leandro Graciá Gil, 2011/06/16 17:39:53]

I have no idea of how this works but it looks a bit dangerous. Are you sure this
is the right way to go?

[mflodman1, 2011/06/20 19:48:03]

This was introduced when implementing OnChannelClosing in VideoCaptureHost to
signal back asap that VideoCaptureDevice has been stopped and no more captured
buffers will be sent to VideoCaptureController/VideoCaptureHost. The alternative
is to signal this in VideoCaptureDevice::EventHandler, but this way we can later
add more than one listener for every VideoCaptureDevice. The manager will then
be respomsible for stopping the device when it's really needed.

Makes sense?

The reason for putting it this wayinstead of VideoCaptureDevice signalling an
event is to have the opportunity for VideoCaptureManager to attach multiple
users of the same VideoCaptureDevice.

   }
 
   if (capture_session_id == kStartOpenSessionId) {
     // This device was opened from Start(), not Open(). Close it!
     OnClose(capture_session_id);
   }
 }
 
-void VideoCaptureManager::OnOpened(
-    MediaCaptureSessionId capture_session_id) {
+void VideoCaptureManager::OnOpened(int capture_session_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (listener_ == NULL) {
     // Listener has been removed
     return;
   }
   listener_->Opened(kVideoCapture, capture_session_id);
 }
 
-void VideoCaptureManager::OnClosed(
-    MediaCaptureSessionId capture_session_id) {
+void VideoCaptureManager::OnClosed(int capture_session_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (listener_ == NULL) {
     // Listener has been removed
     return;
   }
   listener_->Closed(kVideoCapture, capture_session_id);
 }
 
 void VideoCaptureManager::OnDevicesEnumerated(
-    const MediaCaptureDevices& devices) {
+    const StreamDeviceInfoArray& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (listener_ == NULL) {

[Leandro Graciá Gil, 2011/06/16 17:39:53]

I think style says we should use if (!listener_) in these situations, but I'm
not completely sure. Can you check it?

[mflodman1, 2011/06/20 19:48:03]

Done.

     // Listener has been removed
     return;
   }
   listener_->DevicesEnumerated(kVideoCapture, devices);
 }
 
-void VideoCaptureManager::OnError(MediaCaptureSessionId capture_session_id,
+void VideoCaptureManager::OnError(int capture_session_id,
                                   MediaStreamProviderError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (listener_ == NULL) {
     // Listener has been removed
     return;
   }
   listener_->Error(kVideoCapture, capture_session_id, error);
 }
 
-void VideoCaptureManager::PostOnOpened(
-    MediaCaptureSessionId capture_session_id) {
+void VideoCaptureManager::PostOnOpened(int capture_session_id) {
   DCHECK(IsOnCaptureDeviceThread());
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
                           NewRunnableMethod(this,
                                             &VideoCaptureManager::OnOpened,
                                             capture_session_id));
 }
 
-void VideoCaptureManager::PostOnClosed(
-    MediaCaptureSessionId capture_session_id) {
+void VideoCaptureManager::PostOnClosed(int capture_session_id) {
   DCHECK(IsOnCaptureDeviceThread());
   BrowserThread::PostTask(BrowserThread::IO,
                             FROM_HERE,
                             NewRunnableMethod(this,
                                               &VideoCaptureManager::OnClosed,
                                               capture_session_id));
 }
 
-void VideoCaptureManager::PostOnDevicesEnumerated(MediaCaptureDevices devices) {
+void VideoCaptureManager::PostOnDevicesEnumerated(
+    StreamDeviceInfoArray devices) {

[Leandro Graciá Gil, 2011/06/16 17:39:53]

Looks like this could be a const reference to me. You are passing an STL vector
from the same thread when calling this.

[mflodman1, 2011/06/20 19:48:03]

Done.

   DCHECK(IsOnCaptureDeviceThread());
-
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
                           NewRunnableMethod(
                               this,
                               &VideoCaptureManager::OnDevicesEnumerated,
                               devices));
 }
 
-void VideoCaptureManager::PostOnError(MediaCaptureSessionId capture_session_id,
+void VideoCaptureManager::PostOnError(int capture_session_id,
                                       MediaStreamProviderError error) {
   // Don't check thread here, can be called from both IO thread and device
   // thread.
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
                           NewRunnableMethod(this,
                                             &VideoCaptureManager::OnError,
                                             capture_session_id,
                                             error));
 }
@@ skipping 21 matching lines @@
       it != devices_.end();
       ++it) {
     if (device_name.unique_id == it->second->device_name().unique_id) {
       // We've found the device!
       return true;
     }
   }
   return false;
 }
 
-bool VideoCaptureManager::DeviceOpened(
-    const MediaCaptureDeviceInfo& device_info) {
+bool VideoCaptureManager::DeviceOpened(const StreamDeviceInfo& device_info) {
   DCHECK(IsOnCaptureDeviceThread());
 
   for (VideoCaptureDevices::iterator it = devices_.begin();
       it != devices_.end();
       it++) {
     if (device_info.device_id == it->second->device_name().unique_id) {

[Leandro Graciá Gil, 2011/06/16 17:39:53]

Looks like this if doesn't need braces according to the style guidelines.

[mflodman1, 2011/06/20 19:48:03]

We've received comments regarding this that either way is fine, as long as it's
consistent. We chose this way in, to encapsulate it in case of comments like in
the function above.
I'll keep it this way if there are no strong opinions about it, but I can change
if that is preferred :)

       return true;
     }
   }
   return false;
 }
 
 }  // namespace media
+

[scherkus (not reviewing), 2011/06/17 03:03:52]

nit: get rid of blank line

[mflodman1, 2011/06/20 19:48:03]

Done.