Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 3 months ago by magjed_webrtc Modified:
5 years, 3 months ago Reviewers:
hbos, perkj_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionVideoCapturerAndroid: Fix threading issues
This CL makes the following changes:
* Instead of creating a new thread per startCapture()/stopCapture() cycle, VideoCapturerAndroid has a single thread that is initialized in the constructor and kept during the lifetime of the instance. This is more convenient because then it is always possible to post runnables without if-checks. This way, a lot of synchronize statements can be removed. Also, when the camera thread is preserved after stopCapture() it is possible to post late returnBuffer() calls to the correct thread.
* FramePool now enforces single thread use and returnBuffer() calls are posted to the camera thread. This is important because the camera should only be used from one thread, and we call camera.addCallbackBuffer() in returnBuffer().
* switchCamera() no longer returns false on failure, but instead signals the result via the callback.
* Update the test testCaptureAndAsyncRender() - it's not a valid use case to have outstanding frames when calling PeerConnectionFactory.dispose(). Instead, the renderer implementations should have release() functions that block until all frames are returned. The release() functions need to be called in the correct order with PeerConnectionFactory.dispose() last.
BUG=webrtc:4909
R=hbos@webrtc.org, perkj@webrtc.org
Committed: https://crrev.com/f706c8ae914da976f16205ff13d15b1a28ead8fd
Cr-Commit-Position: refs/heads/master@{#10025}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Readded test #
Total comments: 4
Patch Set 3 : Updated comments and renamed FakeAsyncRenderer #
Total comments: 10
Patch Set 4 : Addressing hbos@ comments #
Total comments: 9
Patch Set 5 : Addressing hbos@s comments #
Created: 5 years, 3 months ago
Messages
Total messages: 24 (8 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
magjed@webrtc.org changed reviewers: + hbos@webrtc.org, perkj@webrtc.org
Please take a look. hbos@ is added as Java expert.
lgmt if the comments (only document changes) are addressed. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:148: // Camera switch handler - one of these functions are invoked with the result of switchCamera(). Document threading. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:169: // Returns true on success. False if the next camera does not support the Does not return anything now. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:211: public void onOutputFormatRequest(final int width, final int height, final int fps) { Todo: Document what this does. Change name? https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:277: private void release() { Document why this need to be called. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:456: // TODO(magjed): camera.release() should probably be in a finally clause. done in separte cl
On 2015/09/18 09:17:58, perkj1 wrote: > lgmt if the comments (only document changes) are addressed. > > https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... > File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): > > https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... > talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:148: // Camera > switch handler - one of these functions are invoked with the result of > switchCamera(). > Document threading. > > https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... > talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:169: // > Returns true on success. False if the next camera does not support the > Does not return anything now. > > https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... > talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:211: public > void onOutputFormatRequest(final int width, final int height, final int fps) { > Todo: Document what this does. Change name? > > https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... > talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:277: private > void release() { > Document why this need to be called. > > https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... > talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:456: // > TODO(magjed): camera.release() should probably be in a finally clause. > done in separte cl lgtm I mean.....
https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidte... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (left): https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:418: } I'm thinking it might be worth to tweak this test case so that dispose happens properly. Looks like it was the only test like this. What do you think?
Patchset #2 (id:80001) has been deleted
https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidte... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (left): https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidte... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:418: } On 2015/09/18 11:10:52, hbos wrote: > I'm thinking it might be worth to tweak this test case so that dispose happens > properly. Looks like it was the only test like this. What do you think? Probably true. We have a similar test in testReturnBufferLate(), but it does not test the android capture JNI layer, and that's where we had a crash recently. I haved updated the test now. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:148: // Camera switch handler - one of these functions are invoked with the result of switchCamera(). On 2015/09/18 09:17:57, perkj1 wrote: > Document threading. Done. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:169: // Returns true on success. False if the next camera does not support the On 2015/09/18 09:17:57, perkj1 wrote: > Does not return anything now. Done. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:211: public void onOutputFormatRequest(final int width, final int height, final int fps) { On 2015/09/18 09:17:57, perkj1 wrote: > Todo: Document what this does. Change name? Done. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:277: private void release() { On 2015/09/18 09:17:57, perkj1 wrote: > Document why this need to be called. Done. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/andr... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:456: // TODO(magjed): camera.release() should probably be in a finally clause. On 2015/09/18 09:17:57, perkj1 wrote: > done in separte cl Acknowledged. Rebased.
https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:67: static class AsyncRenderer implements VideoRenderer.Callbacks { Name testrenderer? https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:385: // This test that we can capture frames, keep the frames on the other end of peerconnection, stop This does not create a PC. It tests local rendering of a local video track.
hbos - Please take another look. https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:67: static class AsyncRenderer implements VideoRenderer.Callbacks { On 2015/09/21 08:29:21, perkj1 wrote: > Name testrenderer? Acknowledged. I renamed it to FakeAsyncRenderer. I want to keep async in the name to distinguish it from RendererCallbacks above that is also a test renderer. https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:385: // This test that we can capture frames, keep the frames on the other end of peerconnection, stop On 2015/09/21 08:29:21, perkj1 wrote: > This does not create a PC. It tests local rendering of a local video track. Ah, I see.
https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:79: public List<I420Frame> WaitForFrames() throws InterruptedException { nit: waitForPendingFrames() https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:400: capturer.stopCapture(); waitForFrames() should happen after stopCapture() or else there is a unlikely race in which case the fake renderer could receive more frames after pendingFrames that are never returned. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:415: assertEquals(capturer.pendingFramesTimeStamps(), "[]"); nit: Checking pending frames count == 0 would be nicer. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:420: factory.dispose(); It is not clear that the C++ resources should be freed after some of these have been disposed. And the unittest does not check that this happens. It is up to you if you want to address this or not. Shouldn't the capturer have a dispose()? That would be a different CL and discussion though.
hbos - PTAL. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:79: public List<I420Frame> WaitForFrames() throws InterruptedException { On 2015/09/21 12:59:06, hbos wrote: > nit: waitForPendingFrames() Done. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:400: capturer.stopCapture(); On 2015/09/21 12:59:06, hbos wrote: > waitForFrames() should happen after stopCapture() or else there is a unlikely > race in which case the fake renderer could receive more frames after > pendingFrames that are never returned. Thanks. Done. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:415: assertEquals(capturer.pendingFramesTimeStamps(), "[]"); On 2015/09/21 12:59:06, hbos wrote: > nit: Checking pending frames count == 0 would be nicer. Done. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:420: factory.dispose(); On 2015/09/21 12:59:06, hbos wrote: > It is not clear that the C++ resources should be freed after some of these have > been disposed. And the unittest does not check that this happens. It is up to > you if you want to address this or not. Shouldn't the capturer have a dispose()? > That would be a different CL and discussion though. The capturer is owned by |source| I think, so it does not have a separate dispose(). I will not add a test for VideoCapturerAndroid.release() in this CL.
https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:420: factory.dispose(); On 2015/09/21 15:27:08, magjed_webrtc wrote: > On 2015/09/21 12:59:06, hbos wrote: > > It is not clear that the C++ resources should be freed after some of these > have > > been disposed. And the unittest does not check that this happens. It is up to > > you if you want to address this or not. Shouldn't the capturer have a > dispose()? > > That would be a different CL and discussion though. > > The capturer is owned by |source| I think, so it does not have a separate > dispose(). I will not add a test for VideoCapturerAndroid.release() in this CL. How about a simple has been released flag as discussed offline? https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:238: } If |id| is volatile you would not have to lock here. When incrementing (read-and-write as a single op) you would still need a lock if set by multiple threads, but if it's only ever the camera thread that sets |id| then you could get rid of |cameraIdLock| altogether by using volatile. If we can't get rid of |cameraIdLock| then this comment is optional. https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:284: this.id = 0; If on camera thread, rename to initOnCameraThread and |id| can be set without lock if volatile. But this could be some other C++ thread? Use |cameraIdLock|? https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:477: cameraSurfaceTexture = null; Does cameraSurfaceTexture not have to be released as well or is this done implicitly by camera.release()? https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:589: private void checkIsOnValidThread() { Optional: Could we have a checkIsOnCameraThread like this for the *OnCameraThread methods?
hbos - Please take another look. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidt... talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:420: factory.dispose(); On 2015/09/22 09:12:46, hbos wrote: > On 2015/09/21 15:27:08, magjed_webrtc wrote: > > On 2015/09/21 12:59:06, hbos wrote: > > > It is not clear that the C++ resources should be freed after some of these > > have > > > been disposed. And the unittest does not check that this happens. It is up > to > > > you if you want to address this or not. Shouldn't the capturer have a > > dispose()? > > > That would be a different CL and discussion though. > > > > The capturer is owned by |source| I think, so it does not have a separate > > dispose(). I will not add a test for VideoCapturerAndroid.release() in this > CL. > > How about a simple has been released flag as discussed offline? Done. https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:238: } On 2015/09/22 09:12:46, hbos wrote: > If |id| is volatile you would not have to lock here. > When incrementing (read-and-write as a single op) you would still need a lock if > set by multiple threads, but if it's only ever the camera thread that sets |id| > then you could get rid of |cameraIdLock| altogether by using volatile. > > If we can't get rid of |cameraIdLock| then this comment is optional. Acknowledged. https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:284: this.id = 0; On 2015/09/22 09:12:46, hbos wrote: > If on camera thread, rename to initOnCameraThread and |id| can be set without > lock if volatile. But this could be some other C++ thread? > Use |cameraIdLock|? It is not on camera thread, but it will be called from JNI code in connection with a create() call. Synchronization is not strictly necessary, because create() will block until this is done. I have added synchronization anyway to be robust. https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:477: cameraSurfaceTexture = null; On 2015/09/22 09:12:46, hbos wrote: > Does cameraSurfaceTexture not have to be released as well or is this done > implicitly by camera.release()? Let's try. What can possibly go wrong? https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:589: private void checkIsOnValidThread() { On 2015/09/22 09:12:46, hbos wrote: > Optional: Could we have a checkIsOnCameraThread like this for the > *OnCameraThread methods? Done.
Very nice! lgtm https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/and... talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:477: cameraSurfaceTexture = null; On 2015/09/22 15:04:21, magjed_webrtc wrote: > On 2015/09/22 09:12:46, hbos wrote: > > Does cameraSurfaceTexture not have to be released as well or is this done > > implicitly by camera.release()? > > Let's try. What can possibly go wrong? YOLO. Random opengl thread crashes incoming. Probably a good idea: "Always call this method when you are done with SurfaceTexture. Failing to do so may delay resource deallocation for a significant amount of time."
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1350863002/#ps160001 (title: "Addressing hbos@s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1350863002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1350863002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/4128)
Committed patchset #5 (id:160001) manually as f706c8ae914da976f16205ff13d15b1a28ead8fd (presubmit successful).
Patchset 5 (id:??) landed as https://crrev.com/f706c8ae914da976f16205ff13d15b1a28ead8fd Cr-Commit-Position: refs/heads/master@{#10025} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
