Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833773003:
Support adding and removing MediaRecorder to camera 2 session. (Closed)
DescriptionSupport adding and removing MediaRecorder to camera 2 session.
Camera 1 API is not supported.
BUG=b/36684011
Review-Url: https://codereview.webrtc.org/2833773003
Cr-Commit-Position: refs/heads/master@{#17901}
Committed: https://chromium.googlesource.com/external/webrtc/+/2fc04769faec0031a202963eaeb602420a082c07
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments #
Total comments: 9
Patch Set 3 : Address comments - 2 #Patch Set 4 : Add unit test #
Total comments: 8
Patch Set 5 : Address comments - 3 #
Messages
Total messages: 28 (12 generated)
glaznev@webrtc.org changed reviewers: + magjed@webrtc.org, sakal@webrtc.org
PTAL
Overall looks great. It passes a lot of responsibility to the application on when the media recorder can be added but that is okay as long as the application can handle it. Is it possible to add simple unit tests for this? We already have some tests in Camera2CapturerTest class. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/Camera2Session.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/Camera2Session.java:68: private Surface mediaRecorderSurface; move below surfaceTextureHelper and make final https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:234: private MediaRecorderState mediaRecorderState = MediaRecorderState.IDLE; Since these are only manipulated on the camera thread, I don't think a lock is needed. Make a new section for variables only used on the camera thread. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:482: Logging.e(TAG, error); Please add checkIsOnCameraThread() https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:490: Logging.d(TAG, Please add checkIsOnCameraThread()
https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:65: } else if (mediaRecorderState == MediaRecorderState.IN_PROGRESS) { One more thing, this shouldn't be else if. switchState can become PENDING while mediaRecorderState is IN_PROGRESS. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:70: mediaRecorder = null; Why is mediaRecorder set to null here? This means cameraSwitch will cause the mediaRecorder to be removed. Is this intended?
I also think this approach looks good. We should decide what combinations of switchCamera, start/stop, and addMediaRecorderToCamera/removeMediaRecorderFromCamera are allowed. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/CameraVideoCapturer.java:65: * Media recorder handler - one of these functions are invoked with the result of switchCamera(). nit: not switchCamera(), but addMediaRecorderToCamera()/removeMediaRecorderFromCamera(). https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:451: if (sessionOpening) { We should decide if it's ok or not to call switchCamera in state 'mediaRecorderState == MediaRecorderState.IN_PROGRESS'. I don't think it's working currently.
https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/CameraVideoCapturer.java:65: * Media recorder handler - one of these functions are invoked with the result of switchCamera(). On 2017/04/21 10:45:16, magjed_webrtc wrote: > nit: not switchCamera(), but > addMediaRecorderToCamera()/removeMediaRecorderFromCamera(). Done. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/Camera2Session.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/Camera2Session.java:68: private Surface mediaRecorderSurface; On 2017/04/21 07:56:48, sakal wrote: > move below surfaceTextureHelper and make final Done. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:65: } else if (mediaRecorderState == MediaRecorderState.IN_PROGRESS) { On 2017/04/21 08:06:15, sakal wrote: > One more thing, this shouldn't be else if. switchState can become PENDING while > mediaRecorderState is IN_PROGRESS. Done. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:70: mediaRecorder = null; On 2017/04/21 08:06:15, sakal wrote: > Why is mediaRecorder set to null here? This means cameraSwitch will cause the > mediaRecorder to be removed. Is this intended? Yes, camera switch is not allowed while MediaRecorder is added to camera pipeline. I will add double check to disallow camera switch to start when media recording is in progress. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:234: private MediaRecorderState mediaRecorderState = MediaRecorderState.IDLE; On 2017/04/21 07:56:49, sakal wrote: > Since these are only manipulated on the camera thread, I don't think a lock is > needed. Make a new section for variables only used on the camera thread. Done. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:451: if (sessionOpening) { On 2017/04/21 10:45:16, magjed_webrtc wrote: > We should decide if it's ok or not to call switchCamera in state > 'mediaRecorderState == MediaRecorderState.IN_PROGRESS'. I don't think it's > working currently. I think this is not allowed - we can not reconfigure session while MediaRecorder is added to it. I added a check to not allow camera switch to start if MediaRecorder surface is added to camera pipeline. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:482: Logging.e(TAG, error); On 2017/04/21 07:56:49, sakal wrote: > Please add checkIsOnCameraThread() Done. https://codereview.webrtc.org/2833773003/diff/1/webrtc/sdk/android/src/java/o... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:490: Logging.d(TAG, On 2017/04/21 07:56:49, sakal wrote: > Please add checkIsOnCameraThread() Done.
https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:508: private void updateMediaRecorderInternal(boolean addMediaRecorder, MediaRecorder mediaRecorder, I would prefer to not have |addMediaRecorder| as an argument and instead have: final boolean addMediaRecorder = (mediaRecorder != null); in this function. https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:534: if (addMediaRecorder) { I would like to move 'this.mediaRecorder = mediaRecorder;' outside this if-statement. https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:542: Logging.d(TAG, "updateMediaRecoder: Stopping session"); The documentation for addMediaRecorderToCamera says: "This can only be called while the camera is running.", but we never check if 'currentSession != null' here, and we will actually crash if currentSession is null. So we should probably call reportUpdateMediaRecorderError for that case. However, would it be interesting to support the use case when the MediaRecorder is added before the call to startCapture? I.e. support this: capturer.addMediaRecorderToCamera(...); capturer.startCapture(...); That wouldn't require restarting the session and will save some time. It looks like you can just return here if currentSession is null in order to support it.
https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:234: private MediaRecorder mediaRecorder; I don't think this member variable is needed. It can just be an argument to createSessionInternal. The less state we have the better. At least it is not initialized on initialize so it doesn't belong here.
https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:234: private MediaRecorder mediaRecorder; On 2017/04/24 09:17:32, sakal wrote: > I don't think this member variable is needed. It can just be an argument to > createSessionInternal. The less state we have the better. > > At least it is not initialized on initialize so it doesn't belong here. This is actually used during re attempting to open camera session - in createSessionCallback.onFailure. We need to cache the value to use during these attempts - similar to say cameraName. I moved the declaration to a better place https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:508: private void updateMediaRecorderInternal(boolean addMediaRecorder, MediaRecorder mediaRecorder, On 2017/04/22 08:41:52, magjed_webrtc wrote: > I would prefer to not have |addMediaRecorder| as an argument and instead have: > final boolean addMediaRecorder = (mediaRecorder != null); > in this function. Done. https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:534: if (addMediaRecorder) { On 2017/04/22 08:41:52, magjed_webrtc wrote: > I would like to move 'this.mediaRecorder = mediaRecorder;' outside this > if-statement. Done. https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:542: Logging.d(TAG, "updateMediaRecoder: Stopping session"); On 2017/04/22 08:41:52, magjed_webrtc wrote: > The documentation for addMediaRecorderToCamera says: "This can only be called > while the camera is running.", but we never check if 'currentSession != null' > here, and we will actually crash if currentSession is null. So we should > probably call reportUpdateMediaRecorderError for that case. > > However, would it be interesting to support the use case when the MediaRecorder > is added before the call to startCapture? I.e. support this: > capturer.addMediaRecorderToCamera(...); > capturer.startCapture(...); > That wouldn't require restarting the session and will save some time. It looks > like you can just return here if currentSession is null in order to support it. Done. Yes, this case mat be supported as well, but I suggest to consider it later. This may be a complex use case since it passes more responsibility from app to WebRTC layer. It may also have some bad impact on power usage, since full MediaRecorder preparation need to be done before it can be added to camera and it keeping non released MEdiaRecorder for a log time may increase power/memory usage
The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/20001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:234: private MediaRecorder mediaRecorder; On 2017/04/25 23:11:51, AlexG wrote: > On 2017/04/24 09:17:32, sakal wrote: > > I don't think this member variable is needed. It can just be an argument to > > createSessionInternal. The less state we have the better. > > > > At least it is not initialized on initialize so it doesn't belong here. > > This is actually used during re attempting to open camera session - in > createSessionCallback.onFailure. We need to cache the value to use during these > attempts - similar to say cameraName. I moved the declaration to a better place But you set openAttemptsRemaining = 1; in updateMediaRecorderInternal so there will be no retries? https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... File webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:514: Logging.d(TAG, "updateMediaRecorder!!!"); Probably a debug comment, please remove https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:527: if (useSurfaceCapture) { nit: I would prefer a ternary operator here https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/src/ja... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:538: if (addMediaRecorder) { nit: can be replaced with a ternary operator
Looks good now https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... File webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:596: assertTrue(videoTrackWithRenderer.rendererCallbacks.waitForNextFrameToRender() > 0); Can we make some check on outputFile?
https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... File webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:514: Logging.d(TAG, "updateMediaRecorder!!!"); On 2017/04/26 07:34:11, sakal wrote: > Probably a debug comment, please remove Done. https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:527: if (useSurfaceCapture) { On 2017/04/26 07:34:11, sakal wrote: > nit: I would prefer a ternary operator here Done. https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/instru... webrtc/sdk/android/instrumentationtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:596: assertTrue(videoTrackWithRenderer.rendererCallbacks.waitForNextFrameToRender() > 0); On 2017/04/26 16:01:41, magjed_webrtc wrote: > Can we make some check on outputFile? Done. https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/src/ja... File webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java (right): https://codereview.webrtc.org/2833773003/diff/60001/webrtc/sdk/android/src/ja... webrtc/sdk/android/src/java/org/webrtc/CameraCapturer.java:538: if (addMediaRecorder) { On 2017/04/26 07:34:12, sakal wrote: > nit: can be replaced with a ternary operator Done.
The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by magjed@webrtc.org
lgtm
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493280610019230,
"parent_rev": "e173802797ddeb15364a8241fc51121c3192fefd", "commit_rev":
"2fc04769faec0031a202963eaeb602420a082c07"}
Message was sent while issue was closed.
Description was changed from ========== Support adding and removing MediaRecorder to camera 2 session. Camera 1 API is not supported. BUG=b/36684011 ========== to ========== Support adding and removing MediaRecorder to camera 2 session. Camera 1 API is not supported. BUG=b/36684011 Review-Url: https://codereview.webrtc.org/2833773003 Cr-Commit-Position: refs/heads/master@{#17901} Committed: https://chromium.googlesource.com/external/webrtc/+/2fc04769faec0031a202963ea... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/2fc04769faec0031a202963ea...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.webrtc.org/2844233002/ by magjed@webrtc.org. The reason for reverting is: Breaks external bot. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
