Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(80)

Issue 1350863002: VideoCapturerAndroid: Fix threading issues (Closed)

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.

Description

VideoCapturerAndroid: 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+256 lines, -211 lines) Patch
M talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java View 1 2 3 4 12 chunks +73 lines, -60 lines 0 comments Download
M talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java View 1 2 3 4 23 chunks +179 lines, -151 lines 0 comments Download
M talk/app/webrtc/java/jni/androidvideocapturer_jni.cc View 1 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (8 generated)
magjed_webrtc
Please take a look. hbos@ is added as Java expert.
5 years, 3 months ago (2015-09-17 09:33:33 UTC) #5
perkj_webrtc
lgmt if the comments (only document changes) are addressed. https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java#newcode148 talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:148: ...
5 years, 3 months ago (2015-09-18 09:17:58 UTC) #6
perkj_webrtc
On 2015/09/18 09:17:58, perkj1 wrote: > lgmt if the comments (only document changes) are addressed. ...
5 years, 3 months ago (2015-09-18 09:18:20 UTC) #7
hbos
https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (left): https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#oldcode418 talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:418: } I'm thinking it might be worth to tweak ...
5 years, 3 months ago (2015-09-18 11:10:53 UTC) #8
magjed_webrtc
https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (left): https://codereview.webrtc.org/1350863002/diff/60001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#oldcode418 talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:418: } On 2015/09/18 11:10:52, hbos wrote: > I'm thinking ...
5 years, 3 months ago (2015-09-21 08:19:57 UTC) #10
perkj_webrtc
https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode67 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/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode385 ...
5 years, 3 months ago (2015-09-21 08:29:21 UTC) #11
magjed_webrtc
hbos - Please take another look. https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/100001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode67 talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:67: static class AsyncRenderer ...
5 years, 3 months ago (2015-09-21 09:19:47 UTC) #12
hbos
https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode79 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/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode400 ...
5 years, 3 months ago (2015-09-21 12:59:07 UTC) #13
magjed_webrtc
hbos - PTAL. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode79 talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:79: public List<I420Frame> WaitForFrames() throws InterruptedException { ...
5 years, 3 months ago (2015-09-21 15:27:08 UTC) #14
hbos
https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode420 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 ...
5 years, 3 months ago (2015-09-22 09:12:47 UTC) #15
magjed_webrtc
hbos - Please take another look. https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java (right): https://codereview.webrtc.org/1350863002/diff/120001/talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java#newcode420 talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:420: factory.dispose(); On 2015/09/22 ...
5 years, 3 months ago (2015-09-22 15:04:21 UTC) #16
hbos
Very nice! lgtm https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/1350863002/diff/140001/talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java#newcode477 talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:477: cameraSurfaceTexture = null; On 2015/09/22 15:04:21, ...
5 years, 3 months ago (2015-09-23 07:37:37 UTC) #17
commit-bot: I haz the power
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
5 years, 3 months ago (2015-09-23 08:49:02 UTC) #20
commit-bot: I haz the power
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)
5 years, 3 months ago (2015-09-23 08:57:22 UTC) #22
magjed_webrtc
Committed patchset #5 (id:160001) manually as f706c8ae914da976f16205ff13d15b1a28ead8fd (presubmit successful).
5 years, 3 months ago (2015-09-23 10:01:48 UTC) #23
commit-bot: I haz the power
5 years, 3 months ago (2015-09-23 10:01:48 UTC) #24
Patchset 5 (id:??) landed as
https://crrev.com/f706c8ae914da976f16205ff13d15b1a28ead8fd
Cr-Commit-Position: refs/heads/master@{#10025}

Powered by Google App Engine
This is Rietveld 408576698