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..ad22e3511cd1481a9a80ffce8a58cb3bbca22797 100644
--- a/media/video/capture/mac/video_capture_device_qtkit_mac.mm
+++ b/media/video/capture/mac/video_capture_device_qtkit_mac.mm
@@ -144,28 +144,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];
@@ -240,14 +220,28 @@
 }
 
 - (void)stopCapture {
-  if ([[captureSession_ inputs] count] == 1) {
-    [captureSession_ removeInput:captureDeviceInput_];
-    [captureSession_ stopRunning];
-  }
-
+  // QTKit achieves thread safety and asynchronous execution by posting messages
+  // to main thread, e.g. -addOutput:. Both -removeOutput: and -removeInput:
+  // post a message to main thread while holding a lock that the notification
+  // handler might need. To avoid q deadlock, we perform those tasks in the UI

[Scott Hess - ex-Googler, 2014/08/18 18:23:40]

is the 'q' meant to be an 'a'?

Also straighten up use of articles.  I think most of this is written such that
'the main thread' would be better, to match 'the UI thread' and 'a message' and
'the notification'.  Well, and using both 'main' and 'UI' as thread labels is
confusing.

[mcasas, 2014/08/19 15:24:59]

Done. I use them interchangeably, but is true that in Obj-C++ code it 
should be "the main thread" while in the rest of Cr is "the UI thread".

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

Mostly I'm aiming for consistent use w/in a single comment, so that it's clear
they're not referring to different things.

+  // thread. See bug http://crbug.com/152757 and http://crbug.com/399792.
+  [self performSelectorOnMainThread:@selector(stopCaptureOnUIThread:)
+                         withObject:captureSession_
+                      waitUntilDone:YES];
   [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
+- (void)stopCaptureOnUIThread:(QTCaptureSession*)captureSession {

[Scott Hess - ex-Googler, 2014/08/18 18:23:40]

You never use |captureSession|, instead preferring |captureSession_|. 
waitUntilDone:YES means that this works fine, and using captureDeviceInput_ may
mean that you can't just expand it to be hermetic.  There is no no-argument
helper, so I'd recommend having it be something like (id)dummy and passing
|nil|, rather than having ambiguity as to which value is relevant, here.

[mcasas, 2014/08/19 15:24:59]

Done.

+  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];

[Scott Hess - ex-Googler, 2014/08/18 18:23:40]

Previously, the code handled 0 values for either count.  Is it 100% certain that
this is always going to see exactly one input and one output?

I have literally no idea what any of this code is actually doing, but
code-reading, it looks like -setCaptureDevice: adds an output, but doesn't
actually add the input until -startCapture. 
VideoCaptureDeviceMac::AllocateAndStart() calls those two, but there is a return
case in between, so I think it is possible to break this assertion.

My main concern is whether this will case DCHECK firing for unrelated developers
- and my guess is _probably_ not.  So mostly I guess  just a value judgement as
to whether you think the assertions add important notifications for people doing
media development or not.

[mcasas, 2014/08/19 15:24:59]

I changed a bit the code to reflect your sharp observation, and now it should 
read: if there's no output OR there is no input, then return, but otherwise
there must be exactly one input and one output, and stop them in the adequate
order.

+}
+
 // |captureOutput| is called by the capture device to deliver a new frame.
 - (void)captureOutput:(QTCaptureOutput*)captureOutput
   didOutputVideoFrame:(CVImageBufferRef)videoFrame