Added AVFoundation Glue and Device Monitoring for Mac.

content/browser/device_monitor_mac.mm

 // Copyright (c) 2012 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/device_monitor_mac.h"
 
 #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;
 
-  void Start();
-  void Stop();
+ 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.

 
- private:
-  void OnDeviceChanged();
+   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_;
 
-  DeviceMonitorMac* monitor_;
-  int number_audio_devices_;
-  int number_video_devices_;
-  id device_arrival_;
-  id device_removal_;
-
-  DISALLOW_COPY_AND_ASSIGN(QTMonitorImpl);
 };
 
-DeviceMonitorMac::QTMonitorImpl::QTMonitorImpl(DeviceMonitorMac* monitor)
-    : monitor_(monitor),
-      number_audio_devices_(0),
-      number_video_devices_(0),
-      device_arrival_(nil),
-      device_removal_(nil) {
+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.

+
+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();
+
+  virtual void OnDeviceChanged() OVERRIDE;
+};
+
+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
                   usingBlock:^(NSNotification* notification) {
                       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;
   for (QTCaptureDevice* device in devices) {
     if ([device hasMediaType:QTMediaTypeVideo] ||
         [device hasMediaType:QTMediaTypeMuxed])
       ++number_video_devices;
 
     if ([device hasMediaType:QTMediaTypeSound] ||
         [device hasMediaType:QTMediaTypeMuxed])
       ++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);
   }
 }
 
+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::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() {
-  qt_monitor_.reset(new QTMonitorImpl(this));
-  qt_monitor_->Start();
+  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() {
-  qt_monitor_->Stop();
-}
+DeviceMonitorMac::~DeviceMonitorMac() {}
 
 void DeviceMonitorMac::NotifyDeviceChanged(
     base::SystemMonitor::DeviceType type) {
   // TODO(xians): Remove the global variable for SystemMonitor.
   base::SystemMonitor::Get()->ProcessDevicesChanged(type);
 }
 
 }  // namespace content