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

Issue 2168623002: Refactor stopCapture to be asynchronous in VideoCapturerAndroid. (Closed)

Created:
4 years, 5 months ago by sakal
Modified:
4 years, 4 months ago
Reviewers:
magjed_webrtc
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Base URL:
https://chromium.googlesource.com/external/webrtc.git@androidvideotracksource
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Refactor stopCapture to be asynchronous on VideoCapturerAndroid. Also cleanup on the thread safety of the VideoCapturerAndroid at the same time.

Patch Set 1 #

Patch Set 2 : Cleanup #

Total comments: 24

Patch Set 3 : Changes according to magjed's comments. #

Patch Set 4 : Additional fixes. #

Patch Set 5 : Synchronized. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+279 lines, -190 lines) Patch
M webrtc/api/android/java/src/org/webrtc/Camera2Capturer.java View 1 2 3 2 chunks +3 lines, -1 line 0 comments Download
M webrtc/api/android/java/src/org/webrtc/VideoCapturer.java View 1 2 2 chunks +7 lines, -7 lines 0 comments Download
M webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java View 1 2 3 4 17 chunks +262 lines, -169 lines 0 comments Download
M webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java View 1 2 3 5 chunks +5 lines, -5 lines 0 comments Download
M webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java View 2 chunks +2 lines, -8 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 5 (2 generated)
sakal
Magnus, PTAL.
4 years, 5 months ago (2016-07-20 14:04:37 UTC) #3
magjed_webrtc
https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java File webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java#newcode87 webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:87: // Locked objected objects https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java#newcode116 webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:116: cameraState = newState; ...
4 years, 5 months ago (2016-07-20 14:56:08 UTC) #4
sakal
4 years, 5 months ago (2016-07-21 11:17:24 UTC) #5
https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
File webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java (right):

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:87: // Locked
objected
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> objects

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:116:
cameraState = newState;
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> Add a debug log with the current state and the new state.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:236:
cameraState = CameraState.UNINITIALIAZED;
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> I prefer if you move this to the declaration.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:264:
cameraThreadHandler.postAtTime(
On 2016/07/20 14:56:07, magjed_webrtc wrote:
> You have removed the logic for handling failure to post at some places. Can
you
> throw a RuntimeException here if postAtTime returns false? I think it will
only
> happen if someone quits the looper/thread, which should not happen before
> dispose is called.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:272: // Nothing
should be running on the camera thread so should be safe
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> nit: dot at end of sentence.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:277: return
applicationContext != null && capturerObserver != null;
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> Maybe this is cleaner:
> return cameraState != UNINITIALIAZED;
> ?

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:300:
cameraState = CameraState.IDLE;
On 2016/07/20 14:56:07, magjed_webrtc wrote:
> nit: dot at end of sentence.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:330:
firstFrameReported = false;
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> Move this line under the state check.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:397:
synchronized (cameraSwitchLock) {
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> This block should be moved inside the success path of the try-catch statement,
> i.e. close to 'capturerObserver.onCapturerStarted(true);' Also, you should
call
> onCameraSwitchError if we fail to start the camera.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:524:
Logging.d(TAG, "Calling stopCapture() for already stopped camera.");
On 2016/07/20 14:56:08, magjed_webrtc wrote:
> You need to notify generateCapturerEvents here.

I modified code to use the state instead.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:550:
capturerObserver.onCapturerStopped();
On 2016/07/20 14:56:07, magjed_webrtc wrote:
> I think you should notify capturerObserver and eventsHandler regardless of the
> state.

Done.

https://codereview.webrtc.org/2168623002/diff/20001/webrtc/api/android/java/s...
webrtc/api/android/java/src/org/webrtc/VideoCapturerAndroid.java:587: if
(cameraState == CameraState.IDLE) {
On 2016/07/20 14:56:07, magjed_webrtc wrote:
> Check for UNINITIALIZED as well? Or make the opposite check, i.e. state !=
> STARTING && state != RUNNING.

I made every public function to check if camera is initialized.

Powered by Google App Engine
This is Rietveld 408576698