Added AVFoundation Glue and Device Monitoring for Mac.

content/browser/device_monitor_mac.mm

Index: content/browser/device_monitor_mac.mm
diff --git a/content/browser/device_monitor_mac.mm b/content/browser/device_monitor_mac.mm
index 7b12e91cbdf4ed8b57fc020d3b75c0579240d0ef..8878494696b1275d606f30dd2a7a5a17ccd66fe7 100644
--- a/content/browser/device_monitor_mac.mm
+++ b/content/browser/device_monitor_mac.mm
@@ -7,56 +7,131 @@
 #import <QTKit/QTKit.h>
 
 #include "base/logging.h"
+#include "media/video/capture/mac/avfoundation_glue.h"
 
-namespace content {
+namespace {
+
+// This class is used to keep track of system devices names and their types.
+class DeviceInfo {

[Robert Sesek, 2013/10/14 17:26:44]

If you're going to have public members, this needs to be a struct and the
members should not have trailing _

[mcasas, 2013/10/15 11:50:11]

I changed them to private and provided getters/accessors, i.e. unique_id_ -->
unique_id()

 
-class DeviceMonitorMac::QTMonitorImpl {
  public:
-  explicit QTMonitorImpl(DeviceMonitorMac* monitor);
-  virtual ~QTMonitorImpl() {}
+  enum DeviceType {
+    kAudio,
+    kVideo,
+    kMuxed
+  };
+
+  DeviceInfo(std::string unique_id, DeviceType type)
+      : unique_id_(unique_id), type_(type) {}
+
+  // Operator== is needed here to use this class in a std::vector.
+  bool operator==(const DeviceInfo& device) const {
+    return unique_id_ == device.unique_id_;
+  }
+
+  std::string unique_id_;
+  DeviceType type_;
+  // Allow generated copy constructor and assignment.
+};
 
-  void Start();
-  void Stop();
+// Base class used by DeviceMonitorMac to interact with either a QTKit or an
+// AVFoundation implementation of events and notifications. This class should
+// never be instantiated on its own.
+class MacMonitor {
+ public:
+  MacMonitor() {}
+  explicit MacMonitor(content::DeviceMonitorMac* monitor)
+      : monitor_(monitor),
+        cached_devices_(),
+        device_arrival_(nil),
+        device_removal_(nil) {}
+  virtual ~MacMonitor() {}
 
- private:
-  void OnDeviceChanged();
+  virtual void OnDeviceChanged() = 0;
+  void ConsolidateDevicesListAndNotify(
+      std::vector<DeviceInfo> snapshot_devices);
 
-  DeviceMonitorMac* monitor_;
-  int number_audio_devices_;
-  int number_video_devices_;
+ protected:
+  content::DeviceMonitorMac* monitor_;
+  std::vector<DeviceInfo> cached_devices_;
   id device_arrival_;
   id device_removal_;
+  DISALLOW_COPY_AND_ASSIGN(MacMonitor);
+};
+
+void MacMonitor::ConsolidateDevicesListAndNotify(
+    std::vector<DeviceInfo> snapshot_devices) {
+  int video_device_added = 0;

[Robert Sesek, 2013/10/14 17:26:44]

Why are these ints? It seems like you're just using them as bools.

[mcasas, 2013/10/15 11:50:11]

Since several devices might have been plugged in/out at once, I'm using int for
an easier syntax += (lines 78, 80 etc).

It's not strictly necessary, but at the end bool will be implemented as a native
integer type (32/64b), so I saw no performance gain.

[Robert Sesek, 2013/10/15 20:44:46]

What about bool and |= ? This isn't really a question of performance, but rather
of code comprehension: you're counting integers when you are using it just for
testing existence.

[mcasas, 2013/10/16 11:04:40]

Done. Boolean it is.

+  int audio_device_added = 0;
+  int video_device_removed = 0;
+  int audio_device_removed = 0;
+
+  // Compare the current system devices snapshot with the ones cached to detect
+  // additions, present in the former but not in the latter. If we find a device
+  // in snapshot_devices entry also present in cached_devices, we remove it from
+  // the latter vector.
+  std::vector<DeviceInfo>::iterator it;
+  for (it = snapshot_devices.begin(); it != snapshot_devices.end(); ++it) {
+    std::vector<DeviceInfo>::iterator cached_devices_iterator =
+        std::find(cached_devices_.begin(), cached_devices_.end(), *it);
+    if (cached_devices_iterator == cached_devices_.end()) {
+      video_device_added += ((it->type_ == DeviceInfo::kVideo) ||
+                             (it->type_ == DeviceInfo::kMuxed));
+      audio_device_added += ((it->type_ == DeviceInfo::kAudio) ||
+                             (it->type_ == DeviceInfo::kMuxed));
+      DVLOG(1) << "Device has been added, id: " << it->unique_id_;
+    } else {
+      cached_devices_.erase(cached_devices_iterator);
+    }
+  }
+  // All the remaining entries in cached_devices are removed devices.
+  for (it = cached_devices_.begin(); it != cached_devices_.end(); ++it) {
+    video_device_removed += ((it->type_ == DeviceInfo::kVideo) ||
+                             (it->type_ == DeviceInfo::kMuxed));
+    audio_device_removed += ((it->type_ == DeviceInfo::kAudio) ||
+                             (it->type_ == DeviceInfo::kMuxed));
+    DVLOG(1) << "Device has been removed, id: " << it->unique_id_;
+  }
+  // Update the cached devices with the current system snapshot.
+  cached_devices_.swap(snapshot_devices);
+
+  if (video_device_added || video_device_removed)
+    monitor_->NotifyDeviceChanged(base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE);
+  if (audio_device_added || video_device_removed)
+    monitor_->NotifyDeviceChanged(base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE);
+}
 
-  DISALLOW_COPY_AND_ASSIGN(QTMonitorImpl);
+class QTKitMonitorImpl : public MacMonitor {
+ public:
+  QTKitMonitorImpl() {}

[Robert Sesek, 2013/10/14 17:26:44]

Why do you need to provide this constructor?

[mcasas, 2013/10/15 11:50:11]

No need - removed and also AVFoundationMonitorImpl() {}

+  explicit QTKitMonitorImpl(content::DeviceMonitorMac* monitor);
+  virtual ~QTKitMonitorImpl();
+
+  virtual void OnDeviceChanged() OVERRIDE;
 };
 
-DeviceMonitorMac::QTMonitorImpl::QTMonitorImpl(DeviceMonitorMac* monitor)
-    : monitor_(monitor),
-      number_audio_devices_(0),
-      number_video_devices_(0),
-      device_arrival_(nil),
-      device_removal_(nil) {
+QTKitMonitorImpl::QTKitMonitorImpl(content::DeviceMonitorMac* monitor) {
   DCHECK(monitor);

[Robert Sesek, 2013/10/14 17:26:44]

You DCHECK this but it is unused?

[mcasas, 2013/10/15 11:50:11]

Moved it to the MacMonitor parent class constructor, there it should be (at
least) DCHECKed for NULLness.

-}
 
-void DeviceMonitorMac::QTMonitorImpl::Start() {
   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
   device_arrival_ =
     [nc addObserverForName:QTCaptureDeviceWasConnectedNotification
                     object:nil
                      queue:nil
                 usingBlock:^(NSNotification* notification) {
-                    OnDeviceChanged();}];
+    OnDeviceChanged();

[Robert Sesek, 2013/10/14 17:26:44]

These blocks are not formatted correctly. The contents should be indented four
spaces and the closing brace should align with the line that has the opening
brace. More details:

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block...

[mcasas, 2013/10/15 11:50:11]

Done.

[Robert Sesek, 2013/10/15 20:44:46]

Not quite. Look at the fourth and fifth examples. A new line should exist before
the } and it should be indented to align underneath the 'u'.

[mcasas, 2013/10/16 11:04:40]

I hope I got it right this time :) Interestingly, it was incorrect in the
original code.

[Robert Sesek, 2013/10/16 23:58:07]

Yes, is good now. I don't think a Mac person reviewed the initial code :).

+  }];
 
   device_removal_ =
       [nc addObserverForName:QTCaptureDeviceWasDisconnectedNotification
                       object:nil
                        queue:nil
                   usingBlock:^(NSNotification* notification) {
-                      OnDeviceChanged();}];
+    OnDeviceChanged();
+  }];
 }
 
