Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2003973003:
Reorder actions on stopCapturer, to avoid crashing on camera timeout. (Closed)
|
Created:
4 years, 7 months ago by nisse-webrtc Modified:
4 years, 6 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReorder actions on stopCapturer, to avoid crashing on camera timeout.
BUG=
Committed: https://crrev.com/92379de5c66f27a9a643c0c7fbe785d0dde35542
Cr-Commit-Position: refs/heads/master@{#12963}
Patch Set 1 #Patch Set 2 : Call stopListening earlier. #
Total comments: 8
Patch Set 3 : Move cleanup into stopCaptureOnCameraThread. #
Total comments: 5
Patch Set 4 : Add comment on boolean literals. #Messages
Total messages: 19 (5 generated)
nisse@webrtc.org changed reviewers: + magjed@webrtc.org
First attempt, not yet tested. Will be easier if https://codereview.webrtc.org/1988043002/ is landed first, so we can call surfaceHelper.stopListening without a thread jump.
Rebased on top of the cl making stopListening threadsafe. Now attempts to clean up things in the order stopListening cameraThreadHandler = null; stopCaptureOnCameraThread in both stopCapture and in the startCapture failure case. Difficult to test, though, I'm not sure I'm getting it right.
https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (left): https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:637: throw new RuntimeException("onTextureFrameAvailable() called after stopCapture()."); Is this a possibility after your change? Then you need to handle it, i.e. return the texture. https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:372: surfaceHelper.stopListening(); Why have you added this stuff? https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:492: surfaceHelper.stopListening(); ditto: Why have you added this stuff?
https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (left): https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:637: throw new RuntimeException("onTextureFrameAvailable() called after stopCapture()."); On 2016/05/27 13:13:02, magjed_webrtc wrote: > Is this a possibility after your change? Then you need to handle it, i.e. return > the texture. The idea is to always call stopListening before setting cameraThreadHandler to null. So then this shouldn't happen. And if it does anyway, we'll get a null pointer exception just below, in checkIsOnCameraThread. https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:372: surfaceHelper.stopListening(); On 2016/05/27 13:13:02, magjed_webrtc wrote: > Why have you added this stuff? I moved it out of stopCaptureOnCameraThread, to get the call earlier. I think it makes sense to do stopListening (and excluding new calls to onTextureFrameAvailable) before tearing down the rest of the state. The point is that we'll do the cleanup of surfaceHelper and cameraThreadHandler even in the case that stopCameraOnCameraThread deadlocks somewhere in the driver. And I think it makes the code a bit clearer to have surfaceHelper = null close to stopListening. But maybe it can be arranged differently, I don't quite like the duplication between this error handling and the normal stopCapture method.
https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:372: surfaceHelper.stopListening(); On 2016/05/27 13:28:08, nisse-webrtc wrote: > On 2016/05/27 13:13:02, magjed_webrtc wrote: > > Why have you added this stuff? > > I moved it out of stopCaptureOnCameraThread, to get the call earlier. > I think it makes sense to do stopListening (and excluding new calls to > onTextureFrameAvailable) before tearing down the rest of the state. > > The point is that we'll do the cleanup of surfaceHelper and cameraThreadHandler > even in the case that stopCameraOnCameraThread deadlocks somewhere in the > driver. > > And I think it makes the code a bit clearer to have surfaceHelper = null close > to stopListening. But maybe it can be arranged differently, I don't quite like > the duplication between this error handling and the normal stopCapture method. I don't like the duplication either, this class is getting messier and messier. How about adding a bool argument |stopHandler| to stopCaptureOnCameraThread() instead? Then stopCaptureOnCameraThread() can close everything down in the correct order. https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:560: stopCaptureOnCameraThread(); There is a bug in your current patch that you don't call surfaceHelper.stopListening() here.
Reorganized cleanup as you suggested. https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2003973003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:372: surfaceHelper.stopListening(); On 2016/05/27 13:56:11, magjed_webrtc wrote: > On 2016/05/27 13:28:08, nisse-webrtc wrote: > > On 2016/05/27 13:13:02, magjed_webrtc wrote: > > > Why have you added this stuff? > > > > I moved it out of stopCaptureOnCameraThread, to get the call earlier. > > I think it makes sense to do stopListening (and excluding new calls to > > onTextureFrameAvailable) before tearing down the rest of the state. > > > > The point is that we'll do the cleanup of surfaceHelper and > cameraThreadHandler > > even in the case that stopCameraOnCameraThread deadlocks somewhere in the > > driver. > > > > And I think it makes the code a bit clearer to have surfaceHelper = null close > > to stopListening. But maybe it can be arranged differently, I don't quite like > > the duplication between this error handling and the normal stopCapture method. > > I don't like the duplication either, this class is getting messier and messier. > How about adding a bool argument |stopHandler| to stopCaptureOnCameraThread() > instead? Then stopCaptureOnCameraThread() can close everything down in the > correct order. Sounds reasonable, I've given it a try.
https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:375: stopCaptureOnCameraThread(true); nit: Comment literal true like this: 'true /* stopHandler */', in this and the other call sites. https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:521: surfaceHelper = null; Setting |surfaceHelper| to null will not work in switchCameraOnCameraThread(). You could store |surfaceHelper| in a temporary local variable in switchCameraOnCameraThread() before calling stopCaptureOnCameraThread() and then reset it afterwards, but I would recommend simply removing 'surfaceHelper = null'. It's ok if we keep a reference to |surfaceHelper| after stopCapture is called.
https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:375: stopCaptureOnCameraThread(true); On 2016/05/30 11:10:18, magjed_webrtc wrote: > nit: Comment literal true like this: 'true /* stopHandler */', in this and the > other call sites. Done. https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:521: surfaceHelper = null; On 2016/05/30 11:10:18, magjed_webrtc wrote: > Setting |surfaceHelper| to null will not work in switchCameraOnCameraThread(). > You could store |surfaceHelper| in a temporary local variable in > switchCameraOnCameraThread() before calling stopCaptureOnCameraThread() and then > reset it afterwards, but I would recommend simply removing 'surfaceHelper = > null'. It's ok if we keep a reference to |surfaceHelper| after stopCapture is > called. switchCameraOnCameraThread passes stopHandler = false, so this block shouldn't be executed in that case.
lgtm https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2003973003/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:521: surfaceHelper = null; On 2016/05/30 12:01:55, nisse-webrtc wrote: > On 2016/05/30 11:10:18, magjed_webrtc wrote: > > Setting |surfaceHelper| to null will not work in switchCameraOnCameraThread(). > > You could store |surfaceHelper| in a temporary local variable in > > switchCameraOnCameraThread() before calling stopCaptureOnCameraThread() and > then > > reset it afterwards, but I would recommend simply removing 'surfaceHelper = > > null'. It's ok if we keep a reference to |surfaceHelper| after stopCapture is > > called. > > switchCameraOnCameraThread passes stopHandler = false, so this block shouldn't > be executed in that case. Oh, I see.
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003973003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003973003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003973003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003973003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Reorder actions on stopCapturer, to avoid crashing on camera timeout. BUG= ========== to ========== Reorder actions on stopCapturer, to avoid crashing on camera timeout. BUG= Committed: https://crrev.com/92379de5c66f27a9a643c0c7fbe785d0dde35542 Cr-Commit-Position: refs/heads/master@{#12963} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/92379de5c66f27a9a643c0c7fbe785d0dde35542 Cr-Commit-Position: refs/heads/master@{#12963} |
