DeviceMonitorMac: move CrAVFoundationDeviceObserver and most of SuspendObserverDelegate to UI Thread

content/browser/device_monitor_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Robert Sesek, 2014/07/07 22:47:50]

Unless QTKit is going away soon, I think it may make sense to separate out the
two backends from this file. It would probably make it easier to follow. (In
another CL).

[mcasas, 2014/07/08 15:11:45]

Total +1 !!
http://crbug.com/392076

 // 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 <set>
 
 #include "base/bind_helpers.h"
 #include "base/logging.h"
+#include "base/mac/bind_objc_block.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/threading/thread_checker.h"
 #include "content/public/browser/browser_thread.h"
+#include "media/base/bind_to_current_loop.h"
 #import "media/video/capture/mac/avfoundation_glue.h"
 
 namespace {
 
 // This class is used to keep track of system devices names and their types.
 class DeviceInfo {
  public:
   enum DeviceType {
     kAudio,
     kVideo,
@@ skipping 14 matching lines @@
 
   const std::string& unique_id() const { return unique_id_; }
   DeviceType type() const { return type_; }
 
  private:
   std::string unique_id_;
   DeviceType type_;
   // Allow generated copy constructor and assignment.
 };
 
+class CrAVCaptureDeviceRefCounted :

[Robert Sesek, 2014/07/07 22:47:50]

I don't understand the point of this class. CrAVCaptureDevice is already
thread-safely refcounted by being a NSObject.

[mcasas, 2014/07/08 15:11:46]

Gone; you're right it wasn't needed.

+    public base::RefCountedThreadSafe<CrAVCaptureDeviceRefCounted> {
+ public:
+  explicit CrAVCaptureDeviceRefCounted(CrAVCaptureDevice* device)
+      : device_(device) {}
+ private:
+  friend class base::RefCountedThreadSafe<CrAVCaptureDeviceRefCounted>;
+  ~CrAVCaptureDeviceRefCounted() {};
+
+  CrAVCaptureDevice* device_;
+};
+
 // Base abstract class used by DeviceMonitorMac to interact with either a QTKit
 // or an AVFoundation implementation of events and notifications.
 class DeviceMonitorMacImpl {
  public:
   explicit DeviceMonitorMacImpl(content::DeviceMonitorMac* monitor)
       : monitor_(monitor),
         cached_devices_(),
         device_arrival_(nil),
         device_removal_(nil) {
     DCHECK(monitor);
@@ skipping 149 matching lines @@
   ConsolidateDevicesListAndNotify(snapshot_devices);
 }
 
 // Forward declaration for use by CrAVFoundationDeviceObserver.
 class SuspendObserverDelegate;
 
 }  // namespace
 
 // This class is a Key-Value Observer (KVO) shim. It is needed because C++
 // classes cannot observe Key-Values directly. Created, manipulated, and
-// destroyed on the Device Thread by SuspendedObserverDelegate.
+// destroyed on the UI Thread by SuspendObserverDelegate.
 @interface CrAVFoundationDeviceObserver : NSObject {
  @private
-  SuspendObserverDelegate* receiver_;  // weak
+  base::Closure onDeviceChangedCallback_;

[Robert Sesek, 2014/07/07 22:47:49]

nit: blank line after, and a comment (e.g. on what thread is this run?)

[mcasas, 2014/07/08 15:11:45]

Done.

   // Member to keep track of the devices we are already monitoring.
   std::set<CrAVCaptureDevice*> monitoredDevices_;
 }
 
-- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver;
+- (id)initWithOnChangedCallback:(const base::Closure&)callback;
 - (void)startObserving:(CrAVCaptureDevice*)device;
 - (void)stopObserving:(CrAVCaptureDevice*)device;
 
 @end
 
 namespace {
 
 // This class owns and manages the lifetime of a CrAVFoundationDeviceObserver.
-// Provides a callback for this device observer to indicate that there has been
-// a device change of some kind. Created by AVFoundationMonitorImpl in UI thread
-// but living in Device Thread.
+// AVFoundationMonitorImpl creates and destroys it in UI thread. Runs the
+// expensive device enumerations in OnDeviceChanged() and StartObserver() on
+// Device Thread.
 class SuspendObserverDelegate :
     public base::RefCountedThreadSafe<SuspendObserverDelegate> {
  public:
   explicit SuspendObserverDelegate(DeviceMonitorMacImpl* monitor)
       : avfoundation_monitor_impl_(monitor) {

[Robert Sesek, 2014/07/07 22:47:49]

Please move the ctor and dtor out of line, like the rest of the methods.

[mcasas, 2014/07/08 15:11:46]

Done.

+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     device_thread_checker_.DetachFromThread();
   }
 
   void OnDeviceChanged();
   void StartObserver();
   void ResetDeviceMonitorOnUIThread();
 
  private:
   friend class base::RefCountedThreadSafe<SuspendObserverDelegate>;
 
-  virtual ~SuspendObserverDelegate() {}
+  virtual ~SuspendObserverDelegate() {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[Robert Sesek, 2014/07/07 22:47:49]

You could add a |using content::BrowserThread| if you wanted.

[mcasas, 2014/07/08 15:11:46]

Great, less verbosity, done.

+  }
 
   void OnDeviceChangedOnUIThread(
       const std::vector<DeviceInfo>& snapshot_devices);
 
   base::ThreadChecker device_thread_checker_;
+  // Created, used and released in UI thread.
   base::scoped_nsobject<CrAVFoundationDeviceObserver> suspend_observer_;
   DeviceMonitorMacImpl* avfoundation_monitor_impl_;
 };
 
 void SuspendObserverDelegate::OnDeviceChanged() {
   DCHECK(device_thread_checker_.CalledOnValidThread());
-  NSArray* devices = [AVCaptureDeviceGlue devices];
   std::vector<DeviceInfo> snapshot_devices;
-  for (CrAVCaptureDevice* device in devices) {
-    [suspend_observer_ startObserving:device];
+  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices]) {
+    scoped_refptr<CrAVCaptureDeviceRefCounted> device_refptr =
+        new CrAVCaptureDeviceRefCounted(device);
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::BindBlock(^{ [suspend_observer_ startObserving:device]; }));
     BOOL suspended = [device respondsToSelector:@selector(isSuspended)] &&
         [device isSuspended];
     DeviceInfo::DeviceType device_type = DeviceInfo::kUnknown;
     if ([device hasMediaType:AVFoundationGlue::AVMediaTypeVideo()]) {
       if (suspended)
         continue;
       device_type = DeviceInfo::kVideo;
     } else if ([device hasMediaType:AVFoundationGlue::AVMediaTypeMuxed()]) {
       device_type = suspended ? DeviceInfo::kAudio : DeviceInfo::kMuxed;
     } else if ([device hasMediaType:AVFoundationGlue::AVMediaTypeAudio()]) {
       device_type = DeviceInfo::kAudio;
     }
     snapshot_devices.push_back(DeviceInfo([[device uniqueID] UTF8String],
                                           device_type));
   }
   // Post the consolidation of enumerated devices to be done on UI thread.
   content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
       base::Bind(&SuspendObserverDelegate::OnDeviceChangedOnUIThread,
           this, snapshot_devices));
 }
 
 void SuspendObserverDelegate::StartObserver() {
   DCHECK(device_thread_checker_.CalledOnValidThread());
-  suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc]
-                               initWithChangeReceiver:this]);
-  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices])
-    [suspend_observer_ startObserving:device];
+
+  base::Closure on_device_changed_callback = media::BindToCurrentLoop(
+      base::Bind(&SuspendObserverDelegate::OnDeviceChanged, this));
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::BindBlock(^{ suspend_observer_.reset(

[Robert Sesek, 2014/07/07 22:47:50]

nit: keep ^{ on this line but start the content of the block on the next

[mcasas, 2014/07/08 15:11:45]

Done.

+          [[CrAVFoundationDeviceObserver alloc]
+              initWithOnChangedCallback:on_device_changed_callback]);
+      }));
+  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices]) {
+    scoped_refptr<CrAVCaptureDeviceRefCounted> device_refptr =
+        new CrAVCaptureDeviceRefCounted(device);
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::BindBlock(^{ [suspend_observer_ startObserving:device]; }));
+  }
 }
 
 void SuspendObserverDelegate::ResetDeviceMonitorOnUIThread() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   avfoundation_monitor_impl_ = NULL;
 }
 
 void SuspendObserverDelegate::OnDeviceChangedOnUIThread(
     const std::vector<DeviceInfo>& snapshot_devices) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ skipping 70 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   device_task_runner_->PostTask(FROM_HERE,
       base::Bind(&SuspendObserverDelegate::OnDeviceChanged,
                  suspend_observer_delegate_));
 }
 
 }  // namespace
 
 @implementation CrAVFoundationDeviceObserver
 
-- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver {
+- (id)initWithOnChangedCallback:(const base::Closure&)callback {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if ((self = [super init])) {
-    DCHECK(receiver != NULL);
-    receiver_ = receiver;
+    DCHECK(!callback.is_null());
+    onDeviceChangedCallback_ = callback;
   }
   return self;
 }
 
 - (void)dealloc {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   std::set<CrAVCaptureDevice*>::iterator it = monitoredDevices_.begin();
-  while (it != monitoredDevices_.end())
-    [self stopObserving:*it++];
+  while (it != monitoredDevices_.end()) {
+    [self removeObservers:*it];
+    monitoredDevices_.erase(it++);
+  }
   [super dealloc];
 }
 
 - (void)startObserving:(CrAVCaptureDevice*)device {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(device != nil);
   // Skip this device if there are already observers connected to it.
   if (std::find(monitoredDevices_.begin(), monitoredDevices_.end(), device) !=
           monitoredDevices_.end()) {
     return;
   }
   [device addObserver:self
            forKeyPath:@"suspended"
               options:0
               context:device];
   [device addObserver:self
            forKeyPath:@"connected"
               options:0
               context:device];
   monitoredDevices_.insert(device);
 }
 
 - (void)stopObserving:(CrAVCaptureDevice*)device {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(device != nil);
   std::set<CrAVCaptureDevice*>::iterator found =
       std::find(monitoredDevices_.begin(), monitoredDevices_.end(), device);
   DCHECK(found != monitoredDevices_.end());
-  // Every so seldom, |device| might be gone when getting here, in that case
-  // removing the observer causes a crash. Try to avoid it by checking sanity of
-  // the |device| via its -observationInfo. http://crbug.com/371271.
+  [self removeObservers:*found];
+  monitoredDevices_.erase(found);
+}
+
+- (void)removeObservers:(CrAVCaptureDevice*)device {
+  // Check sanity of |device| via its -observationInfo. http://crbug.com/371271.
   if ([device observationInfo]) {
     [device removeObserver:self
                 forKeyPath:@"suspended"];
     [device removeObserver:self
                 forKeyPath:@"connected"];
   }
-  monitoredDevices_.erase(found);
 }
 
 - (void)observeValueForKeyPath:(NSString*)keyPath
                       ofObject:(id)object
                         change:(NSDictionary*)change
                        context:(void*)context {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if ([keyPath isEqual:@"suspended"])
-    receiver_->OnDeviceChanged();
+    onDeviceChangedCallback_.Run();

[Robert Sesek, 2014/07/07 22:47:50]

Won't this run the callback on the UI thread, rather than the device thread?

[mcasas, 2014/07/08 15:11:46]

l.314 media::BindToCurrentLoop does the magic of caching the 
original message loop proxy and trampolining the PostTask back
to it.

[Robert Sesek, 2014/07/08 17:05:39]

Looks like this got dropped in the latest patch set?

   if ([keyPath isEqual:@"connected"])
     [self stopObserving:static_cast<CrAVCaptureDevice*>(context)];
 }
 
 @end  // @implementation CrAVFoundationDeviceObserver
 
 namespace content {
 
 DeviceMonitorMac::DeviceMonitorMac() {
   // Both QTKit and AVFoundation do not need to be fired up until the user
@@ skipping 18 matching lines @@
 }
 
 void DeviceMonitorMac::NotifyDeviceChanged(
     base::SystemMonitor::DeviceType type) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // TODO(xians): Remove the global variable for SystemMonitor.
   base::SystemMonitor::Get()->ProcessDevicesChanged(type);
 }
 
 }  // namespace content