-void DeviceMonitorMac::QTMonitorImpl::Stop() {
+QTKitMonitorImpl::~QTKitMonitorImpl() {
   if (!monitor_)

[Robert Sesek, 2013/10/14 17:26:44]

Couldn't this leave dangling observers? You do not conditionally add them in the
ctor.

[mcasas, 2013/10/15 11:50:11]

Most likely. RIght now I don't see why a not initialised monitor_ -- just
DCHECK()'ed on constructor, could possibly prevent deregistering the observers.
This problem possibly never surfaced because the DeviceMonitorMac is a singleton
lasting as long as the browser process itself. I'm removing the if(!monitor)
checks.

     return;
 
@@ -65,39 +140,115 @@ void DeviceMonitorMac::QTMonitorImpl::Stop() {
   [nc removeObserver:device_removal_];
 }
 
-void DeviceMonitorMac::QTMonitorImpl::OnDeviceChanged() {
+void QTKitMonitorImpl::OnDeviceChanged() {
+  std::vector<DeviceInfo> snapshot_devices;
+  DeviceInfo::DeviceType device_info;
+
   NSArray* devices = [QTCaptureDevice inputDevices];
-  int number_video_devices = 0;
-  int number_audio_devices = 0;
   for (QTCaptureDevice* device in devices) {
-    if ([device hasMediaType:QTMediaTypeVideo] ||
-        [device hasMediaType:QTMediaTypeMuxed])
-      ++number_video_devices;
+    if ([device hasMediaType:QTMediaTypeVideo])
+      device_info = DeviceInfo::kVideo;
+    else if ([device hasMediaType:QTMediaTypeMuxed])
+      device_info = DeviceInfo::kMuxed;
+    else if ([device hasMediaType:QTMediaTypeSound])
+      device_info = DeviceInfo::kAudio;
 
-    if ([device hasMediaType:QTMediaTypeSound] ||
-        [device hasMediaType:QTMediaTypeMuxed])
-      ++number_audio_devices;
+    snapshot_devices.push_back(
+        DeviceInfo([[device uniqueID] UTF8String], device_info));
   }
 
-  if (number_video_devices_ != number_video_devices) {
-    number_video_devices_ = number_video_devices;
-    monitor_->NotifyDeviceChanged(base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE);
-  }
+  ConsolidateDevicesListAndNotify(snapshot_devices);
+}
 
-  if (number_audio_devices_ != number_audio_devices) {
-    number_audio_devices_ = number_audio_devices;
-    monitor_->NotifyDeviceChanged(base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE);
+class AVFoundationMonitorImpl : public MacMonitor {
+ public:
+  AVFoundationMonitorImpl() {}
+  explicit AVFoundationMonitorImpl(content::DeviceMonitorMac* monitor);
+  virtual ~AVFoundationMonitorImpl();
+
+  virtual void OnDeviceChanged() OVERRIDE;
+};
+
+AVFoundationMonitorImpl::AVFoundationMonitorImpl(
+    content::DeviceMonitorMac* monitor) {
+  DCHECK(monitor);
+
+  NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+  NSOperationQueue* mainQueue = [NSOperationQueue mainQueue];

[Robert Sesek, 2013/10/14 17:26:44]

naming: main_queue

[mcasas, 2013/10/15 11:50:11]

Done.

+
+  device_arrival_ =
+      [nc addObserverForName:AVFoundationGlue::
+      avCaptureDeviceWasConnectedNotification()
+                      object:nil
+                       queue:mainQueue
+                  usingBlock:^(NSNotification* notification) {
+    OnDeviceChanged();
+  }];
+  DCHECK(device_arrival_);
+   device_removal_ =
+      [nc addObserverForName:AVFoundationGlue::
+      avCaptureDeviceWasDisconnectedNotification()
+                      object:nil
+                       queue:mainQueue
+                  usingBlock:^(NSNotification* notification) {
+     OnDeviceChanged();
+   }];
+   DCHECK(device_removal_);
+}
+
+AVFoundationMonitorImpl::~AVFoundationMonitorImpl() {
+  if (!monitor_)
+    return;
+
+  NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+  [nc removeObserver:device_arrival_];
+  [nc removeObserver:device_removal_];
+}
+
+void AVFoundationMonitorImpl::OnDeviceChanged() {
+  std::vector<DeviceInfo> snapshot_devices;
+  DeviceInfo::DeviceType device_info;
+
+  NSArray* devices = [AVCaptureDeviceGlue devices];
+  for (CrAVCaptureDevice* device in devices) {
+    if ([AVCaptureDeviceGlue hasMediaType:AVFoundationGlue::avMediaTypeVideo()
+                         forCaptureDevice:device]) {
+      device_info = DeviceInfo::kVideo;
+    } else if ([AVCaptureDeviceGlue
+                   hasMediaType:AVFoundationGlue::avMediaTypeMuxed()
+               forCaptureDevice:device]) {
+      device_info = DeviceInfo::kMuxed;
+    } else if ([AVCaptureDeviceGlue
+                   hasMediaType:AVFoundationGlue::avMediaTypeAudio()
+               forCaptureDevice:device]) {
+      device_info = DeviceInfo::kAudio;
+    }
+    snapshot_devices.push_back(DeviceInfo(
+        [[AVCaptureDeviceGlue uniqueID:device] UTF8String], device_info));
   }
+
+  ConsolidateDevicesListAndNotify(snapshot_devices);
 }
 
+}  // namespace
+
+namespace content {
+
 DeviceMonitorMac::DeviceMonitorMac() {
-  qt_monitor_.reset(new QTMonitorImpl(this));
-  qt_monitor_->Start();
+  if (AVFoundationGlue::isAVFoundationSupported()) {
+    DVLOG(1) << "Monitoring via AVFoundation";
+    device_monitor_impl_.reset(new AVFoundationMonitorImpl(this));
+    // Force the device enumeration so we enumerate correctly devices already in
+    // the system and at the same time use the AVCaptureDeviceGlue so it in
+    // turn forces the AVCaptureDeviceGlue alloc-init.
+    device_monitor_impl_->OnDeviceChanged();
+  } else {
+    DVLOG(1) << "Monitoring via QTKit";
+    device_monitor_impl_.reset(new QTKitMonitorImpl(this));
+  }
 }
 
-DeviceMonitorMac::~DeviceMonitorMac() {
-  qt_monitor_->Stop();
-}
+DeviceMonitorMac::~DeviceMonitorMac() {}
 
 void DeviceMonitorMac::NotifyDeviceChanged(
     base::SystemMonitor::DeviceType type) {