Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1895483002:
Android: Make base interface for camera1 and camera2 (Closed)
|
Created:
4 years, 8 months ago by magjed_webrtc Modified:
4 years, 7 months ago Reviewers:
perkj_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid: Make base interface for camera1 and camera2
This CL adds a new interface CameraVideoCapturer that extends VideoCapturer with a switchCamera() function. It also moves moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to this new interface. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class.
BUG=webrtc:5519
Committed: https://crrev.com/6bdacaddfb18edef1f0cdd778209f6b05a8f9210
Cr-Commit-Position: refs/heads/master@{#12723}
Patch Set 1 : #
Total comments: 15
Patch Set 2 : perkj@s comments #
Total comments: 2
Patch Set 3 : Add one more thread comment #
Messages
Total messages: 20 (9 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Move interfaces from camera1 implementation to common VideoCapturer class This CL moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to VideoCapturer. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 ========== to ========== Move interfaces from camera1 implementation to common VideoCapturer class This CL moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to VideoCapturer. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 ==========
magjed@webrtc.org changed reviewers: + perkj@webrtc.org
perkj - Please take a look.
Description was changed from ========== Move interfaces from camera1 implementation to common VideoCapturer class This CL moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to VideoCapturer. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 ========== to ========== Android: Make base interface for camera1 and camera2 This CL adds a new interface CameraVideoCapturer that extends VideoCapturer with a switchCamera() function. It also moves moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to this new interface. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 ==========
Patchset #1 (id:20001) has been deleted
Ping. perkj - please take a look.
Sorry - I had missed this. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:20: * Camera events handler - can be used to be notifed about camera events. add thread note here to please. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:27: // Invoked when camera stops receiving frames nit: frames. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:33: // Callback invoked when first camera frame is available after camera is opened. nit: is started? https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:36: // Callback invoked when camera closed. // Callback invoked when camera is closed. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:53: * Helper class to log framerate and detect if the camera freezes. on which thread? https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:55: public static class CameraStatistics { protected: Only an implementation can instantiate and use. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:123: void switchCamera(CameraSwitchHandler switchEventsHandler); Suggest moving this method below CameraSwitchHandler and have public interfaces and methods at the top and protected below.
https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:20: * Camera events handler - can be used to be notifed about camera events. On 2016/04/21 08:14:56, perkj_webrtc wrote: > add thread note here to please. Done. Please note that this code is just moved from VideoCapturerAndroid.java. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:27: // Invoked when camera stops receiving frames On 2016/04/21 08:14:56, perkj_webrtc wrote: > nit: frames. Done. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:33: // Callback invoked when first camera frame is available after camera is opened. On 2016/04/21 08:14:56, perkj_webrtc wrote: > nit: is started? Done. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:36: // Callback invoked when camera closed. On 2016/04/21 08:14:56, perkj_webrtc wrote: > // Callback invoked when camera is closed. Done. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:53: * Helper class to log framerate and detect if the camera freezes. On 2016/04/21 08:14:56, perkj_webrtc wrote: > on which thread? Done. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:55: public static class CameraStatistics { On 2016/04/21 08:14:56, perkj_webrtc wrote: > protected: Only an implementation can instantiate and use. Done. https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:123: void switchCamera(CameraSwitchHandler switchEventsHandler); On 2016/04/21 08:14:56, perkj_webrtc wrote: > Suggest moving this method below CameraSwitchHandler and have public interfaces > and methods at the top and protected below. Done.
https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/1895483002/diff/40001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:55: public static class CameraStatistics { On 2016/05/12 11:42:43, magjed_webrtc wrote: > On 2016/04/21 08:14:56, perkj_webrtc wrote: > > protected: Only an implementation can instantiate and use. > > Done. Wait, it's not possible to have protected classes in an interface.
lgtm https://codereview.webrtc.org/1895483002/diff/60001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/1895483002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:56: void switchCamera(CameraSwitchHandler switchEventsHandler); nit: on which thread?
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1895483002/#ps80001 (title: "Add one more thread comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895483002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895483002/80001
https://codereview.webrtc.org/1895483002/diff/60001/webrtc/api/java/android/o... File webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/1895483002/diff/60001/webrtc/api/java/android/o... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:56: void switchCamera(CameraSwitchHandler switchEventsHandler); On 2016/05/12 13:04:32, perkj_webrtc wrote: > nit: on which thread? Done.
Message was sent while issue was closed.
Description was changed from ========== Android: Make base interface for camera1 and camera2 This CL adds a new interface CameraVideoCapturer that extends VideoCapturer with a switchCamera() function. It also moves moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to this new interface. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 ========== to ========== Android: Make base interface for camera1 and camera2 This CL adds a new interface CameraVideoCapturer that extends VideoCapturer with a switchCamera() function. It also moves moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to this new interface. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Android: Make base interface for camera1 and camera2 This CL adds a new interface CameraVideoCapturer that extends VideoCapturer with a switchCamera() function. It also moves moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to this new interface. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 ========== to ========== Android: Make base interface for camera1 and camera2 This CL adds a new interface CameraVideoCapturer that extends VideoCapturer with a switchCamera() function. It also moves moves CameraEventsHandler, CameraStatistics, and CameraSwitchHandler from VideoCapturerAndroid to this new interface. The purpose is to prepare for a camera2 implementation that will use the same interfaces and helper class. BUG=webrtc:5519 Committed: https://crrev.com/6bdacaddfb18edef1f0cdd778209f6b05a8f9210 Cr-Commit-Position: refs/heads/master@{#12723} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6bdacaddfb18edef1f0cdd778209f6b05a8f9210 Cr-Commit-Position: refs/heads/master@{#12723}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:80001) has been created in https://codereview.webrtc.org/1979583002/ by magjed@webrtc.org. The reason for reverting is: Breaks downstream import.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
