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..02247b875d63ea33a27ca9857431bd0a1e81dc8b 100644
--- a/content/browser/device_monitor_mac.mm
+++ b/content/browser/device_monitor_mac.mm
@@ -7,48 +7,57 @@
 #import <QTKit/QTKit.h>
 
 #include "base/logging.h"
+#include "media/video/capture/mac/avfoundation_glue.h"
 
 namespace content {
 
-class DeviceMonitorMac::QTMonitorImpl {
+// Interface used by DeviceMonitorMac to interact with either a QTKit or an
+// AVFoundation implementation of events and notifications.
+class DeviceMonitorMac::MacMonitorInterface {
  public:
-  explicit QTMonitorImpl(DeviceMonitorMac* monitor);
-  virtual ~QTMonitorImpl() {}
+  MacMonitorInterface() {};
+  MacMonitorInterface(DeviceMonitorMac* monitor)

[Mark Mentovai, 2013/09/26 17:09:10]

This constructor should be explicit.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Explic...

[mcasas, 2013/09/30 17:53:50]

Done.

+      : monitor_(monitor),
+        number_audio_devices_(0),
+        number_video_devices_(0),
+        device_arrival_(nil),

[Mark Mentovai, 2013/09/26 17:09:10]

nil is an initializer for an Objective-C object pointer, but device_arrival_ and
device_removal_ are both ints, so you should initialize them with 0.

[mcasas, 2013/09/30 17:53:50]

Done.

+        device_removal_(nil) {};
+  virtual ~MacMonitorInterface() = 0;
+
+ protected:
+   virtual void OnDeviceChanged() = 0;

[Mark Mentovai, 2013/09/26 17:09:10]

Weird indentation in this entire protected section.

[mcasas, 2013/09/30 17:53:50]

Done.

+
+   DeviceMonitorMac* monitor_;

[Mark Mentovai, 2013/09/26 17:09:10]

Protected data is almost unseen, even in interface cases like this one.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
and
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

I recommend making MacMonitorInterface a pure interface class. Remove all of the
data and track it all in the subclasses. As it stands now, there is no logic in
MacMonitorInterface that actually uses or maintains any of this data.

[mcasas, 2013/09/30 17:53:50]

Done.

+   int number_audio_devices_;
+   int number_video_devices_;
+   int device_arrival_;
+   int device_removal_;
 
-  void Start();
-  void Stop();
+};
 
- private:
-  void OnDeviceChanged();
+DeviceMonitorMac::MacMonitorInterface::~MacMonitorInterface() { }

[Mark Mentovai, 2013/09/26 17:09:10]

In the class declaration above, you made the destructor pure virtual, so what’s
this for?

[mcasas, 2013/09/30 17:53:50]

Gone.

 
-  DeviceMonitorMac* monitor_;
-  int number_audio_devices_;
-  int number_video_devices_;
-  id device_arrival_;
-  id device_removal_;
+class DeviceMonitorMac::QTKitMonitorImpl : public MacMonitorInterface {
+ public:
+  QTKitMonitorImpl(DeviceMonitorMac* monitor);

[Mark Mentovai, 2013/09/26 17:09:10]

explicit.

[mcasas, 2013/09/30 17:53:50]

Done.

+  virtual ~QTKitMonitorImpl();
 
-  DISALLOW_COPY_AND_ASSIGN(QTMonitorImpl);
+  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) {
+DeviceMonitorMac::QTKitMonitorImpl::QTKitMonitorImpl(DeviceMonitorMac* monitor)
+    : MacMonitorInterface(monitor) {
   DCHECK(monitor);
-}
 
-void DeviceMonitorMac::QTMonitorImpl::Start() {
   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
-  device_arrival_ =
+  device_arrival_ = (int)

[Mark Mentovai, 2013/09/26 17:09:10]

No, you definitely can’t cast id to int. I don’t know why you’d even try, since
you only ever seem to use it as an id.

id is a pointer type. Aside from generally not wanting to mix pointer types with
integer types for sanity’s sake, in the 64-bit environment, id will be 64 bits
wide, and the cast to int will truncate it.

This comment applies throughout the file.

[mcasas, 2013/09/30 17:53:50]

Without wanting to escape my public defamation, I'd like to say in my defense
that (most of) this code comes from somewhere else...

     [nc addObserverForName:QTCaptureDeviceWasConnectedNotification
                     object:nil
                      queue:nil
                 usingBlock:^(NSNotification* notification) {
                     OnDeviceChanged();}];
 
-  device_removal_ =
+  device_removal_ = (int)
       [nc addObserverForName:QTCaptureDeviceWasDisconnectedNotification
                       object:nil
                        queue:nil
@@ -56,16 +65,16 @@ void DeviceMonitorMac::QTMonitorImpl::Start() {
                       OnDeviceChanged();}];
 }
 
-void DeviceMonitorMac::QTMonitorImpl::Stop() {
+DeviceMonitorMac::QTKitMonitorImpl::~QTKitMonitorImpl() {
   if (!monitor_)
     return;
 
   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
-  [nc removeObserver:device_arrival_];
-  [nc removeObserver:device_removal_];
+  [nc removeObserver:(id)device_arrival_];
+  [nc removeObserver:(id)device_removal_];
 }
 
-void DeviceMonitorMac::QTMonitorImpl::OnDeviceChanged() {
+void DeviceMonitorMac::QTKitMonitorImpl::OnDeviceChanged() {
   NSArray* devices = [QTCaptureDevice inputDevices];
   int number_video_devices = 0;
   int number_audio_devices = 0;
@@ -90,15 +99,80 @@ void DeviceMonitorMac::QTMonitorImpl::OnDeviceChanged() {
   }
 }
 
-DeviceMonitorMac::DeviceMonitorMac() {
-  qt_monitor_.reset(new QTMonitorImpl(this));
-  qt_monitor_->Start();
+class DeviceMonitorMac::AVFoundationMonitorImpl : public MacMonitorInterface {
+ public:
+  AVFoundationMonitorImpl(DeviceMonitorMac* monitor);

[Mark Mentovai, 2013/09/26 17:09:10]

explicit

[mcasas, 2013/09/30 17:53:50]

Done.

+  virtual ~AVFoundationMonitorImpl();
+
+  virtual void OnDeviceChanged() OVERRIDE;
+};
+
+DeviceMonitorMac::AVFoundationMonitorImpl::AVFoundationMonitorImpl(
+  DeviceMonitorMac* monitor)

[Mark Mentovai, 2013/09/26 17:09:10]

This line would be a 4-space continuation line indent. (And the next line can
stay as-is.)

[mcasas, 2013/09/30 17:53:50]

Done.

+    : MacMonitorInterface(monitor) {
+  DCHECK(monitor);
+
+  NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+  device_arrival_ = (int)
+    [nc addObserverForName:AVCaptureDeviceWasConnectedNotification
+                    object:nil
+                     queue:nil
+                usingBlock:^(NSNotification* notification) {
+                    OnDeviceChanged();}];
+
+  device_removal_ = (int)
+      [nc addObserverForName:AVCaptureDeviceWasDisconnectedNotification
+                      object:nil
+                       queue:nil
+                  usingBlock:^(NSNotification* notification) {
+                      OnDeviceChanged();}];
 }
 
-DeviceMonitorMac::~DeviceMonitorMac() {
-  qt_monitor_->Stop();
+DeviceMonitorMac::AVFoundationMonitorImpl::~AVFoundationMonitorImpl() {
+  if (!monitor_)
+    return;
+
+  NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
+  [nc removeObserver:(id)device_arrival_];
+  [nc removeObserver:(id)device_removal_];
 }
 
+void DeviceMonitorMac::AVFoundationMonitorImpl::OnDeviceChanged() {
+  NSArray* devices = [AVCaptureDeviceGlue devices];
+  int number_video_devices = 0;
+  int number_audio_devices = 0;
+  for (AVCaptureDeviceGlue* device in devices) {

[Mark Mentovai, 2013/09/26 17:09:10]

Is it possible for the actual devices to change without the number of devices
changing?

Even if you’re getting one notification per connection/disconnection, it’s
possible that your notification is not processed until after other events happen
on the system. The state of your processing is not necessarily synchronized with
the state of the system at the time the notification was generated. If one video
device is disconnected and a different one is subsequently connected, and you
don’t process either notification until after the second one was connected,
number_video_devices will never appear to change, but the devices attached to
the system were definitely changed. Your processing the first disconnect
notification races against the second connect event.

The QTKit code above shares this problem.

[mcasas, 2013/09/30 17:53:50]

I thought this is addressed by the "usingBlock" in the addObserverForName, that
would make the OnDeviceChanged() run on a critical section and with no race
conditions? 

+    if ([device hasMediaType:AVMediaTypeVideo] ||
+        [device hasMediaType:AVMediaTypeMuxed])
+      ++number_video_devices;
+
+    if ([device hasMediaType:AVMediaTypeAudio] ||
+        [device hasMediaType:AVMediaTypeMuxed])
+      ++number_audio_devices;
+  }
+
+  if (number_video_devices_ != number_video_devices) {
+    number_video_devices_ = number_video_devices;
+    monitor_->NotifyDeviceChanged(base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE);
+  }
+
+  if (number_audio_devices_ != number_audio_devices) {
+    number_audio_devices_ = number_audio_devices;
+    monitor_->NotifyDeviceChanged(base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE);
+  }
+}
+
+DeviceMonitorMac::DeviceMonitorMac() {
+  if ([AVFoundationGlue IsAVFoundationSupported]){

[Mark Mentovai, 2013/09/26 17:09:10]

Method names (even class method names, what would be a class-static function in
C++) in Objective-C are always named with a leading lowercase letter.
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml#Objective-C_M...

[mcasas, 2013/09/30 17:53:50]

Happy to do so. I was following the style in:

IsOSLionOrLater()

:(

+    device_monitor_impl_.reset(new AVFoundationMonitorImpl(this));
+  } else {
+    DVLOG(1) << "Monitoring via QTKit";
+    device_monitor_impl_.reset(new QTKitMonitorImpl(this));
+  }
+}
+
+DeviceMonitorMac::~DeviceMonitorMac() {}
+
 void DeviceMonitorMac::NotifyDeviceChanged(
     base::SystemMonitor::DeviceType type) {
   // TODO(xians): Remove the global variable for SystemMonitor.