|
|
Created:
4 years, 6 months ago by sakal Modified:
4 years, 5 months ago Reviewers:
magjed_webrtc CC:
tterriberry_mozilla.com, webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@enumerator_change Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid: Camera2 implementation and tests for it.
BUG=webrtc:5519
R=magjed@webrtc.org
Committed: https://crrev.com/8cf2a3a3ad2fc635d20a9912ad8a7a80e4c0c523
Cr-Commit-Position: refs/heads/master@{#13320}
Patch Set 1 : WIP #Patch Set 2 : Remove useless imports. #Patch Set 3 : Camera2Enumerator constructor should be public. #Patch Set 4 : Longer delimeter. #
Total comments: 44
Patch Set 5 : Changes according to magjed's comments #1 #
Total comments: 4
Patch Set 6 : Remove exception capturing in switchCamera. #Patch Set 7 : Add override annotations in Camera2Enumerator. #Patch Set 8 : Make stopCapture non-blocking. #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 46 (22 generated)
Description was changed from ========== Android: Camera2 implementation and tests for it. BUG=webrtc:5519 ========== to ========== Android: Camera2 implementation and tests for it. BUG=webrtc:5519 ==========
sakal@webrtc.org changed reviewers: + perkj@webrtc.org
PTAL
sakal@webrtc.org changed reviewers: + nisse@webrtc.org
nisse, PTAL
I don't think I can't review this properly. I was a bit confused by the tests, they didn't seem to exercise the new Camera2 capturer and enumerator classes?
On 2016/06/20 09:18:39, nisse-webrtc wrote: > I don't think I can't review this properly. > > I was a bit confused by the tests, they didn't seem to exercise the new Camera2 > capturer and enumerator classes? In TestObjectFactory, getCameraEnumerator() returns a Camera2Enumerator. The enumerator is then used to create Camera2Capturer objects for the tests in CameraVideoCapturerTestFixtures.
On 2016/06/20 09:23:45, sakal wrote: > On 2016/06/20 09:18:39, nisse-webrtc wrote: > > I don't think I can't review this properly. > > > > I was a bit confused by the tests, they didn't seem to exercise the new > Camera2 > > capturer and enumerator classes? > > In TestObjectFactory, getCameraEnumerator() returns a Camera2Enumerator. The > enumerator is then used to create Camera2Capturer objects for the tests in > CameraVideoCapturerTestFixtures. I see, thanks for the explanation. It was a bit obscured by the SimpleCamera2 class. It would be nice if the rawOpenCamera thing could be deleted ("CameraVideoCapturer API is too slow for some of our tests where we need to open a competing camera."), but that's for a different cl, I guess.
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera2Capturer.java (right): https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:50: private final static Object STOP_TIMEOUT_RUNNABLE_TOKEN = "STOP_TIMEOUT_RUNNABLE_TOKEN"; What's the benefit of using a string vs just "new Object()"? Will it ever be printed? https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:113: * Helper method for checking method is executed on camera thread. Also allows calls from other nit: "...calls from other _threads_ if camera is closed" https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:153: CameraState transitionalState, long timeout) { nit: add ms as suffix, i.e. timeoutMs https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:191: * Wait until camera state is not STARTING. Returns if true if camera is nit: Update comment - this function does not return anything. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:199: * Sets the name of the camera. Camera must be stopped when this is called. nit: Update comment, it's ok to call it in any state since waitForCameraToStopIfStopping() is used? https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:328: if(cameraThreadHandler == null) { nit: space after if... https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:391: * nit: missing a comment. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:414: CameraCharacteristics cameraCharacteristics = null; nit: Make final and don't initialize to null. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:478: throw new IllegalStateException("Unexpected camera state for startCapture: " I think we should Log.e and return instead of throwing here. This function is called from outside this class, so it's hard to control correct usage. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:528: consecutiveCameraOpenFailures++; I think we should wait with adding this retry logic until we know it actually helps for the camera2 case. Unless you already know it's needed? I'm guessing you added it since camera1 has it. This is the kind of band aid we bolt on later :) https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:558: // StreamConfigurationMap.getOutputSizes(SurfaceTexture.class) Remove this comment https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:702: if(cameraState != CameraState.RUNNING) { nit: space after if https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:733: // TODO(magjed): Handle failure if camera does not exist anymore? Please remove TODO. Hopefully improbable enough to never happen. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:738: new Thread() { An alternative implementation is to store switchEventsHandler in a member variable and call it directly from onConfigured() instead of creating a new thread. I'm not sure what I dislike most though, new threads or new member state. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:743: if(cameraState == CameraState.RUNNING) { nit: space after if https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:772: if (cameraDevice == null) { Can you check if |capturerObserver| is null instead of |cameraDevice|? That would be more intuitive. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:794: if(cameraState != CameraState.RUNNING) { nit: space after if... https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:810: // TODO(magjed): Just recreate session. You can add your name to the TODO if you want. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:811: // TODO(magjed): Handle failure if camera does not exist anymore? Remove the last TODO. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera2Enumerator.java (right): https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Enumerator.java:161: if (defaultMaxFps < 1000) { This check should not be needed for CaptureFormat.FramerateRange because it's always multiplied 1000? https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Enumerator.java:190: public static List<Size> convertSizes(android.util.Size[] cameraSizes) { Can this be a private method instead? https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Enumerator.java:199: public static List<CaptureFormat.FramerateRange> convertFramerates( ditto, private?
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera2Capturer.java (right): https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:50: private final static Object STOP_TIMEOUT_RUNNABLE_TOKEN = "STOP_TIMEOUT_RUNNABLE_TOKEN"; On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > What's the benefit of using a string vs just "new Object()"? Will it ever be printed? No, I don't think it will be printed. I changed this to new Object(). I don't think it really matters though. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:113: * Helper method for checking method is executed on camera thread. Also allows calls from other On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > nit: "...calls from other _threads_ if camera is closed" Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:153: CameraState transitionalState, long timeout) { On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > nit: add ms as suffix, i.e. timeoutMs Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:191: * Wait until camera state is not STARTING. Returns if true if camera is On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > nit: Update comment - this function does not return anything. Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:199: * Sets the name of the camera. Camera must be stopped when this is called. On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > nit: Update comment, it's ok to call it in any state since waitForCameraToStopIfStopping() is used? This function should only be called when camera is stopped or it is stopping. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:328: if(cameraThreadHandler == null) { On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > nit: space after if... Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:391: * On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > nit: missing a comment. Added a comment to startCapture instead. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:414: CameraCharacteristics cameraCharacteristics = null; On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > nit: Make final and don't initialize to null. Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:478: throw new IllegalStateException("Unexpected camera state for startCapture: " On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > I think we should Log.e and return instead of throwing here. This function is called from outside this class, so it's hard to control correct usage. Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:528: consecutiveCameraOpenFailures++; On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > I think we should wait with adding this retry logic until we know it actually helps for the camera2 case. Unless you already know it's needed? I'm guessing you added it since camera1 has it. This is the kind of band aid we bolt on later :) This code is necessary for the startWhileCameraIsAlreadyOpenAndCloseCamera test to pass. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:558: // StreamConfigurationMap.getOutputSizes(SurfaceTexture.class) On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > Remove this comment Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:702: if(cameraState != CameraState.RUNNING) { On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > nit: space after if Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:733: // TODO(magjed): Handle failure if camera does not exist anymore? On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > Please remove TODO. Hopefully improbable enough to never happen. Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:738: new Thread() { On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > An alternative implementation is to store switchEventsHandler in a member variable and call it directly from onConfigured() instead of creating a new thread. I'm not sure what I dislike most though, new threads or new member state. Hmm, I think calling the callbacks in onConfigured is a bit cleaner. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:743: if(cameraState == CameraState.RUNNING) { On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > nit: space after if Code removed. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:772: if (cameraDevice == null) { On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > Can you check if |capturerObserver| is null instead of |cameraDevice|? That would be more intuitive. Should not make a difference since they are both set to null on closeAndRelease. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:794: if(cameraState != CameraState.RUNNING) { On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > nit: space after if... Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:810: // TODO(magjed): Just recreate session. On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > You can add your name to the TODO if you want. Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:811: // TODO(magjed): Handle failure if camera does not exist anymore? On 2016/06/27 at 10:33:23, magjed_webrtc wrote: > Remove the last TODO. Done. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera2Enumerator.java (right): https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Enumerator.java:161: if (defaultMaxFps < 1000) { On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > This check should not be needed for CaptureFormat.FramerateRange because it's always multiplied 1000? You're right. https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Enumerator.java:190: public static List<Size> convertSizes(android.util.Size[] cameraSizes) { On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > Can this be a private method instead? I changed this. I reduced visibility in Camera1Enumerator as well.
https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera2Capturer.java (right): https://codereview.webrtc.org/2078473002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:528: consecutiveCameraOpenFailures++; On 2016/06/27 12:08:27, sakal wrote: > On 2016/06/27 at 10:33:24, magjed_webrtc wrote: > > I think we should wait with adding this retry logic until we know it actually > helps for the camera2 case. Unless you already know it's needed? I'm guessing > you added it since camera1 has it. This is the kind of band aid we bolt on later > :) > > This code is necessary for the startWhileCameraIsAlreadyOpenAndCloseCamera test > to pass. Alright. Sounds like a strange test though, and racy. https://codereview.webrtc.org/2078473002/diff/140001/webrtc/api/java/android/... File webrtc/api/java/android/org/webrtc/Camera2Capturer.java (right): https://codereview.webrtc.org/2078473002/diff/140001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:677: public void switchCamera(final CameraSwitchHandler switchEventsHandler) { This function is blocking because of the stopCapture() call. I think we should make it asynchronous so we don't block apps that might call it from e.g. the UI thread. Is it possible to just post this code as a Runnable to the camera thread? https://codereview.webrtc.org/2078473002/diff/140001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:752: } catch (Exception e) { I guess this try-catch is the recommended way of using java semaphore, but what exceptions are we actually catching here? Can you make it more fine grained to e.g. CameraAccessException?
https://codereview.webrtc.org/2078473002/diff/140001/webrtc/api/java/android/... File webrtc/api/java/android/org/webrtc/Camera2Capturer.java (right): https://codereview.webrtc.org/2078473002/diff/140001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:677: public void switchCamera(final CameraSwitchHandler switchEventsHandler) { On 2016/06/27 13:18:38, magjed_webrtc wrote: > This function is blocking because of the stopCapture() call. I think we should > make it asynchronous so we don't block apps that might call it from e.g. the UI > thread. Is it possible to just post this code as a Runnable to the camera > thread? Sadly, it is not possible to post on the CameraThread because that can lead to a deadlock. Discussed offline, decided to leave for now. https://codereview.webrtc.org/2078473002/diff/140001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:752: } catch (Exception e) { On 2016/06/27 13:18:38, magjed_webrtc wrote: > I guess this try-catch is the recommended way of using java semaphore, but what > exceptions are we actually catching here? Can you make it more fine grained to > e.g. CameraAccessException? Decided to remove this try-catch completely.
The CQ bit was checked by sakal@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: Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14494)
lgtm. Ship it!
The CQ bit was checked by sakal@webrtc.org
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
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14543) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6551)
sakal@webrtc.org changed reviewers: + tommi@webrtc.org
Tommi, can you take a look please? I think your approval is needed because of the ThreadUtils change.
The CQ bit was checked by sakal@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: Try jobs failed on following builders: mac_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10484)
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by sakal@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: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14560)
sakal@webrtc.org changed reviewers: - nisse@webrtc.org, perkj@webrtc.org, tommi@webrtc.org
I made the stopCapture non-blocking. PTAL
lgtm https://codereview.webrtc.org/2078473002/diff/220001/webrtc/api/java/android/... File webrtc/api/java/android/org/webrtc/Camera2Capturer.java (right): https://codereview.webrtc.org/2078473002/diff/220001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/Camera2Capturer.java:678: public void switchCamera(final CameraSwitchHandler switchEventsHandler) { I think this functions needs to be made asynchronous as well, but it's ok to land this CL first. You can add a TODO if you want.
The CQ bit was checked by sakal@webrtc.org
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
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14586)
Description was changed from ========== Android: Camera2 implementation and tests for it. BUG=webrtc:5519 ========== to ========== Android: Camera2 implementation and tests for it. BUG=webrtc:5519 R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/8cf2a3a3ad2fc635d20a9912a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001) manually as 8cf2a3a3ad2fc635d20a9912ad8a7a80e4c0c523 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Android: Camera2 implementation and tests for it. BUG=webrtc:5519 R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/8cf2a3a3ad2fc635d20a9912a... ========== to ========== Android: Camera2 implementation and tests for it. BUG=webrtc:5519 R=magjed@webrtc.org Committed: https://crrev.com/8cf2a3a3ad2fc635d20a9912ad8a7a80e4c0c523 Cr-Commit-Position: refs/heads/master@{#13320} ========== |