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

media/video/capture/mac/video_capture_device_qtkit_mac.mm

Index: media/video/capture/mac/video_capture_device_qtkit_mac.mm
diff --git a/media/video/capture/mac/video_capture_device_qtkit_mac.mm b/media/video/capture/mac/video_capture_device_qtkit_mac.mm
index 86d4e82d71ac2933e44a0954db702558e663120f..26e942d19fd6d136156f5fe69bae430056e5e60e 100644
--- a/media/video/capture/mac/video_capture_device_qtkit_mac.mm
+++ b/media/video/capture/mac/video_capture_device_qtkit_mac.mm
@@ -146,28 +146,8 @@
           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];
@@ -242,14 +222,32 @@
 }
 
 - (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];
 }
 
+- (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