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 7f4a853c63a9bde6d9b260a720a947d143f68cbc..e2260d0698fa9e1686c719ffcb5847fd7441c300 100644
--- a/media/video/capture/mac/video_capture_device_qtkit_mac.mm
+++ b/media/video/capture/mac/video_capture_device_qtkit_mac.mm
@@ -144,10 +144,7 @@
           stringWithUTF8String:"No video capture device set, on removal."]];
       return YES;
     }
-    if ([[captureSession_ inputs] count] > 0) {
-      // The device is still running.
-      [self stopCapture];
-    }
+    [self stopCapture];
     if ([[captureSession_ outputs] count] > 0) {
       // Only one output is set for |captureSession_|.
       DCHECK_EQ([[captureSession_ outputs] count], 1u);
@@ -240,12 +237,20 @@
 }
 
 - (void)stopCapture {

[Scott Hess - ex-Googler, 2014/08/15 23:38:15]

Since you've removed the other user of -stopCapture, I think that bouncing
through this method obscures what's happening.  You can't really just call it
and assume it does the right thing, you have to know what it's doing.  So might
as well just inline it.

[mcasas, 2014/08/18 11:29:43]

 -stopCapture is part of the device interface [1] that also removes/deregisters
observers in Device Thread. Also, the homonymous in AVFoundation
does not need to jump back to UI.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...

+  // Similar to the comments about -removeOutput: in -setCaptureDevice:,
+  // -removeInput: can post a call to main thread while holding a lock,
+  // causing a deadlock, see http://crbug.com/399792.
+  [self performSelectorOnMainThread:@selector(stopCaptureOnUIThread:)
+                         withObject:captureSession_
+                      waitUntilDone:YES];
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
+- (void)stopCaptureOnUIThread:(QTCaptureSession*)captureSession {
   if ([[captureSession_ inputs] count] == 1) {

[Scott Hess - ex-Googler, 2014/08/15 23:38:15]

This test should definitely not be here.  If the code gets to this point and
this is != 1, then something bad has probably happened.

waitUntilDone:YES means that this is going to block waiting for things to run
over on the main thread.  So IMHO you can just do it directly against the
session object just like the output side:

[captureSession_ performSelectorOnMainThread:@selector(removeInput:)
withObject:captureDeviceInput_ waitUntilDone:YES];

That said ... do we have any overall love for this?  Having two wait-until-done
posts from a background thread makes me somewhat sad, if it makes you sad also,
then maybe there could be a method like -[VideoCaptureDeviceQTKit
shutdownAllTheThings] which you post if there are either inputs or outputs, and
which does the -removeInput:, -stopRunning, and -removeOutput: on the UI thread.

[mcasas, 2014/08/18 11:29:43]

This test is actually a bug, since there is just one call to 
-addInput [1] and is protected by 
if ([[captureSession_ inputs] count] == 0) , so there is no way
a session can have more than one input -and it wouldn't make
sense either-.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
There's still the -stopRunning method call that -and this is
guesswork- would still need to be called. I'll drop this suggestion
in favour of your last and following proposal.
This sounds good, I stuffed all the tear down parts: -removeInput, 
-stopRunning and -removeOutput in -stopCaptureOnUIThread: , and
added DCHECKs for input and output count to be exactly one.

     [captureSession_ removeInput:captureDeviceInput_];
     [captureSession_ stopRunning];
   }
-
-  [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
 // |captureOutput| is called by the capture device to deliver a new frame.