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 126 matching lines @@
     [captureDecompressedOutput setPixelBufferAttributes:captureDictionary];
 
     return YES;
   } else {
     // Remove the previously set capture device.
     if (!captureDeviceInput_) {
       [self sendErrorString:[NSString
           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);
       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
@@ skipping 71 matching lines @@
         [NSNotificationCenter defaultCenter];
     [notificationCenter addObserver:self
                            selector:@selector(handleNotification:)
                                name:QTCaptureSessionRuntimeErrorNotification
                              object:captureSession_];
     [captureSession_ startRunning];
   }
   return YES;
 }
 
 - (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.
 - (void)captureOutput:(QTCaptureOutput*)captureOutput
   didOutputVideoFrame:(CVImageBufferRef)videoFrame
      withSampleBuffer:(QTSampleBuffer*)sampleBuffer
        fromConnection:(QTCaptureConnection*)connection {
   [lock_ lock];
   if(!frameReceiver_) {
     [lock_ unlock];
@@ skipping 80 matching lines @@
 
 - (void)sendErrorString:(NSString*)error {
   DLOG(ERROR) << [error UTF8String];
   [lock_ lock];
   if (frameReceiver_)
     frameReceiver_->ReceiveError([error UTF8String]);
   [lock_ unlock];
 }
 
 @end