Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
464363003:
Mac QTKit Video Capture: Run QTCaptureSession teardown in UI thread, clean up -stopCapture use. (Closed)
|
Created:
6 years, 4 months ago by mcasas Modified:
6 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMac QTKit Video Capture: Run QTCaptureSession teardown in UI thread, clean up -stopCapture use.
It can be read in the code:
> 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
In the same spirit, seems that -removeInput: and -stopRunning are
being called from a background thread causing deadlocks every now
and then. This CL temptatively sends _both_ to be run on UI thread,
hence avoiding the potential deadlock.
-stopCapture is called from VideoCaptureDeviceMac::StopAndDeAllocate()
and then maybe in -setCaptureDevice. Remove the first call
and make sure it's always called inside -setCaptureDevice.
BUG=399792, 285053
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291018
Patch Set 1 #
Total comments: 6
Patch Set 2 : shess@ comments #
Total comments: 4
Patch Set 3 : shess@ comments: Moved input/output removal to one place, cleaned up input/output count #
Total comments: 9
Patch Set 4 : shess@ comments, rebased. #
Total comments: 2
Patch Set 5 : shess@ comments on -stopCaptureOnUIThread: #
Messages
Total messages: 23 (0 generated)
vrk@, rsesek@, PTAL. I would like to get this tested and would not land it before the 38 cut.
lgtm https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_qtkit_mac.mm:247: [self performSelectorOnMainThread:@selector(stopCaptureOnUIThread:) Hmm, when I read the stack trace from crbug.com/399792, it looks removeInput already automatically posts to the main thread: 2292 -[QTCaptureSession removeInput:] 2292 -[QTCaptureSession _beginSessionUpdates] 2292 -[NSObject(NSThreadPerformAdditions) performSelectorOnMainThread:withObject:waitUntilDone:modes:] But I don't know how it really works. I have no problems with doing this explicitly.
shess@ you addressed similar issues before, could you PTAL? https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_qtkit_mac.mm:247: [self performSelectorOnMainThread:@selector(stopCaptureOnUIThread:) On 2014/08/14 02:48:30, Victoria Kirst wrote: > Hmm, when I read the stack trace from crbug.com/399792, it looks removeInput > already automatically posts to the main thread: > > 2292 -[QTCaptureSession removeInput:] > 2292 -[QTCaptureSession _beginSessionUpdates] > 2292 -[NSObject(NSThreadPerformAdditions) > performSelectorOnMainThread:withObject:waitUntilDone:modes:] > > But I don't know how it really works. I have no problems with doing this > explicitly. That's true, though I suspect you'd see a similar stack from -removeOutput:, in other words the lock would be acquired before _beginSessionUpdates:. This is all very speculative :) but it fits in a line of reasoning of QTKit and/or QuickTime never designed to be thread-safe.
I'll defer to shess@ on this one, since he fought this nasty/crazy stuff before. https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_qtkit_mac.mm:255: -(void)stopCaptureOnUIThread:(QTCaptureSession*)captureSession { nit: space before (
https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (left): https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_qtkit_mac.mm:244: [captureSession_ removeInput:captureDeviceInput_]; As phrased, it's saying "If the number of inputs is exactly one, remove the last one", so the tested state is immediately changed. By posting to the main thread, there will now be a window where the tested state remains true, but where it's going to be invalidated. I don't know what this means, but since the code appears to use [[cS_ inputs] count] to indicate state, I'm nervous about it. In the case of -removeOutput:, the captureSession_ itself is invalidated immediately. For input, it looks to me like StopAndDeAllocate() is going to call -stopCapture, then -setCaptureDevice:nil, which is going to call -stopCapture again. I wonder if the explicit -stopCapture can be removed? Insert "I don't know what I'm doing" dog here.
shess@, tommi@ PTAL. https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (left): https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_qtkit_mac.mm:244: [captureSession_ removeInput:captureDeviceInput_]; On 2014/08/14 17:35:55, Scott Hess wrote: > As phrased, it's saying "If the number of inputs is exactly one, remove the last > one", so the tested state is immediately changed. By posting to the main > thread, there will now be a window where the tested state remains true, but > where it's going to be invalidated. I don't know what this means, but since the > code appears to use [[cS_ inputs] count] to indicate state, I'm nervous about > it. > Perfectly reasonable; I moved the |count| check to -stopCaptureOnUIThread. > In the case of -removeOutput:, the captureSession_ itself is invalidated > immediately. For input, it looks to me like StopAndDeAllocate() is going to > call -stopCapture, then -setCaptureDevice:nil, which is going to call > -stopCapture again. I wonder if the explicit -stopCapture can be removed? > You're right, I'll adapt the docos and remove the -stopCapture call at device_monitor_mac.mm. > Insert "I don't know what I'm doing" dog here. https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/1/media/video/capture/mac/vide... media/video/capture/mac/video_capture_device_qtkit_mac.mm:255: -(void)stopCaptureOnUIThread:(QTCaptureSession*)captureSession { On 2014/08/14 16:15:42, rsesek wrote: > nit: space before ( Done.
lgtm fwiw - I'm having a hard time wrapping my head around how this works :-/ (weak objc)
Apologies for lateness, I was trying to accomplish a thing today so was ignoring emails. Also, apologies that someone is having to deal with this. I can see how the idea of running stuff on a crazy background thread made sense on some other platform, but all it really does on OSX is add another layer of threading on top of whatever threading Apple is already doing (Apple is pretty thread-happy), which mostly just makes things worse. https://codereview.chromium.org/464363003/diff/80001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_qtkit_mac.mm:239: - (void)stopCapture { 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. https://codereview.chromium.org/464363003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_qtkit_mac.mm:250: if ([[captureSession_ inputs] count] == 1) { 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.
shess@ PTAL. https://codereview.chromium.org/464363003/diff/80001/media/video/capture/mac/... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_qtkit_mac.mm:239: - (void)stopCapture { On 2014/08/15 23:38:15, Scott Hess wrote: > 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. -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... https://codereview.chromium.org/464363003/diff/80001/media/video/capture/mac/... media/video/capture/mac/video_capture_device_qtkit_mac.mm:250: if ([[captureSession_ inputs] count] == 1) { On 2014/08/15 23:38:15, Scott Hess wrote: > This test should definitely not be here. If the code gets to this point and > this is != 1, then something bad has probably happened. 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... > > 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]; > 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. > 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. 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.
LGTM, I'm down to quibbles. The only one I _really_ care about is -stopCaptureOnUIThread:, so if you prefer dealing with that a different way, loop me back in, please. You can take or leave my comments on the other cases as you prefer. https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... File media/video/capture/mac/platform_video_capturing_mac.h (right): https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/platform_video_capturing_mac.h:44: // Starts video capturing, register observers. Returns YES on sucess, NO If you're going there, you need "registers", too. "Starts", "registers", "Returns". https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:226: // handler might need. To avoid q deadlock, we perform those tasks in the UI 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. https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:234: - (void)stopCaptureOnUIThread:(QTCaptureSession*)captureSession { 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. https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:242: [captureSession_ removeOutput:output]; 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.
shess@ PTAL https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... File media/video/capture/mac/platform_video_capturing_mac.h (right): https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/platform_video_capturing_mac.h:44: // Starts video capturing, register observers. Returns YES on sucess, NO On 2014/08/18 18:23:40, Scott Hess wrote: > If you're going there, you need "registers", too. "Starts", "registers", > "Returns". Done. https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:226: // handler might need. To avoid q deadlock, we perform those tasks in the UI On 2014/08/18 18:23:40, Scott Hess wrote: > 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. 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". https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:234: - (void)stopCaptureOnUIThread:(QTCaptureSession*)captureSession { On 2014/08/18 18:23:40, Scott Hess wrote: > 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. Done. https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:242: [captureSession_ removeOutput:output]; On 2014/08/18 18:23:40, Scott Hess wrote: > 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. 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.
https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/120001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:226: // handler might need. To avoid q deadlock, we perform those tasks in the UI On 2014/08/19 15:24:59, mcasas wrote: > On 2014/08/18 18:23:40, Scott Hess wrote: > > 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. > > 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". Mostly I'm aiming for consistent use w/in a single comment, so that it's clear they're not referring to different things. https://codereview.chromium.org/464363003/diff/140001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/140001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:240: return; 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 :-).]]
shess@ PTAL https://codereview.chromium.org/464363003/diff/140001/media/video/capture/mac... File media/video/capture/mac/video_capture_device_qtkit_mac.mm (right): https://codereview.chromium.org/464363003/diff/140001/media/video/capture/mac... media/video/capture/mac/video_capture_device_qtkit_mac.mm:240: return; On 2014/08/19 18:27:50, Scott Hess wrote: > 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.] > 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... > [[Yes, I'm pretty conservative, especially in code known to generate deadlocks > :-).]] Never too careful in Cr-land :)
LGTM. Commit before I comment again!
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/464363003/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/464363003/160001
Message was sent while issue was closed.
Committed patchset #5 (160001) as 291018 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
