Mac QTKit Video Capture: Run QTCaptureSession teardown in UI thread, clean up -stopCapture use.

media/video/capture/mac/video_capture_device_qtkit_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.
 
 #import "media/video/capture/mac/video_capture_device_qtkit_mac.h"
 
 #import <QTKit/QTKit.h>
 
 #include "base/debug/crash_logging.h"
 #include "base/logging.h"
@@ skipping 128 matching lines @@
     return YES;
   } else {
     // Remove the previously set capture device.
     if (!captureDeviceInput_) {
       // Being here means stopping a device that never started OK in the first
       // place, log it.
       [self sendLogString:[NSString
           stringWithUTF8String:"No video capture device set, on removal."]];
       return YES;
     }
-    if ([[captureSession_ inputs] count] > 0) {
-      // The device is still running.
-      [self stopCapture];
-    }
-    if ([[captureSession_ outputs] count] > 0) {
-      // Only one output is set for |captureSession_|.
-      DCHECK_EQ([[captureSession_ outputs] count], 1u);
-      id output = [[captureSession_ outputs] objectAtIndex:0];
-      [output setDelegate:nil];
-
-      // TODO(shess): QTKit achieves thread safety by posting messages to the
-      // main thread.  As part of -addOutput:, it posts a message to the main
-      // thread which in turn posts a notification which will run in a future
-      // spin after the original method returns.  -removeOutput: can post a
-      // main-thread message in between while holding a lock which the
-      // notification handler  will need.  Posting either -addOutput: or
-      // -removeOutput: to the main thread should fix it, remove is likely
-      // safer. http://crbug.com/152757
-      [captureSession_ performSelectorOnMainThread:@selector(removeOutput:)
-                                        withObject:output
-                                     waitUntilDone:YES];
-    }
+    // Tear down input and output, stop the capture and deregister observers.
+    [self stopCapture];
     [captureSession_ release];
     captureSession_ = nil;
     [captureDeviceInput_ release];
     captureDeviceInput_ = nil;
     return YES;
   }
 }
 
 - (BOOL)setCaptureHeight:(int)height
                    width:(int)width
@@ skipping 54 matching lines @@
     [notificationCenter addObserver:self
                            selector:@selector(handleNotification:)
                                name:QTCaptureSessionRuntimeErrorNotification
                              object:captureSession_];
     [captureSession_ startRunning];
   }
   return YES;
 }
 
 - (void)stopCapture {
-  if ([[captureSession_ inputs] count] == 1) {
-    [captureSession_ removeInput:captureDeviceInput_];
-    [captureSession_ stopRunning];
-  }
+  // QTKit achieves thread safety and asynchronous execution by posting messages
+  // to the main thread, e.g. -addOutput:. Both -removeOutput: and -removeInput:
+  // post a message to the main thread while holding a lock that the
+  // notification handler might need. To avoid a deadlock, we perform those
+  // tasks in the main thread. See bugs http://crbug.com/152757 and
+  // http://crbug.com/399792.
+  [self performSelectorOnMainThread:@selector(stopCaptureOnUIThread:)
+                         withObject:nil
+                      waitUntilDone:YES];
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
 
-  [[NSNotificationCenter defaultCenter] removeObserver:self];
+- (void)stopCaptureOnUIThread:(id)dummy {
+  if ([[captureSession_ outputs] count] == 0u ||
+      [[captureSession_ inputs] count] == 0u)
+    return;

[Scott Hess - ex-Googler, 2014/08/19 18:27:50]

If this is correct, it should be in -stopCapture itself, guarding posting the
-perform in the first place.

This still doesn't mirror the previous code, though, notably in the case I
mentioned where you could plausibly have an output setup but no input.  Was the
previous code making incorrect assumptions about these counts?  AFAICT, the test
for no captureDeviceInput_ means that the immediate next test testing inputs
count should never have failed.

Since the previous code was structured as "if there's an input, remove it; if
there's an output, remove it", and that pattern is not at question here, I think
it probably makes most sense just the replicate that unless you have grounds to
believe that it is hurting something.  Especially if this code using QTKit is
going away, which I think was implied on the bug.  [WRT guarding the -perform*,
that would imply posting it if there was either an input or an output.]

[[Yes, I'm pretty conservative, especially in code known to generate deadlocks
:-).]]

[mcasas, 2014/08/20 09:08:25]

I think that's reasonable. So I made it look more like before. In particular 
note the (#inputs > 0), this is a combination of the previous [1] and [2].
Since the #inputs should never be larger than 1 (protected by [3]), and
I think this should be notified at least in debug builds, I added a DCHECK.

However since we are in the final stages, if you want a more verbatim:

if ([[captureSession_ inputs count] == 1u)
  [captureSession_ removeInput:captureDeviceInput_];
if ([[captureSession_ inputs count] > 0)
  [captureSession_ stopRunning];

just say so and I'll swap them.


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
Never too careful in Cr-land :)

+  DCHECK_EQ([[captureSession_ inputs] count], 1u);
+  [captureSession_ removeInput:captureDeviceInput_];
+  [captureSession_ stopRunning];
+
+  DCHECK_EQ([[captureSession_ outputs] count], 1u);
+  id output = [[captureSession_ outputs] objectAtIndex:0];
+  [output setDelegate:nil];
+  [captureSession_ removeOutput:output];
 }
 
 // |captureOutput| is called by the capture device to deliver a new frame.
 - (void)captureOutput:(QTCaptureOutput*)captureOutput
   didOutputVideoFrame:(CVImageBufferRef)videoFrame
      withSampleBuffer:(QTSampleBuffer*)sampleBuffer
        fromConnection:(QTCaptureConnection*)connection {
   [lock_ lock];
   if(!frameReceiver_) {
     [lock_ unlock];
@@ skipping 88 matching lines @@
 
 - (void)sendLogString:(NSString*)message {
   DVLOG(1) << [message UTF8String];
   [lock_ lock];
   if (frameReceiver_)
     frameReceiver_->LogMessage([message UTF8String]);
   [lock_ unlock];
 }
 
 @end