Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2071213003:
Allow disabling capture to texture on Camera1Enumerator using constructor parameter. (Closed)
DescriptionAllow disabling capture to texture on Camera1Enumerator using constructor parameter.
The plan is that the CameraEnumerationAndroid will in the future have
method called getEnumerator that will return an enumerator that can be
used to create CameraVideoCapturer objects. It will return
Camera2Enumerator if it is supported or else Camera1Enumerator. Some
apps want to capture to byte buffers which is no longer supported in the
camera2 version of CameraVideoCapturer. Camera1Enumerator constructed
with false parameter as captureToTexture will be returned to these apps.
BUG=webrtc:5519
R=magjed@webrtc.org
Committed: https://crrev.com/b52e81c05466c0d90622b65a4a0427b0329e7331
Cr-Commit-Position: refs/heads/master@{#13294}
Patch Set 1 #Patch Set 2 : Comment. #
Total comments: 7
Patch Set 3 : Changes according to magjed's comments #1 #Messages
Total messages: 24 (11 generated)
Description was changed from ========== Allow disabling capture to texture on Camera1Enumerator using constructor parameter. The plan is that the CameraEnumerationAndroid will in the future have method called getEnumerator that will return an enumerator that can be used to create CameraVideoCapturer objects. It will return Camera2Enumerator if it is supported or else Camera1Enumerator. Some apps want to capture to byte buffers which is no longer supported in the camera2 version of CameraVideoCapturer. Camera1Enumerator constructed with false parameter as captureToTexture will be returned to these apps. BUG=webrtc:5519 ========== to ========== Allow disabling capture to texture on Camera1Enumerator using constructor parameter. The plan is that the CameraEnumerationAndroid will in the future have method called getEnumerator that will return an enumerator that can be used to create CameraVideoCapturer objects. It will return Camera2Enumerator if it is supported or else Camera1Enumerator. Some apps want to capture to byte buffers which is no longer supported in the camera2 version of CameraVideoCapturer. Camera1Enumerator constructed with false parameter as captureToTexture will be returned to these apps. BUG=webrtc:5519 ==========
sakal@webrtc.org changed reviewers: + perkj@webrtc.org
PTAL
sakal@webrtc.org changed reviewers: + nisse@webrtc.org
nisse, PTAL
https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera1Enumerator.java (right): https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera1Enumerator.java:50: return new VideoCapturerAndroid(deviceName, eventsHandler, captureToTexture); This is the only use of the new member variable, right? Would it be possible to have an additional argument to createCapturer instead?
https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera1Enumerator.java (right): https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera1Enumerator.java:50: return new VideoCapturerAndroid(deviceName, eventsHandler, captureToTexture); On 2016/06/20 08:53:05, nisse-webrtc wrote: > This is the only use of the new member variable, right? > Would it be possible to have an additional argument to createCapturer instead? Camera2 implementation of CameraVideoCapturer doesn't support capturing to a byte buffer anymore. This is a method defined by the common interface so adding an argument would require it to be added to the Camera2Enumerator also. Of course, we could just return null in Camera2Enumerator if we get passed false but I think that's more error prone than this solution.
https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera1Enumerator.java (right): https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera1Enumerator.java:50: return new VideoCapturerAndroid(deviceName, eventsHandler, captureToTexture); On 2016/06/20 08:57:30, sakal wrote: > On 2016/06/20 08:53:05, nisse-webrtc wrote: > > This is the only use of the new member variable, right? > > Would it be possible to have an additional argument to createCapturer instead? > > Camera2 implementation of CameraVideoCapturer doesn't support capturing to a > byte buffer anymore. Is it not possible at all to do memory capture in the Camera2 OS API, or just not implemented in webrtc? It seems a bit strange that if an application wants memory frames, it is forced to use a deprecated API (no problem if it's intended as a temporary arrangement, though). > This is a method defined by the common interface so adding > an argument would require it to be added to the Camera2Enumerator also. Of > course, we could just return null in Camera2Enumerator if we get passed false > but I think that's more error prone than this solution. I see, so it's really intended to be an argument to CameraEnumerationAndroid.getEnumerator? From a design point of view, it looks like a collision between two types of "enumerator" designs. Either the enumerator should make everything available, and let the application choose later what it want to use. Or the application decides up front what it can work with, passes some kind of constraints to the enumerator to express that, and get back an enumeration of only the camera devices supporting what the application needs. I guess you and Magnus' have discussed the design? I think I lack the background to understand what's really intended.
https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera1Enumerator.java (right): https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera1Enumerator.java:50: return new VideoCapturerAndroid(deviceName, eventsHandler, captureToTexture); On 2016/06/20 09:36:05, nisse-webrtc wrote: > On 2016/06/20 08:57:30, sakal wrote: > > On 2016/06/20 08:53:05, nisse-webrtc wrote: > > > This is the only use of the new member variable, right? > > > Would it be possible to have an additional argument to createCapturer > instead? > > > > Camera2 implementation of CameraVideoCapturer doesn't support capturing to a > > byte buffer anymore. > > Is it not possible at all to do memory capture in the Camera2 OS API, or just > not implemented in webrtc? It seems a bit strange that if an application wants > memory frames, it is forced to use a deprecated API (no problem if it's intended > as a temporary arrangement, though). I don't capturing to a byte buffer can be done very easily on camera2 API. Magnus said in the lines of: "Hopefully, we don't have to support capturing to a byte buffer on Camera2." > > This is a method defined by the common interface so adding > > an argument would require it to be added to the Camera2Enumerator also. Of > > course, we could just return null in Camera2Enumerator if we get passed false > > but I think that's more error prone than this solution. > > I see, so it's really intended to be an argument to > CameraEnumerationAndroid.getEnumerator? > > From a design point of view, it looks like a collision between two types of > "enumerator" designs. > > Either the enumerator should make everything available, and let the application > choose later what it want to use. > > Or the application decides up front what it can work with, passes some kind of > constraints to the enumerator to express that, and get back an enumeration of > only the camera devices supporting what the application needs. > > I guess you and Magnus' have discussed the design? I think I lack the background > to understand what's really intended. Yes, I plan on adding captureToTexture as an argument to getEnumerator. I am not sure about the API. I thought about making a common enumerator that would return both Camera2 and Camera1 cameras. I am not sure if that would be a cleaner solution.
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
lgtm https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera1Enumerator.java (right): https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera1Enumerator.java:28: private boolean captureToTexture; nit: Make final. https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera1Enumerator.java:50: return new VideoCapturerAndroid(deviceName, eventsHandler, captureToTexture); On 2016/06/20 09:56:50, sakal wrote: > I don't capturing to a byte buffer can be done very easily on camera2 API. > Magnus said in the lines of: "Hopefully, we don't have to support capturing to a > byte buffer on Camera2." The Camera2 API only supports Surfaces, but there is an ImageReader class that can be used for Surface to byte buffer conversion. Hopefully, apps that use camera2 can work with surfaces directly, and if not, they can use the NativeToI420Buffer() function that might be as good/better than ImageReader. > > From a design point of view, it looks like a collision between two types of > > "enumerator" designs. > > > > Either the enumerator should make everything available, and let the > application > > choose later what it want to use. > > > > Or the application decides up front what it can work with, passes some kind of > > constraints to the enumerator to express that, and get back an enumeration of > > only the camera devices supporting what the application needs. > > > > I guess you and Magnus' have discussed the design? I think I lack the > background > > to understand what's really intended. > > Yes, I plan on adding captureToTexture as an argument to getEnumerator. I am not > sure about the API. I thought about making a common enumerator that would return > both Camera2 and Camera1 cameras. I am not sure if that would be a cleaner > solution. The 'enumerator' name was set when the classes only contained helper methods for enumerating camera names and camera formats. The interface now also contains a createCapturer() function. Maybe it would be clearer if CameraEnumerator was renamed to CameraFactory, but I'm ok with the names for now.
https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/Camera1Enumerator.java (right): https://codereview.webrtc.org/2071213003/diff/20001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/Camera1Enumerator.java:28: private boolean captureToTexture; On 2016/06/27 10:41:46, magjed_webrtc wrote: > nit: Make final. Done.
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2071213003/#ps40001 (title: "Changes according to magjed's comments #1")
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 tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14484)
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 tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14489)
Description was changed from ========== Allow disabling capture to texture on Camera1Enumerator using constructor parameter. The plan is that the CameraEnumerationAndroid will in the future have method called getEnumerator that will return an enumerator that can be used to create CameraVideoCapturer objects. It will return Camera2Enumerator if it is supported or else Camera1Enumerator. Some apps want to capture to byte buffers which is no longer supported in the camera2 version of CameraVideoCapturer. Camera1Enumerator constructed with false parameter as captureToTexture will be returned to these apps. BUG=webrtc:5519 ========== to ========== Allow disabling capture to texture on Camera1Enumerator using constructor parameter. The plan is that the CameraEnumerationAndroid will in the future have method called getEnumerator that will return an enumerator that can be used to create CameraVideoCapturer objects. It will return Camera2Enumerator if it is supported or else Camera1Enumerator. Some apps want to capture to byte buffers which is no longer supported in the camera2 version of CameraVideoCapturer. Camera1Enumerator constructed with false parameter as captureToTexture will be returned to these apps. BUG=webrtc:5519 R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/b52e81c05466c0d90622b65a4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as b52e81c05466c0d90622b65a4a0427b0329e7331 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Allow disabling capture to texture on Camera1Enumerator using constructor parameter. The plan is that the CameraEnumerationAndroid will in the future have method called getEnumerator that will return an enumerator that can be used to create CameraVideoCapturer objects. It will return Camera2Enumerator if it is supported or else Camera1Enumerator. Some apps want to capture to byte buffers which is no longer supported in the camera2 version of CameraVideoCapturer. Camera1Enumerator constructed with false parameter as captureToTexture will be returned to these apps. BUG=webrtc:5519 R=magjed@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/b52e81c05466c0d90622b65a4... ========== to ========== Allow disabling capture to texture on Camera1Enumerator using constructor parameter. The plan is that the CameraEnumerationAndroid will in the future have method called getEnumerator that will return an enumerator that can be used to create CameraVideoCapturer objects. It will return Camera2Enumerator if it is supported or else Camera1Enumerator. Some apps want to capture to byte buffers which is no longer supported in the camera2 version of CameraVideoCapturer. Camera1Enumerator constructed with false parameter as captureToTexture will be returned to these apps. BUG=webrtc:5519 R=magjed@webrtc.org Committed: https://crrev.com/b52e81c05466c0d90622b65a4a0427b0329e7331 Cr-Commit-Position: refs/heads/master@{#13294} ========== |
