Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11860002:
Add video capture on Android. (Closed)
|
Created:
7 years, 11 months ago by wjia(left Chromium) Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, jochen+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd video capture on Android.
The capture video frames are obtained by using camera's PreviewCallbackWithBuffer.
Most of VideoCaptureDevice unit tests are disabled on Android due to different data flow and threading.
BUG=161417
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181839
Patch Set 1 #Patch Set 2 : #Patch Set 3 : fix threading #Patch Set 4 : support generic rotation #Patch Set 5 : final #Patch Set 6 : ready for review #
Total comments: 28
Patch Set 7 : code review #
Total comments: 8
Patch Set 8 : #
Total comments: 4
Patch Set 9 : fix unit tests #
Total comments: 23
Patch Set 10 : code review and rebase #Patch Set 11 : #
Total comments: 2
Patch Set 12 : code review #
Total comments: 88
Patch Set 13 : code review and rebase #
Total comments: 24
Patch Set 14 : rebase #Patch Set 15 : code review #
Total comments: 8
Patch Set 16 : more code review #
Total comments: 4
Patch Set 17 : restore some missing lines #Patch Set 18 : rebase #Patch Set 19 : add back CHECK against width #
Total comments: 26
Patch Set 20 : code review #
Total comments: 13
Patch Set 21 : code review #
Total comments: 4
Patch Set 22 : catch only IOException #Messages
Total messages: 33 (0 generated)
The video capture device for android is ready. Please start with patch set 6. Thanks!
https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:175: byte[] buffer = new byte[bufSize]; indent by 4 https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:282: mDeviceOrientation = rotation; indent by 4 https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:319: Context.WINDOW_SERVICE); indent by 8 https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:321: case Surface.ROTATION_0 : orientation = 0 ; break; use new lines https://codereview.chromium.org/11860002/diff/34001/media/media.gyp File media/media.gyp (left): https://codereview.chromium.org/11860002/diff/34001/media/media.gyp#oldcode554 media/media.gyp:554: # Video capture isn't supported in Android yet. change this comment https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:22: void OnFrameAvailable(JNIEnv* env, jobject obj, why this one does not fit into an empty namespace? https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:37: static_cast<int>(length), I think there is no need to cast jint to int https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:170: if (num_cameras <= 0) { remove { for single line if statement https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:192: //for (int camera_id = 0; camera_id < num_cameras; ++camera_id) { remove this line https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:227: if (!self || !self->Init()) { no { https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:386: if (state_ != kCapturing || !observer_) { no {
PTAL. https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:175: byte[] buffer = new byte[bufSize]; On 2013/01/22 21:31:42, qinmin wrote: > indent by 4 Done. https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:282: mDeviceOrientation = rotation; On 2013/01/22 21:31:42, qinmin wrote: > indent by 4 Done. https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:319: Context.WINDOW_SERVICE); On 2013/01/22 21:31:42, qinmin wrote: > indent by 8 Done. https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:321: case Surface.ROTATION_0 : orientation = 0 ; break; On 2013/01/22 21:31:42, qinmin wrote: > use new lines Done. https://codereview.chromium.org/11860002/diff/34001/media/media.gyp File media/media.gyp (left): https://codereview.chromium.org/11860002/diff/34001/media/media.gyp#oldcode554 media/media.gyp:554: # Video capture isn't supported in Android yet. On 2013/01/22 21:31:42, qinmin wrote: > change this comment Done and merge it with same condition above. https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:22: void OnFrameAvailable(JNIEnv* env, jobject obj, On 2013/01/22 21:31:42, qinmin wrote: > why this one does not fit into an empty namespace? This is the callback from VideoCapture JAVA class via JNI. The jni header file defines it without namespace. Comments have been added. https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:37: static_cast<int>(length), On 2013/01/22 21:31:42, qinmin wrote: > I think there is no need to cast jint to int Done, and remove all unneeded static_cast. https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:170: if (num_cameras <= 0) { On 2013/01/22 21:31:42, qinmin wrote: > remove { for single line if statement Done. https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:192: //for (int camera_id = 0; camera_id < num_cameras; ++camera_id) { On 2013/01/22 21:31:42, qinmin wrote: > remove this line Done. https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:227: if (!self || !self->Init()) { On 2013/01/22 21:31:42, qinmin wrote: > no { Done. https://codereview.chromium.org/11860002/diff/34001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:386: if (state_ != kCapturing || !observer_) { On 2013/01/22 21:31:42, qinmin wrote: > no { Done.
https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:211: if (mIsRunning) { not unlock mPreviewBufferLock https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:236: if (!mIsRunning) { not unlock mPreviewBufferLock https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:309: public void onFrameAvailable(SurfaceTexture surfaceTexture) { } Is interface OnFrameAvailableListener necessary since its callback is no-op. How about add TODO if it's going to be implemented in future ? https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:33: LOG(ERROR) << "OnFrmaeAvailable: failed to GetByteArrayElements"; OnFrmaeAvailable ~~ https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:105: so pixels in square are shown, isn't it ? will stretch be supported ? https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:219: << orientation; orientation seems not used except for DVLOG, can remove ? https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:396: if no rotation/flip, could pass original buffer to observer_ directly, avoiding three separate fast memcpys
https://codereview.chromium.org/11860002/diff/43001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/43001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:63: Log.d("JAVA VideoCapture", "createVideoCapture: " + id); can you define a static final string TAG = "JVC", and use this TAG in log? https://codereview.chromium.org/11860002/diff/43001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:174: } Instead of using callback buffer, it's worth of investigating the performance difference between read data from texture. can you add a TODO?
Thanks! PTAL. Now all try bots are green. https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:211: if (mIsRunning) { On 2013/01/23 05:09:24, zhongping.wang wrote: > not unlock mPreviewBufferLock Switched to use try{} finally{}. https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:236: if (!mIsRunning) { On 2013/01/23 05:09:24, zhongping.wang wrote: > not unlock mPreviewBufferLock Done. https://codereview.chromium.org/11860002/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:309: public void onFrameAvailable(SurfaceTexture surfaceTexture) { } On 2013/01/23 05:09:24, zhongping.wang wrote: > Is interface OnFrameAvailableListener necessary since its callback is no-op. How > about add TODO if it's going to be implemented in future ? Done. https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:33: LOG(ERROR) << "OnFrmaeAvailable: failed to GetByteArrayElements"; On 2013/01/23 05:09:24, zhongping.wang wrote: > OnFrmaeAvailable > ~~ Done. https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:105: On 2013/01/23 05:09:24, zhongping.wang wrote: > so pixels in square are shown, isn't it ? will stretch be supported ? For 90/270 rotation, the pixels in square are shown. Scaling is pretty expensive and will not be supported for now. https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:219: << orientation; On 2013/01/23 05:09:24, zhongping.wang wrote: > orientation seems not used except for DVLOG, can remove ? It's helpful to know orientation of camera in case rotation goes wrong. https://codereview.chromium.org/11860002/diff/38003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:396: On 2013/01/23 05:09:24, zhongping.wang wrote: > if no rotation/flip, could pass original buffer to observer_ directly, avoiding > three separate fast memcpys Yes. The rotation code is written in such a way that it can be moved into VideoCaptureController where the pixels will be copied into shared memory. In other word, VideoCaptureDevice will not do rotation. It just passes rotation info to VCC and VCC will do pixel copying and necessary rotation. https://codereview.chromium.org/11860002/diff/43001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/43001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:63: Log.d("JAVA VideoCapture", "createVideoCapture: " + id); On 2013/01/23 23:36:24, leozwang wrote: > can you define a static final string TAG = "JVC", and use this TAG in log? Done. https://codereview.chromium.org/11860002/diff/43001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:174: } On 2013/01/23 23:36:24, leozwang wrote: > Instead of using callback buffer, it's worth of investigating the performance > difference between read data from texture. can you add a TODO? Done. The comment is added at OnFrameAvailable(SurfaceTexture).
https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:30: public class VideoCapture implements PreviewCallback, OnFrameAvailableListener { add @JNINamespace("xxxx"), so the calls to native will have namespace appended https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:42: private boolean mIsRunning=false; spaces before and after "=" https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:141: Log.d("JAVA VideoCaptuer", "allocate: matched width=" + use TAG https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:214: try { why we need a try for a if statement? https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:237: try { same here https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:317: long nativeCapture, int rotation, indent by 8 spaces https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:23: static void OnFrameAvailable(JNIEnv* env, jobject obj, if you add the namespace in java, this should have a namespace https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.h (right): https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:21: virtual ~VideoCaptureDeviceAndroid(); do we need virtual? anyone inherits this? https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:34: virtual void OnFrameAvailable(uint8* buffer, int length, base::Time timestamp, do we need virtual?
some nits. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:58: private static final String TAG = "JAVA VideoCapture"; Do you want to make it shorter? https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:287: mCameraOrientation); Do you want to print out for every callback? It's good for debug but might be too much for product.
PTAL. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:30: public class VideoCapture implements PreviewCallback, OnFrameAvailableListener { On 2013/01/24 18:41:34, qinmin wrote: > add @JNINamespace("xxxx"), so the calls to native will have namespace appended Done. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:42: private boolean mIsRunning=false; On 2013/01/24 18:41:34, qinmin wrote: > spaces before and after "=" Done. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:58: private static final String TAG = "JAVA VideoCapture"; On 2013/01/24 19:01:59, leozwang2 wrote: > Do you want to make it shorter? Any suggestions on the tag name? My sense is the message will be in log. so it would be better to make log message clear. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:141: Log.d("JAVA VideoCaptuer", "allocate: matched width=" + On 2013/01/24 18:41:34, qinmin wrote: > use TAG Done. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:214: try { On 2013/01/24 18:41:34, qinmin wrote: > why we need a try for a if statement? findbugs_diff.py will complain if I don't use try...finally. This is also suggested by http://developer.android.com/reference/java/util/concurrent/locks/ReentrantLo.... https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:237: try { On 2013/01/24 18:41:34, qinmin wrote: > same here Same reply as above. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:287: mCameraOrientation); On 2013/01/24 19:01:59, leozwang2 wrote: > Do you want to print out for every callback? It's good for debug but might be > too much for product. This message will be logged only when device orientation is changed, not every frame delivery. https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:317: long nativeCapture, int rotation, On 2013/01/24 18:41:34, qinmin wrote: > indent by 8 spaces Done. https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:23: static void OnFrameAvailable(JNIEnv* env, jobject obj, On 2013/01/24 18:41:34, qinmin wrote: > if you add the namespace in java, this should have a namespace Done. https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.h (right): https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:21: virtual ~VideoCaptureDeviceAndroid(); On 2013/01/24 18:41:34, qinmin wrote: > do we need virtual? anyone inherits this? To make clang happy: [chromium-style] Overriding method must have "virtual" keyword. https://codereview.chromium.org/11860002/diff/38012/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:34: virtual void OnFrameAvailable(uint8* buffer, int length, base::Time timestamp, On 2013/01/24 18:41:34, qinmin wrote: > do we need virtual? Removed.
https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/38012/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:58: private static final String TAG = "JAVA VideoCapture"; A typical tag is <= 8 chars long, but it's not mandatory.
lgtm https://codereview.chromium.org/11860002/diff/41018/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/41018/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:273: current_settings_.width = remove 1 whitespace
adding owners for stamp: @fischman for media. @yfriedman for android. https://codereview.chromium.org/11860002/diff/41018/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/41018/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:273: current_settings_.width = On 2013/01/27 05:36:32, qinmin wrote: > remove 1 whitespace Done.
https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:5: package org.chromium.media; Didn't really review this once I realized I think you want to change the approach per comment at media/video/capture/android/video_capture_device_android.cc:31 https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:13: import android.opengl.GLES11Ext; Unused; please audit all imports for necessity. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:26: import javax.microedition.khronos.opengles.GL10; Any reason you're using this at all instead of the GLES20 above? https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:60: private static final String TAG = "JAVA VideoCapture"; s/JAVA // - using spaces in tags makes logcat incapable of filtering on them, and JAVA doesn't seem to be common in clank code. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:78: @CalledByNative doco return values? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:28: memset(buffer, 128, y_size / 2); What about the stride!=width case? Also this is obviously very I420/YV12-specific; throw that into the name of the function? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:31: void RotatePlaneByPixels(uint8* src, uint8* dest, int width, int height, I'm confused by your choice here; why ask Camera to give you a raw byte[] and do the rotation yourself, instead of having Camera render to a SurfaceTexture, doing the rotation internally (and hopefully more efficiently) using setDisplayOrientation, and then registering for frame-available notifications through SurfaceTexture.setOnFrameAvailableListener() ? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:134: CHECK(env); ACT() already DCHECKs. Why CHECK here? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:150: // Signature of getCameraInfo: "(ILandroid/hardware/Camera$CameraInfo;)V" Is this adding value? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:193: << name.unique_id.c_str() .c_str() unnecessary here and above https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:202: if (!self || !self->Init()) !self is unnecessary https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:203: return NULL; Init() failure leaks here. A better idiom is: scoped_ptr<VideoCaptureDeviceAndroid> ret(new VideoCaptureDeviceAndroid(device_name)); if (ret->Init()) return ret.release(); return NULL; https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:228: if (device_name_.device_name.find("camera") == std::string::npos) why do you care about this? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:231: base::StringToInt(device_name_.unique_id, &id); you should always test the return code of string-parsing routines like this. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:251: base::AutoLock lock(lock_); IWBN for the decl of lock_ in the .h to be accompanied by some comments explaining what needs to be locked and why. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:272: current_settings_.color = VideoCaptureCapability::kYV12; please initialize struct fields in declaration order to make it easier to verify all are present. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:280: DCHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); Why insist on even dimensions (but not powers-of-two)? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:280: DCHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); What about the other fields of VCC? Shouldn't they be sentinel- or zero-initialized? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:291: int y_size = current_settings_.width * current_settings_.height; Again, what about stride? See, for example, the equations at http://developer.android.com/reference/android/hardware/Camera.Parameters.htm... https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:302: return; why isn't this a DCHECK/SetErrorState()? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:316: state_ = kCapturing; having two critical sections means two threads can arrive at Start() and each call the java method. But VCD's commentary says the public API is called only from a single thread. So actually I think the locks in this method are unnecessary. I hope the commentary requested above for lock_ will clarify things. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:326: state_ = kAllocated; This makes Stop() silently clear an error state, which seems like a bug (what if Allocate failed?) https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:347: base::AutoUnlock unlock(lock_); There is a race here if two threads can call DeAllocate() (same as l.316 comment). https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:351: if (state_ == kAllocated) How can this fail to be true, other than the Error-clearing behavior of Stop() above which I think is a bug? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:351: if (state_ == kAllocated) Generally I think most of your == tests should be changed to != for early returns. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:403: src = rotation_buffer_.get(); I found this confusing; why not just use rotation_buffer_.get() in the next line, or else s/src/dest/ in this line and the next? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:411: state_ = kError; This is racy; probably you want to rename SES to SetErrorState_Locked and ensure lock_ is always held when this is called. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.h (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:23: static bool RegisterVideoCaptureDevice(JNIEnv* env); drop VideoCaptureDevice from the name, given it's in the class name? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:27: EventHandler* observer) OVERRIDE; this wrapping is meh - either all params should be indented to the opening paren (like |width| is here) or all should be +4 indented (like |observer| is here), but not mixed. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:33: bool Init(); It is unfortunate that you end up with a public, undocumented, method just b/c VCD::Create() is not a static member of this class. IMO it would be better to make the ctor private and add a static Create() method to this class that VCD::Create() could call, and inline the contents of Init into Create. WDYT? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:34: void OnFrameAvailable(JNIEnv* env, jobject obj, Doco this as implementing org.chromium.media.VideoCapture.nativeOnFrameAvailable() ? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:38: jobject java_capture_object() { return j_capture_.obj(); } What calls this? I think you can drop it, or else I'm missing something about this CL. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/video... File media/video/capture/video_capture_device_unittest.cc (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/video... media/video/capture/video_capture_device_unittest.cc:3: // found in the LICENSE file. Is there a layouttest or some other JS-driven test that can be enabled now? https://codereview.chromium.org/11860002/diff/50001/media/video/capture/video... media/video/capture/video_capture_device_unittest.cc:36: // TODO(wjia): enable those tests on Android. Do you want to enumerate here the reasons why each is disabled (as is done above) or link to a crbug where the reasons are listed?
https://codereview.chromium.org/11860002/diff/50001/content/shell/android/jav... File content/shell/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/11860002/diff/50001/content/shell/android/jav... content/shell/android/java/AndroidManifest.xml:67: <uses-permission android:name="android.permission.CAMERA" /> You need to add this to chromium_testshell too (chrome/android/testshell/java/AndroidManifest.xml) https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:66: Log.d(TAG, "createVideoCapture: " + id); There's too much spurious logging in this class. Please remove. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:268: catch (Exception ex) { Nit: catch/finally (i.e. throughout this file) go on previous line. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:290: boolean flip_vert = false; rename flipVertical https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:291: boolean flip_horiz = false; flipHorizontal https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:320: boolean flip_vert, boolean flip_horiz); flipVertical flipHorizontal https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:136: std::string camera_string("android/hardware/Camera"); Can you use the jni generator to generate bindings first. You can see the media_player_jni_headers target in media/media.gyp https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:211: DCHECK(g_VideoCapture_clazz); No need to DCHECK. just do: return Register.. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.h (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:38: jobject java_capture_object() { return j_capture_.obj(); } This is unused (and likely shouldn't be exposed)
PTAL. https://codereview.chromium.org/11860002/diff/50001/content/shell/android/jav... File content/shell/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/11860002/diff/50001/content/shell/android/jav... content/shell/android/java/AndroidManifest.xml:67: <uses-permission android:name="android.permission.CAMERA" /> On 2013/01/28 23:58:51, Yaron wrote: > You need to add this to chromium_testshell too > (chrome/android/testshell/java/AndroidManifest.xml) Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:5: package org.chromium.media; On 2013/01/28 23:55:47, Ami Fischman wrote: > Didn't really review this once I realized I think you want to change the > approach per comment at > media/video/capture/android/video_capture_device_android.cc:31 See reply at video_capture_device_android.cc:31. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:13: import android.opengl.GLES11Ext; On 2013/01/28 23:55:47, Ami Fischman wrote: > Unused; please audit all imports for necessity. Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:26: import javax.microedition.khronos.opengles.GL10; On 2013/01/28 23:55:47, Ami Fischman wrote: > Any reason you're using this at all instead of the GLES20 above? Removed. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:60: private static final String TAG = "JAVA VideoCapture"; On 2013/01/28 23:55:47, Ami Fischman wrote: > s/JAVA // - using spaces in tags makes logcat incapable of filtering on them, > and JAVA doesn't seem to be common in clank code. Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:66: Log.d(TAG, "createVideoCapture: " + id); On 2013/01/28 23:58:51, Yaron wrote: > There's too much spurious logging in this class. Please remove. Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:78: @CalledByNative On 2013/01/28 23:55:47, Ami Fischman wrote: > doco return values? Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:268: catch (Exception ex) { On 2013/01/28 23:58:51, Yaron wrote: > Nit: catch/finally (i.e. throughout this file) go on previous line. Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:290: boolean flip_vert = false; On 2013/01/28 23:58:51, Yaron wrote: > rename flipVertical Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:291: boolean flip_horiz = false; On 2013/01/28 23:58:51, Yaron wrote: > flipHorizontal Done. https://codereview.chromium.org/11860002/diff/50001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:320: boolean flip_vert, boolean flip_horiz); On 2013/01/28 23:58:51, Yaron wrote: > flipVertical flipHorizontal Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:28: memset(buffer, 128, y_size / 2); On 2013/01/28 23:55:47, Ami Fischman wrote: > What about the stride!=width case? > Also this is obviously very I420/YV12-specific; throw that into the name of the > function? We need support width!=stride case, all the way in all VideoCapture classses. I added a TODO here. Will follow up a patch soon. Added I420 as part of function name. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:31: void RotatePlaneByPixels(uint8* src, uint8* dest, int width, int height, On 2013/01/28 23:55:47, Ami Fischman wrote: > I'm confused by your choice here; why ask Camera to give you a raw byte[] and do > the rotation yourself, instead of having Camera render to a SurfaceTexture, > doing the rotation internally (and hopefully more efficiently) using > setDisplayOrientation, and then registering for frame-available notifications > through SurfaceTexture.setOnFrameAvailableListener() ? There is a TODO in VideoCapture.java to investigate whether reading from texture could give better performance and frame rate. My take is that there is only one SurfaceTexture for the camera. When its OnFrameAvailable() is called, it will prevent camera from generating a new frame to avoid pixel data overwriting. In case OnFrameAvailable() takes a bit longer time, camera will miss one frame. However, when using PreviewCallbackWithBuffer and multiple callback buffers (here is 3), we can avoid this. Then it will improve capturing frame rate. I agree it would be more efficient to use SurfaceTexture directly since camera doesn't have to copy data into callback buffer. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:134: CHECK(env); On 2013/01/28 23:55:47, Ami Fischman wrote: > ACT() already DCHECKs. Why CHECK here? I think caller wouldn't assume any implementation details of called functions. In release mode, AttachCurrentThread can still return a NULL env since it has DCHECK, not CHECk. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:136: std::string camera_string("android/hardware/Camera"); On 2013/01/28 23:58:51, Yaron wrote: > Can you use the jni generator to generate bindings first. You can see the > media_player_jni_headers target in media/media.gyp Did it for Camera. But jar_file_jni_generator.gypi doesn't support system inner classes. Add a TODO for Camera$CameraInfo. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:150: // Signature of getCameraInfo: "(ILandroid/hardware/Camera$CameraInfo;)V" On 2013/01/28 23:55:47, Ami Fischman wrote: > Is this adding value? Removed. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:193: << name.unique_id.c_str() On 2013/01/28 23:55:47, Ami Fischman wrote: > .c_str() unnecessary here and above Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:202: if (!self || !self->Init()) On 2013/01/28 23:55:47, Ami Fischman wrote: > !self is unnecessary Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:203: return NULL; On 2013/01/28 23:55:47, Ami Fischman wrote: > Init() failure leaks here. A better idiom is: > scoped_ptr<VideoCaptureDeviceAndroid> ret(new > VideoCaptureDeviceAndroid(device_name)); > if (ret->Init()) > return ret.release(); > return NULL; Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:211: DCHECK(g_VideoCapture_clazz); On 2013/01/28 23:58:51, Yaron wrote: > No need to DCHECK. just do: return Register.. Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:228: if (device_name_.device_name.find("camera") == std::string::npos) On 2013/01/28 23:55:47, Ami Fischman wrote: > why do you care about this? Removed. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:231: base::StringToInt(device_name_.unique_id, &id); On 2013/01/28 23:55:47, Ami Fischman wrote: > you should always test the return code of string-parsing routines like this. Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:251: base::AutoLock lock(lock_); On 2013/01/28 23:55:47, Ami Fischman wrote: > IWBN for the decl of lock_ in the .h to be accompanied by some comments > explaining what needs to be locked and why. Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:272: current_settings_.color = VideoCaptureCapability::kYV12; On 2013/01/28 23:55:47, Ami Fischman wrote: > please initialize struct fields in declaration order to make it easier to verify > all are present. Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:280: DCHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); On 2013/01/28 23:55:47, Ami Fischman wrote: > Why insist on even dimensions (but not powers-of-two)? Even number would ensure buffer size calculation is correct. The typical dimensions are 640x480, etc. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:280: DCHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); On 2013/01/28 23:55:47, Ami Fischman wrote: > What about the other fields of VCC? Shouldn't they be sentinel- or > zero-initialized? zero-init'ed current_settings_ in constructor. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:291: int y_size = current_settings_.width * current_settings_.height; On 2013/01/28 23:55:47, Ami Fischman wrote: > Again, what about stride? See, for example, the equations at > http://developer.android.com/reference/android/hardware/Camera.Parameters.htm...) Yes, I am aware of this. Need to do a refactoring for VCC to support stride. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:302: return; On 2013/01/28 23:55:47, Ami Fischman wrote: > why isn't this a DCHECK/SetErrorState()? changed to DCHECK. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:316: state_ = kCapturing; On 2013/01/28 23:55:47, Ami Fischman wrote: > having two critical sections means two threads can arrive at Start() and each > call the java method. > But VCD's commentary says the public API is called only from a single thread. > So actually I think the locks in this method are unnecessary. I hope the > commentary requested above for lock_ will clarify things. Comments for |lock_| have been added. |state_| can be accessed from a different thread (JAVA). https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:326: state_ = kAllocated; On 2013/01/28 23:55:47, Ami Fischman wrote: > This makes Stop() silently clear an error state, which seems like a bug (what if > Allocate failed?) Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:347: base::AutoUnlock unlock(lock_); On 2013/01/28 23:55:47, Ami Fischman wrote: > There is a race here if two threads can call DeAllocate() (same as l.316 > comment). DeAllocate() is called from only one thread. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:351: if (state_ == kAllocated) On 2013/01/28 23:55:47, Ami Fischman wrote: > How can this fail to be true, other than the Error-clearing behavior of Stop() > above which I think is a bug? In Stop(), error state is kept. Error could also happen during Java_VideoCapture_stopCapture() call. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:351: if (state_ == kAllocated) On 2013/01/28 23:55:47, Ami Fischman wrote: > Generally I think most of your == tests should be changed to != for early > returns. Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:403: src = rotation_buffer_.get(); On 2013/01/28 23:55:47, Ami Fischman wrote: > I found this confusing; why not just use rotation_buffer_.get() in the next > line, or else s/src/dest/ in this line and the next? I was kind of looking ahead. Since this rotation code will be moved into VideoCaptureController later, the call to OnIncomingCapturedFrame will be using src as parameter. Anyhow, change it to use rotation_buffer_.get() directly for now. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:411: state_ = kError; On 2013/01/28 23:55:47, Ami Fischman wrote: > This is racy; probably you want to rename SES to SetErrorState_Locked and ensure > lock_ is always held when this is called. Oops, missed this one. Accessing |state_| should acquire lock first. This function is called without lock acquired. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.h (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:23: static bool RegisterVideoCaptureDevice(JNIEnv* env); On 2013/01/28 23:55:47, Ami Fischman wrote: > drop VideoCaptureDevice from the name, given it's in the class name? I was following http://code.google.com/searchframe#OAMlx_jo-ck/src/media/base/android/media_j.... Hopefully this is ok. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:27: EventHandler* observer) OVERRIDE; On 2013/01/28 23:55:47, Ami Fischman wrote: > this wrapping is meh - either all params should be indented to the opening paren > (like |width| is here) or all should be +4 indented (like |observer| is here), > but not mixed. Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:33: bool Init(); On 2013/01/28 23:55:47, Ami Fischman wrote: > It is unfortunate that you end up with a public, undocumented, method just b/c > VCD::Create() is not a static member of this class. > IMO it would be better to make the ctor private and add a static Create() method > to this class that VCD::Create() could call, and inline the contents of Init > into Create. WDYT? Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:34: void OnFrameAvailable(JNIEnv* env, jobject obj, On 2013/01/28 23:55:47, Ami Fischman wrote: > Doco this as implementing > org.chromium.media.VideoCapture.nativeOnFrameAvailable() ? Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:38: jobject java_capture_object() { return j_capture_.obj(); } On 2013/01/28 23:55:47, Ami Fischman wrote: > What calls this? I think you can drop it, or else I'm missing something about > this CL. Removed. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:38: jobject java_capture_object() { return j_capture_.obj(); } On 2013/01/28 23:58:51, Yaron wrote: > This is unused (and likely shouldn't be exposed) Done. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/video... File media/video/capture/video_capture_device_unittest.cc (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/video... media/video/capture/video_capture_device_unittest.cc:3: // found in the LICENSE file. On 2013/01/28 23:55:47, Ami Fischman wrote: > Is there a layouttest or some other JS-driven test that can be enabled now? There is a layout test for media stream(platform/chromium/media/video-capture-preview.html). It's using a faked video capture module. More tests (at browser level) would be added later. https://codereview.chromium.org/11860002/diff/50001/media/video/capture/video... media/video/capture/video_capture_device_unittest.cc:36: // TODO(wjia): enable those tests on Android. On 2013/01/28 23:55:47, Ami Fischman wrote: > Do you want to enumerate here the reasons why each is disabled (as is done > above) or link to a crbug where the reasons are listed? Added comments on the reason why these tests can't pass. They share the same root cause.
https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:31: void RotatePlaneByPixels(uint8* src, uint8* dest, int width, int height, On 2013/01/30 18:25:47, wjia wrote: > On 2013/01/28 23:55:47, Ami Fischman wrote: > > I'm confused by your choice here; why ask Camera to give you a raw byte[] and > do > > the rotation yourself, instead of having Camera render to a SurfaceTexture, > > doing the rotation internally (and hopefully more efficiently) using > > setDisplayOrientation, and then registering for frame-available notifications > > through SurfaceTexture.setOnFrameAvailableListener() ? > > There is a TODO in VideoCapture.java to investigate whether reading from texture > could give better performance and frame rate. My take is that there is only one > SurfaceTexture for the camera. When its OnFrameAvailable() is called, it will > prevent camera from generating a new frame to avoid pixel data overwriting. In > case OnFrameAvailable() takes a bit longer time, camera will miss one frame. > > However, when using PreviewCallbackWithBuffer and multiple callback buffers > (here is 3), we can avoid this. Then it will improve capturing frame rate. This explanation doesn't make sense to me. The code as-is does far more than 3x the amount of work a memcpy would do, so even blocking OFA() for a single full copy would result in better framerate than what you're doing now. Even *that* copy should be unnecessary, since eventually the frame will end up being composited back out on a texture. Ideally, VCD::Observer could be taught about texture-backed frames and avoid the CSC in VideoCaptureController::OnIncomingCapturedFrame() entirely, but that's obviously a much bigger chunk of work. Opened https://code.google.com/p/chromium/issues/detail?id=173158 for that. https://codereview.chromium.org/11860002/diff/65003/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/65003/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:72: // Returns 0 on success, -1 otherwise. Return boolean instead, true on success? https://codereview.chromium.org/11860002/diff/65003/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/11860002/diff/65003/media/media.gyp#newcode21 media/media.gyp:21: }], I wish you had separated addressing the code review comments and rebasing into separate rietveld patchsets (in that order) because doing so makes it much easier to skim over the (usually irrelevant) rebase inter-patchset diffs. Please do that in the future. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:25: // TODO(wjia): add stride as part of buffer parameter. This is not enough; IMO you need to enforce the assumptions your code more clearly. For example, Allocate() should make sure the equations from the javadocs guarantee stride==queryWidth() and planes have no padding between them (in other words that width*height*3/2 is in fact the exact length of buffer here, always). If this assumption fails to hold then Allocate() should error out. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:136: CHECK(env); Chromium code eschews NULL CHECKs like this that only prevent the next use of the variable from segv'ing. (see chromium-dev archives) https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:208: return ret; return RegisterNativesImpl(env) && JNI_Camera::RegisterNativesImpl(env); ? https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:230: return false; Can never be true. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:267: current_settings_.color = VideoCaptureCapability::kYV12; This is still out of order (sort fields in declaration order). (did you get confused by the fact that the *enum type* is declared at the top of VCC, but the field of that type is not the first field?) https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:275: DCHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); The odd-size business can't be DCHECKs because other code *assumes* they are true. This needs to be either CHECK or SetErrorState on failure. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:295: base::AutoLock lock(lock_); This is now a serialization point used only for a DCHECK, which looks weird, but fine. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:410: observer_->OnError(); your .h comment promises to protect observer_ with lock_, but here it's accessed unlocked. Hopefully not a problem once you drop locking completely :) https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.h (right): https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:35: jbyteArray data, jint length, nit: indent here is wrong (mixing styles again). Please make style changes in whole CLs, not just where pointed out. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:51: // accessed from different threads. This is less useful than it could be. What threads touch this class? I believe only the Browser main thread and the Java thread touch this class, and the Java thread does so only by calling OnFrameAvailable(). If that's true, then you can significantly simplify the implementation by replacing the locking that happens everywhere with a simple trampoline for OFA() to post itself back to the UI thread. If it's not true, then I think you need a class-level header explaining which threads touch this class and how (e.g. it's not clear why it's ok to acquire/relinquish/re-acquire the lock in Start(), and expect that two threads won't have both called the Java start method). https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:54: VideoCaptureDevice::EventHandler* observer_; add a blank line after this to separate the lock_-protected members from the rest.
PTAL (patch set 15). https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/50001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:31: void RotatePlaneByPixels(uint8* src, uint8* dest, int width, int height, On 2013/01/30 19:46:23, Ami Fischman wrote: > On 2013/01/30 18:25:47, wjia wrote: > > On 2013/01/28 23:55:47, Ami Fischman wrote: > > > I'm confused by your choice here; why ask Camera to give you a raw byte[] > and > > do > > > the rotation yourself, instead of having Camera render to a SurfaceTexture, > > > doing the rotation internally (and hopefully more efficiently) using > > > setDisplayOrientation, and then registering for frame-available > notifications > > > through SurfaceTexture.setOnFrameAvailableListener() ? > > > > There is a TODO in VideoCapture.java to investigate whether reading from > texture > > could give better performance and frame rate. My take is that there is only > one > > SurfaceTexture for the camera. When its OnFrameAvailable() is called, it will > > prevent camera from generating a new frame to avoid pixel data overwriting. In > > case OnFrameAvailable() takes a bit longer time, camera will miss one frame. > > > > However, when using PreviewCallbackWithBuffer and multiple callback buffers > > (here is 3), we can avoid this. Then it will improve capturing frame rate. > > This explanation doesn't make sense to me. > The code as-is does far more than 3x the amount of work a memcpy would do, so > even blocking OFA() for a single full copy would result in better framerate than > what you're doing now. > > Even *that* copy should be unnecessary, since eventually the frame will end up > being composited back out on a texture. Ideally, VCD::Observer could be taught > about texture-backed frames and avoid the CSC in > VideoCaptureController::OnIncomingCapturedFrame() entirely, but that's obviously > a much bigger chunk of work. Opened > https://code.google.com/p/chromium/issues/detail?id=173158 for that. As we discussed offline, CallbackBuffer would be used as intermittent solution. https://codereview.chromium.org/11860002/diff/65003/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/65003/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:72: // Returns 0 on success, -1 otherwise. On 2013/01/30 19:46:23, Ami Fischman wrote: > Return boolean instead, true on success? Done. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:25: // TODO(wjia): add stride as part of buffer parameter. On 2013/01/30 19:46:23, Ami Fischman wrote: > This is not enough; IMO you need to enforce the assumptions your code more > clearly. > For example, Allocate() should make sure the equations from the javadocs > guarantee stride==queryWidth() and planes have no padding between them (in other > words that width*height*3/2 is in fact the exact length of buffer here, always). > If this assumption fails to hold then Allocate() should error out. Since Android YV12 always does 16 byte alignment for both luma and chroma, I added some check in allocate() in JAVA so that it will fail if it can't find a resolution with multiple of 32 as width. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:136: CHECK(env); On 2013/01/30 19:46:23, Ami Fischman wrote: > Chromium code eschews NULL CHECKs like this that only prevent the next use of > the variable from segv'ing. (see chromium-dev archives) removed all CHECK(env). https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:208: return ret; On 2013/01/30 19:46:23, Ami Fischman wrote: > return RegisterNativesImpl(env) && JNI_Camera::RegisterNativesImpl(env); > ? Done. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:230: return false; On 2013/01/30 19:46:23, Ami Fischman wrote: > Can never be true. Done. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:267: current_settings_.color = VideoCaptureCapability::kYV12; On 2013/01/30 19:46:23, Ami Fischman wrote: > This is still out of order (sort fields in declaration order). > > (did you get confused by the fact that the *enum type* is declared at the top of > VCC, but the field of that type is not the first field?) Done. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:275: DCHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); On 2013/01/30 19:46:23, Ami Fischman wrote: > The odd-size business can't be DCHECKs because other code *assumes* they are > true. This needs to be either CHECK or SetErrorState on failure. Done. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:410: observer_->OnError(); On 2013/01/30 19:46:23, Ami Fischman wrote: > your .h comment promises to protect observer_ with lock_, but here it's accessed > unlocked. Hopefully not a problem once you drop locking completely :) hrmm, I thought about the lock again. Actually, the |state_| and |oberver| will be modified only on calling thread from VideoCaptureManager. SetErrorState is also called on that thread. Only OnFrameAvailable is called from a different thread (i.e., UI thread or JAVA thread), but it doesn't modify both members. So there is no need to acquire lock here. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.h (right): https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:35: jbyteArray data, jint length, On 2013/01/30 19:46:23, Ami Fischman wrote: > nit: indent here is wrong (mixing styles again). Please make style changes in > whole CLs, not just where pointed out. Done. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:51: // accessed from different threads. On 2013/01/30 19:46:23, Ami Fischman wrote: > This is less useful than it could be. What threads touch this class? I > believe only the Browser main thread and the Java thread touch this class, and > the Java thread does so only by calling OnFrameAvailable(). > > If that's true, then you can significantly simplify the implementation by > replacing the locking that happens everywhere with a simple trampoline for OFA() > to post itself back to the UI thread. > > If it's not true, then I think you need a class-level header explaining which > threads touch this class and how (e.g. it's not clear why it's ok to > acquire/relinquish/re-acquire the lock in Start(), and expect that two threads > won't have both called the Java start method). Added class-level explanation. https://codereview.chromium.org/11860002/diff/65003/media/video/capture/andro... media/video/capture/android/video_capture_device_android.h:54: VideoCaptureDevice::EventHandler* observer_; On 2013/01/30 19:46:23, Ami Fischman wrote: > add a blank line after this to separate the lock_-protected members from the > rest. Done.
LGTM % nits and locking business. https://codereview.chromium.org/11860002/diff/78001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/78001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:72: // Returns 0 on success, -1 otherwise. Comment is a lie https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:276: CHECK(current_settings_.width > 0 && !(current_settings_.width % 2)); It's strange to assert %2==0 when you really depend on %32==0 for packing guarantees. https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:411: state_ = kError; Your logic that these don't need to be locked because they are only written on one thread is incorrect; the read-only thread is not guaranteed to ever see the updated values, much less see them updated in a coherent fashion. Was there a reason that it doesn't work to trampoline OFA to the VCM thread and avoid the need for locking entirely?
Thanks! https://codereview.chromium.org/11860002/diff/78001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/78001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:72: // Returns 0 on success, -1 otherwise. On 2013/02/06 04:10:46, Ami Fischman wrote: > Comment is a lie Done. https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:276: CHECK(current_settings_.width > 0 && !(current_settings_.width % 2)); On 2013/02/06 04:10:46, Ami Fischman wrote: > It's strange to assert %2==0 when you really depend on %32==0 for packing > guarantees. Done. https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:411: state_ = kError; On 2013/02/06 04:10:46, Ami Fischman wrote: > Your logic that these don't need to be locked because they are only written on > one thread is incorrect; the read-only thread is not guaranteed to ever see the > updated values, much less see them updated in a coherent fashion. > > Was there a reason that it doesn't work to trampoline OFA to the VCM thread and > avoid the need for locking entirely? I guess I was not clear about using lock. Just to clarify, the lock is definitely needed. It's needed only when |state_| and |observer_| are accessed on JAVA thread, and when they are modified on VideoCaptureManager's thread. When VCM thread tries to read the value of those 2 members, it doesn't need to acquire the lock since no other thread would modify those 2 members. I have tried to trampoline OFA to VCM thread before. But it's much more complicated than just using lock for the following reasons: 1. Have to do buffer = env->GetByteArrayElements(data, NULL); on JAVA thread. 2. After VCM thread is done data delivery, it has to notify JAVA thread to free the buffer and send |data| back to camera. 3. In typical use cases, when OFA is called, only JAVA thread is trying to acquire lock because Allocate/Start/Stop/DeAllocate are called only once each for a session. It would be faster than thread hopping.
https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:411: state_ = kError; On 2013/02/06 17:33:35, wjia wrote: > On 2013/02/06 04:10:46, Ami Fischman wrote: > > Your logic that these don't need to be locked because they are only written on > > one thread is incorrect; the read-only thread is not guaranteed to ever see > the > > updated values, much less see them updated in a coherent fashion. > > > > Was there a reason that it doesn't work to trampoline OFA to the VCM thread > and > > avoid the need for locking entirely? > > I guess I was not clear about using lock. Just to clarify, the lock is > definitely needed. It's needed only when |state_| and |observer_| are accessed > on JAVA thread, and when they are modified on VideoCaptureManager's thread. When > VCM thread tries to read the value of those 2 members, it doesn't need to > acquire the lock since no other thread would modify those 2 members. So how can you SetErrorState from the Java thread without acquiring a lock here?? https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:276: CHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); note you removed the width > 0 check. Not sure if that was intentional. (not sure what purpose either width/height>0 checks served)
https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/78001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:411: state_ = kError; On 2013/02/06 17:39:14, Ami Fischman wrote: > On 2013/02/06 17:33:35, wjia wrote: > > On 2013/02/06 04:10:46, Ami Fischman wrote: > > > Your logic that these don't need to be locked because they are only written > on > > > one thread is incorrect; the read-only thread is not guaranteed to ever see > > the > > > updated values, much less see them updated in a coherent fashion. > > > > > > Was there a reason that it doesn't work to trampoline OFA to the VCM thread > > and > > > avoid the need for locking entirely? > > > > I guess I was not clear about using lock. Just to clarify, the lock is > > definitely needed. It's needed only when |state_| and |observer_| are accessed > > on JAVA thread, and when they are modified on VideoCaptureManager's thread. > When > > VCM thread tries to read the value of those 2 members, it doesn't need to > > acquire the lock since no other thread would modify those 2 members. > > So how can you SetErrorState from the Java thread without acquiring a lock > here?? SetErrorState is only called on VCM thread. I accidentally dropped the lock protection for modifying |state_|. Added it back. https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:276: CHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); On 2013/02/06 17:39:14, Ami Fischman wrote: > note you removed the width > 0 check. Not sure if that was intentional. (not > sure what purpose either width/height>0 checks served) Since the width is guaranteed to be multiple of 32, only height is needed to be checked here. The height has to be even number since the calculation of chroma plane size assumes that.
https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:276: CHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); On 2013/02/06 17:52:29, wjia wrote: > On 2013/02/06 17:39:14, Ami Fischman wrote: > > note you removed the width > 0 check. Not sure if that was intentional. (not > > sure what purpose either width/height>0 checks served) > > Since the width is guaranteed to be multiple of 32, only height is needed to be > checked here. The height has to be even number since the calculation of chroma > plane size assumes that. What about width or height of 0? (why do you check that height>0 but not force width>0?)
https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/83001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:276: CHECK(current_settings_.height > 0 && !(current_settings_.height % 2)); On 2013/02/06 18:46:45, Ami Fischman wrote: > On 2013/02/06 17:52:29, wjia wrote: > > On 2013/02/06 17:39:14, Ami Fischman wrote: > > > note you removed the width > 0 check. Not sure if that was intentional. > (not > > > sure what purpose either width/height>0 checks served) > > > > Since the width is guaranteed to be multiple of 32, only height is needed to > be > > checked here. The height has to be even number since the calculation of chroma > > plane size assumes that. > > What about width or height of 0? > (why do you check that height>0 but not force width>0?) There should be no problem about that since the queried width/height are resolution supported by the camera. I think it's better to just add back the previous check on width, since the restriction of multiple of 32 is checked in java code. This file shouldn't assume anything over there.
lgtm % comments https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:58: // Returns an instance of VideoCapture. Comment seems unnecessary https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:185: Log.e(TAG, "allocate: RuntimeException"); Why are you catching all RuntimeExceptions and not even outputting the exception in the log? This provides no information for debugging and is dangerous. https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:231: Log.d(TAG, "stopCapture: camera is null"); Should this be Log.e (match with startCapture) https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:167: CheckException(env); This is unnecessary (handled by the binding)
I'm worried about the rotation code in this CL (which I hadn't reviewed until now b/c originally I was expecting it would drop out of the CL, and then b/c I assumed it was reviewed by others). https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:38: int rotation, clockwise? Counter? Should have a comment. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:85: src += width * offset; So in this case width <= height, so offset is <= 0, so you're considering moving src to *before* the beginning of the plane? I'm confused. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:86: num_rows = num_cols = width; I don't get this; doesn't this result in only rotating a square subrect of |src| into |dest|? https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:92: dest_row_step = 1; Set this like dest_col_step at l.90? https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:97: dest += (width > height ? offset : width * offset); I'm not 100% sure I understand how offset is being used, but I *think* that if you pre-multiply it by width at l.87 then the two cases in the ?: in each of the 4 occurrences in l.94-107 have their "else" clause look a lot more like the "then" clause. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:108: } else NOTREACHED? https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:122: } This is a crazy function to have no test for. Especially given how easy it would be to write that test; fill a small (assymetric) plane with uint8's 0..23 and test the 16 rotations/flips by typing out the matrices as literals. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:386: ResetBufferI420(rotation_buffer_.get(), width, height); Is there no need to call observer_->OnFrameInfo() here in case width/height got swapped as a result of the rotation?
Please review video_capture_device_android.* and VideoCapture.java. Other files have not been changed. https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:58: // Returns an instance of VideoCapture. On 2013/02/06 19:25:51, Yaron wrote: > Comment seems unnecessary Done. https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:185: Log.e(TAG, "allocate: RuntimeException"); On 2013/02/06 19:25:51, Yaron wrote: > Why are you catching all RuntimeExceptions and not even outputting the exception > in the log? This provides no information for debugging and is dangerous. At one point, I was trying to catch just Exception, but somehow I got error complaining RuntimeException is not caught. But it seems ok to switch back to catch Exception now. Added exception message in the log. https://codereview.chromium.org/11860002/diff/74008/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:231: Log.d(TAG, "stopCapture: camera is null"); On 2013/02/06 19:25:51, Yaron wrote: > Should this be Log.e (match with startCapture) Done. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:38: int rotation, On 2013/02/06 20:13:42, Ami Fischman wrote: > clockwise? Counter? Should have a comment. Done. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:85: src += width * offset; On 2013/02/06 20:13:42, Ami Fischman wrote: > So in this case width <= height, so offset is <= 0, so you're considering moving > src to *before* the beginning of the plane? I'm confused. hrmm, the offset is calculated 2 lines behind. That's apparently a mistake. Thanks for catching that! https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:86: num_rows = num_cols = width; On 2013/02/06 20:13:42, Ami Fischman wrote: > I don't get this; doesn't this result in only rotating a square subrect of |src| > into |dest|? Yes, a square is the overlapped area of src and dest. The src has width x height. After rotated by 90 degree, it becomes heigh x width. But we still want to deliver width x height frame to renderer process. So we have to crop src either horizontally or vertically, then pad dest on the opposite. The reason for always delivering width x height frames is that we don't support multiple OnFrameInfo yet. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:92: dest_row_step = 1; On 2013/02/06 20:13:42, Ami Fischman wrote: > Set this like dest_col_step at l.90? This one is correct. I have added some comments about meaning of dest_row_step and dest_col_step. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:97: dest += (width > height ? offset : width * offset); On 2013/02/06 20:13:42, Ami Fischman wrote: > I'm not 100% sure I understand how offset is being used, but I *think* that if > you pre-multiply it by width at l.87 then the two cases in the ?: in each of the > 4 occurrences in l.94-107 have their "else" clause look a lot more like the > "then" clause. "offset" is the cropped/padded pixels on one line (actually half of them) when width != height. It's used to calculate the starting pointer of src and dest for pixel copying. The pixel copying always follows src pixel order, traverse row by row. Therefore, we need calculate dest_row_step and dest_col_step corresponding to row and column change in src pointer, respectively. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:108: } On 2013/02/06 20:13:42, Ami Fischman wrote: > else NOTREACHED? l99 is the "else". https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:122: } On 2013/02/06 20:13:42, Ami Fischman wrote: > This is a crazy function to have no test for. > Especially given how easy it would be to write that test; fill a small > (assymetric) plane with uint8's 0..23 and test the 16 rotations/flips by typing > out the matrices as literals. Agree that we need a test for this function. I have manually tested some cases (when src's width > height). I will follow up a patch to add the test. Need to move it to some util file. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:167: CheckException(env); On 2013/02/06 19:25:51, Yaron wrote: > This is unnecessary (handled by the binding) Done. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:386: ResetBufferI420(rotation_buffer_.get(), width, height); On 2013/02/06 20:13:42, Ami Fischman wrote: > Is there no need to call observer_->OnFrameInfo() here in case width/height got > swapped as a result of the rotation? No, we don't support dimension change in one capture session yet. It requires re-allocate shared memory buffers. That's why we can only copy a square area when width != height. To think about it further, even if we support multiple OnFrameInfo, when renderer process receives the new frames with different size, it has to blit the frame into the same display size. In this case, in order to keep aspect ratio, renderer process has to chop/pad anyway.
https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:386: ResetBufferI420(rotation_buffer_.get(), width, height); On 2013/02/06 23:20:39, wjia wrote: > On 2013/02/06 20:13:42, Ami Fischman wrote: > > Is there no need to call observer_->OnFrameInfo() here in case width/height > got > > swapped as a result of the rotation? > > No, we don't support dimension change in one capture session yet. It requires > re-allocate shared memory buffers. That's why we can only copy a square area > when width != height. > > To think about it further, even if we support multiple OnFrameInfo, when > renderer process receives the new frames with different size, it has to blit the > frame into the same display size. In this case, in order to keep aspect ratio, > renderer process has to chop/pad anyway. See above; I think we need to honor rotation from the beginning to have a reasonable UX when the device starts rotated/flipped. https://codereview.chromium.org/11860002/diff/76011/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/76011/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:18: import java.lang.Exception; java.lang doesn't need imports https://codereview.chromium.org/11860002/diff/76011/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:259: Log.e(TAG, "deallocate: failed to deallocate camera, " + ex.getMessage()); Is there some reason you use the idiom: Log.e(TAG, "msg" + ex.getMessage()); instead of the more concise and more informative Log.e(TAG, "msg", ex); ? https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:85: int offset = (width - height) / 2; the initialization value seems to me to belong in the then clause below (l.87). https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:92: num_rows = num_cols = width; On 2013/02/06 23:20:39, wjia wrote: > On 2013/02/06 20:13:42, Ami Fischman wrote: > > I don't get this; doesn't this result in only rotating a square subrect of > |src| > > into |dest|? > > Yes, a square is the overlapped area of src and dest. The src has width x > height. After rotated by 90 degree, it becomes heigh x width. But we still want > to deliver width x height frame to renderer process. So we have to crop src > either horizontally or vertically, then pad dest on the opposite. > > The reason for always delivering width x height frames is that we don't support > multiple OnFrameInfo yet. If the camera starts out rotated 90deg then a single OnFrameInfo is enough, but you're still messing up the frame. What actually happens when you rotate the camera on android with this patch? Do you just capture square previews with a black (width-height) x height size? That doesn't seem shippable to me. https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:97: dest_row_step = 1; On 2013/02/06 23:20:39, wjia wrote: > On 2013/02/06 20:13:42, Ami Fischman wrote: > > Set this like dest_col_step at l.90? > > This one is correct. I have added some comments about meaning of dest_row_step > and dest_col_step. I think you misunderstood me. I was just suggesting you could set dest_row_step using a ?: expression like at l.95. https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:114: } On 2013/02/06 23:20:39, wjia wrote: > On 2013/02/06 20:13:42, Ami Fischman wrote: > > else NOTREACHED? > > l99 is the "else". You misunderstood me - I was saying that the outline of if (rotation == 0) { } else if (rotation == 90} } ... should have an else NOTREACHED clause instead of ... (which is empty now). https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:127: } On 2013/02/06 23:20:39, wjia wrote: > On 2013/02/06 20:13:42, Ami Fischman wrote: > > This is a crazy function to have no test for. > > Especially given how easy it would be to write that test; fill a small > > (assymetric) plane with uint8's 0..23 and test the 16 rotations/flips by > typing > > out the matrices as literals. > > Agree that we need a test for this function. I have manually tested some cases > (when src's width > height). I will follow up a patch to add the test. Need to > move it to some util file. Testing this should be super-simple. Please do it as part of this CL. (I'd be fine with an initial version of this CL that simply didn't handle rotation, and then rotation+test in a follow-up, but landing this function untested seems like a *very* bad idea).
only video_capture_device_android.cc and VideoCapture.java have changes. https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/74008/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:386: ResetBufferI420(rotation_buffer_.get(), width, height); On 2013/02/07 05:37:34, Ami Fischman wrote: > On 2013/02/06 23:20:39, wjia wrote: > > On 2013/02/06 20:13:42, Ami Fischman wrote: > > > Is there no need to call observer_->OnFrameInfo() here in case width/height > > got > > > swapped as a result of the rotation? > > > > No, we don't support dimension change in one capture session yet. It requires > > re-allocate shared memory buffers. That's why we can only copy a square area > > when width != height. > > > > To think about it further, even if we support multiple OnFrameInfo, when > > renderer process receives the new frames with different size, it has to blit > the > > frame into the same display size. In this case, in order to keep aspect ratio, > > renderer process has to chop/pad anyway. > > See above; I think we need to honor rotation from the beginning to have a > reasonable UX when the device starts rotated/flipped. To support resolution change on the fly, we need add quite some thing in the video capture frame work. There seems desire to do that. After resolution change is supported in video capture framework, we could do OnFrameInfo here. https://codereview.chromium.org/11860002/diff/76011/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/76011/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:18: import java.lang.Exception; On 2013/02/07 05:37:34, Ami Fischman wrote: > java.lang doesn't need imports Done. https://codereview.chromium.org/11860002/diff/76011/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:259: Log.e(TAG, "deallocate: failed to deallocate camera, " + ex.getMessage()); On 2013/02/07 05:37:34, Ami Fischman wrote: > Is there some reason you use the idiom: > Log.e(TAG, "msg" + ex.getMessage()); > instead of the more concise and more informative > Log.e(TAG, "msg", ex); > ? Done. https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:85: int offset = (width - height) / 2; On 2013/02/07 05:37:34, Ami Fischman wrote: > the initialization value seems to me to belong in the then clause below (l.87). Done. https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:92: num_rows = num_cols = width; On 2013/02/07 05:37:34, Ami Fischman wrote: > On 2013/02/06 23:20:39, wjia wrote: > > On 2013/02/06 20:13:42, Ami Fischman wrote: > > > I don't get this; doesn't this result in only rotating a square subrect of > > |src| > > > into |dest|? > > > > Yes, a square is the overlapped area of src and dest. The src has width x > > height. After rotated by 90 degree, it becomes heigh x width. But we still > want > > to deliver width x height frame to renderer process. So we have to crop src > > either horizontally or vertically, then pad dest on the opposite. > > > > The reason for always delivering width x height frames is that we don't > support > > multiple OnFrameInfo yet. > > If the camera starts out rotated 90deg then a single OnFrameInfo is enough, but > you're still messing up the frame. > > What actually happens when you rotate the camera on android with this patch? Do > you just capture square previews with a black (width-height) x height size? > That doesn't seem shippable to me. This comes from the restriction that current video capture framework doesn't support resolution change in a capture session. We will improve that. Then this rotation will include full image, instead of just the square portion. https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:97: dest_row_step = 1; On 2013/02/07 05:37:34, Ami Fischman wrote: > On 2013/02/06 23:20:39, wjia wrote: > > On 2013/02/06 20:13:42, Ami Fischman wrote: > > > Set this like dest_col_step at l.90? > > > > This one is correct. I have added some comments about meaning of dest_row_step > > and dest_col_step. > > I think you misunderstood me. > I was just suggesting you could set dest_row_step using a ?: expression like at > l.95. Changed to ?: expression. I thought since flip_horiz will be checked anyway, putting assignment of dest_row_step in each branch doesn't require second checking on flip_horiz. https://codereview.chromium.org/11860002/diff/76011/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:114: } On 2013/02/07 05:37:34, Ami Fischman wrote: > On 2013/02/06 23:20:39, wjia wrote: > > On 2013/02/06 20:13:42, Ami Fischman wrote: > > > else NOTREACHED? > > > > l99 is the "else". > > You misunderstood me - I was saying that the outline of > if (rotation == 0) { > } else if (rotation == 90} > } ... > should have an else NOTREACHED clause instead of ... (which is empty now). Done.
https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:178: } catch (Exception ex) { My previous question about asking about a catch-all exception though was that in general it should be avoided. If you have an actual bug in implementation, you won't get that information and instead you'll get a user report that somtimes video capture doesn't work, and if you're very lucky you'll get a crash report that includes the message. Either way, the user doesn't get the video and in one case you get information to fix it and in the other you don't. What exception are you trying to guard against that isn't indicative of a logic error in your code that should be fixed? Are you're worried about some sort of driver issue or exception in the opengl code that can happen on some devices?
https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:178: } catch (Exception ex) { On 2013/02/12 00:29:23, Yaron wrote: > My previous question about asking about a catch-all exception though was that in > general it should be avoided. If you have an actual bug in implementation, you > won't get that information and instead you'll get a user report that somtimes > video capture doesn't work, and if you're very lucky you'll get a crash report > that includes the message. Either way, the user doesn't get the video and in one > case you get information to fix it and in the other you don't. > > What exception are you trying to guard against that isn't indicative of a logic > error in your code that should be fixed? Are you're worried about some sort of > driver issue or exception in the opengl code that can happen on some devices? This catching exception will allow device code to notify its client that some error happens when trying to allocating resources. Then client can take corresponding action. In case any error/exception happens in low-lever code, we will just bail out.
https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:178: } catch (Exception ex) { On 2013/02/12 00:53:18, wjia wrote: > On 2013/02/12 00:29:23, Yaron wrote: > > My previous question about asking about a catch-all exception though was that > in > > general it should be avoided. If you have an actual bug in implementation, you > > won't get that information and instead you'll get a user report that somtimes > > video capture doesn't work, and if you're very lucky you'll get a crash report > > that includes the message. Either way, the user doesn't get the video and in > one > > case you get information to fix it and in the other you don't. > > > > What exception are you trying to guard against that isn't indicative of a > logic > > error in your code that should be fixed? Are you're worried about some sort of > > driver issue or exception in the opengl code that can happen on some devices? > > This catching exception will allow device code to notify its client that some > error happens when trying to allocating resources. Then client can take > corresponding action. In case any error/exception happens in low-lever code, we > will just bail out. Yes, but you didn't answer my question. Catching _all_ exceptions means you hide potential bugs in your code and the client has limited information to deal with it. See http://source.android.com/source/code-style.html#dont-catch-generic-exception for more details
https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/11860002/diff/83018/media/base/android/java/s... media/base/android/java/src/org/chromium/media/VideoCapture.java:178: } catch (Exception ex) { On 2013/02/12 00:59:49, Yaron wrote: > On 2013/02/12 00:53:18, wjia wrote: > > On 2013/02/12 00:29:23, Yaron wrote: > > > My previous question about asking about a catch-all exception though was > that > > in > > > general it should be avoided. If you have an actual bug in implementation, > you > > > won't get that information and instead you'll get a user report that > somtimes > > > video capture doesn't work, and if you're very lucky you'll get a crash > report > > > that includes the message. Either way, the user doesn't get the video and in > > one > > > case you get information to fix it and in the other you don't. > > > > > > What exception are you trying to guard against that isn't indicative of a > > logic > > > error in your code that should be fixed? Are you're worried about some sort > of > > > driver issue or exception in the opengl code that can happen on some > devices? > > > > This catching exception will allow device code to notify its client that some > > error happens when trying to allocating resources. Then client can take > > corresponding action. In case any error/exception happens in low-lever code, > we > > will just bail out. > > Yes, but you didn't answer my question. Catching _all_ exceptions means you hide > potential bugs in your code and the client has limited information to deal with > it. See > http://source.android.com/source/code-style.html#dont-catch-generic-exception > for more details Done.
lgtm |
