Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
135213005:
Add GetDeviceSupportedFormats to Android VideoCapture.java. (Closed)
|
Created:
6 years, 10 months ago by mcasas Modified:
6 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd GetDeviceSupportedFormats to Android VideoCapture.java.
The |formats| Java Array is sent to native where is pushed into
the C++ |formats| array. This is easier than to pass data types
to and from Java.
VideoCapture.CaptureCapability modified to match C++ equivalent
VideoCaptureFormat [1]. Added mPixelFormat to it. Internal logic
adapted to keep capture format in that class too. Some internal
allocate() logic is passed from iterator to Java foreach loop.
[1] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/capture/video_capture_types.h&sq=package:chromium&l=37&type=cs
BUG=309554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251857
Patch Set 1 : #
Total comments: 8
Patch Set 2 : bulach@ comments #
Total comments: 19
Patch Set 3 : wjia@ and bulach@ comments #Patch Set 4 : Addressed findbugs spotted issue. #
Total comments: 2
Patch Set 5 : wjia@s comments #
Total comments: 3
Messages
Total messages: 19 (0 generated)
wjia@, bulach@ PTAL.
Cc: tommi@.
thanks! nits and one potential issue below: https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:44: public int mPixelFormat; nit: given the constructor now, make all of these members final? https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:90: static void applyMinDimensions(CaptureFormat capability) { nit: perhaps s/capability/captureFormat/ ? https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:258: BuggyDeviceHack.applyMinDimensions(mCaptureFormat); hmm.. shouldn't 271 mCaptureFormat = move up here? looks like at this point, mCaptureFormat is null, no? https://codereview.chromium.org/135213005/diff/620001/media/video/capture/and... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/135213005/diff/620001/media/video/capture/and... media/video/capture/android/video_capture_device_android.cc:71: jobject format = env->GetObjectArrayElement(collected_formats.obj(), i); nit: base::android::ScopedJavaLocalRef and remove DeleteLocalRef from 89..
bulach@, wjia@, PTAL. https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:44: public int mPixelFormat; On 2014/02/12 12:48:20, bulach wrote: > nit: given the constructor now, make all of these members final? Done for two of them. |mWidth| and |mHeight| are modified in BuggyDeviceHack.applyMinDimensions(). https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:90: static void applyMinDimensions(CaptureFormat capability) { On 2014/02/12 12:48:20, bulach wrote: > nit: perhaps s/capability/captureFormat/ ? Done (format). https://codereview.chromium.org/135213005/diff/620001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:258: BuggyDeviceHack.applyMinDimensions(mCaptureFormat); On 2014/02/12 12:48:20, bulach wrote: > hmm.. shouldn't 271 mCaptureFormat = move up here? > looks like at this point, mCaptureFormat is null, no? Done. https://codereview.chromium.org/135213005/diff/620001/media/video/capture/and... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/135213005/diff/620001/media/video/capture/and... media/video/capture/android/video_capture_device_android.cc:71: jobject format = env->GetObjectArrayElement(collected_formats.obj(), i); On 2014/02/12 12:48:20, bulach wrote: > nit: base::android::ScopedJavaLocalRef and remove DeleteLocalRef from 89.. Done.
https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:162: List<Integer> pixelFormats = parameters.getSupportedPreviewFormats(); sanity check. see below. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:164: if (previewFormat == ImageFormat.YV12) add {} for "if" unless both condition and body fit on one line. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:167: pixelFormat = AndroidImageFormatList.ANDROID_IMAGEFORMAT_NV21; Any reason to expose NV21 format? We always pass I420 over IPC. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:171: List<int[]> listFpsRange = parameters.getSupportedPreviewFpsRange(); sanity check. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:180: } Are you sure all combined results are supported? Typically, FPS should be combined with dimension when OS provides supported capabilities. But Android is not doing that. It really depends on how Android separate FPS and dimension. If they take common FPS range of all dimensions to calculate FPS, then all combined results are supported. Otherwise, there is a problem here. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:216: List<int[]> listFpsRange = parameters.getSupportedPreviewFpsRange(); The sanity check shouldn't be removed. Refer to http://src.chromium.org/viewvc/chrome?view=revision&revision=219382.
lgtm % making wjia happy and some nits below... thanks! https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:159: int pixelFormat = AndroidImageFormatList.ANDROID_IMAGEFORMAT_UNKNOWN; nit: looks like this can be moved to 164, inside the loop, and then remove the else clause there? https://codereview.chromium.org/135213005/diff/700001/media/video/capture/and... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/135213005/diff/700001/media/video/capture/and... media/video/capture/android/video_capture_device_android.cc:60: JNIEnv* env = AttachCurrentThread(); nit: could move this after the StringToInt.. https://codereview.chromium.org/135213005/diff/700001/media/video/capture/and... media/video/capture/android/video_capture_device_android.cc:91: VideoCaptureFormat& last_format = formats->back(); nit: maybe clang would complain since this will become unused on a non-debug mode? wrapping with ifndef NDEBUG would be bad too, so here's a suggestion: VideoCaptureFormat format(gfx::Size(...), getFrameRate, ...); formats->push_back(format); DVLOG(1) << ... ; the compiler will hopefully optimize the local format away..
wjia@ PTAL. bulach@: Thanks! https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:159: int pixelFormat = AndroidImageFormatList.ANDROID_IMAGEFORMAT_UNKNOWN; On 2014/02/13 14:08:23, bulach wrote: > nit: looks like this can be moved to 164, inside the loop, and then remove the > else clause there? Done. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:162: List<Integer> pixelFormats = parameters.getSupportedPreviewFormats(); On 2014/02/12 23:13:20, wjia wrote: > sanity check. see below. Done. On the sanity checks, please note that the CaptureFormats array is a best-effort type of data, so we can admit having unknown pixel formats, 0 fps ranges and 0x0 dimensions. That will/should be ignored at the destination, i.e. is interpreted as the underlying hardware not giving correct information or being in an unstable state. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:164: if (previewFormat == ImageFormat.YV12) On 2014/02/12 23:13:20, wjia wrote: > add {} for "if" unless both condition and body fit on one line. Done. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:167: pixelFormat = AndroidImageFormatList.ANDROID_IMAGEFORMAT_NV21; On 2014/02/12 23:13:20, wjia wrote: > Any reason to expose NV21 format? We always pass I420 over IPC. CaptureFormats contains the camera supported data type(s, regardless of the IPC format. Reason for it is that (perhaps) in the future the device constraints resolution algorithm would (like to) include pixel format as another input. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:171: List<int[]> listFpsRange = parameters.getSupportedPreviewFpsRange(); On 2014/02/12 23:13:20, wjia wrote: > sanity check. Done. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:180: } On 2014/02/12 23:13:20, wjia wrote: > Are you sure all combined results are supported? > > Typically, FPS should be combined with dimension when OS provides supported > capabilities. But Android is not doing that. It really depends on how Android > separate FPS and dimension. If they take common FPS range of all dimensions to > calculate FPS, then all combined results are supported. Otherwise, there is a > problem here. Indeed. The TestingCam App allows to experiment with those and so far, all combinations of frame rate - resolution - pixel format seem to be supported. https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:216: List<int[]> listFpsRange = parameters.getSupportedPreviewFpsRange(); On 2014/02/12 23:13:20, wjia wrote: > The sanity check shouldn't be removed. Refer to > http://src.chromium.org/viewvc/chrome?view=revision&revision=219382. Done. Added some comment about why the sanity check. https://codereview.chromium.org/135213005/diff/700001/media/video/capture/and... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/135213005/diff/700001/media/video/capture/and... media/video/capture/android/video_capture_device_android.cc:60: JNIEnv* env = AttachCurrentThread(); On 2014/02/13 14:08:23, bulach wrote: > nit: could move this after the StringToInt.. Done. https://codereview.chromium.org/135213005/diff/700001/media/video/capture/and... media/video/capture/android/video_capture_device_android.cc:91: VideoCaptureFormat& last_format = formats->back(); On 2014/02/13 14:08:23, bulach wrote: > nit: maybe clang would complain since this will become unused on a non-debug > mode? wrapping with ifndef NDEBUG would be bad too, so here's a suggestion: > > VideoCaptureFormat format(gfx::Size(...), getFrameRate, ...); > formats->push_back(format); > DVLOG(1) << ... ; > > the compiler will hopefully optimize the local format away.. Done.
https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/700001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:167: pixelFormat = AndroidImageFormatList.ANDROID_IMAGEFORMAT_NV21; On 2014/02/14 11:28:39, mcasas wrote: > On 2014/02/12 23:13:20, wjia wrote: > > Any reason to expose NV21 format? We always pass I420 over IPC. > > CaptureFormats contains the camera supported data type(s, > regardless of the IPC format. Reason for it is that (perhaps) in > the future the device constraints resolution algorithm would > (like to) include pixel format as another input. Sounds like adding NV21 is for possible "future" design. Please remove it for now, and add it back when we need it. https://codereview.chromium.org/135213005/diff/910001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/910001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:165: if (pixelFormats == null) nit: add {} when if condition and body doesn't fit on one line. Please fix all cases.
wjia@ PTAL. https://codereview.chromium.org/135213005/diff/910001/media/base/android/java... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/910001/media/base/android/java... media/base/android/java/src/org/chromium/media/VideoCapture.java:165: if (pixelFormats == null) On 2014/02/14 18:33:00, wjia wrote: > nit: add {} when if condition and body doesn't fit on one line. Please fix all > cases. Done. Interesting, Java style [1] differs from C++ style [2] for these 1-liners. [1] https://source.android.com/source/code-style.html#use-standard-brace-style [2] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... https://codereview.chromium.org/135213005/diff/1010001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/1010001/media/base/android/jav... media/base/android/java/src/org/chromium/media/VideoCapture.java:177: continue; NV21 is explicitly skipped.
https://codereview.chromium.org/135213005/diff/1010001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/1010001/media/base/android/jav... media/base/android/java/src/org/chromium/media/VideoCapture.java:178: } I thought this over again. Need some clarification from you: How will those SupportedFormats be used? Will they be sent to renderer process (or even JS), or some other places? The renderer process (or JS) doesn't need to know raw supported color formats by camera, unless there is a plan to support multiple color formats over IPC. In current implementation, renderer process always sees one color format, i.e., I420.
https://codereview.chromium.org/135213005/diff/1010001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://codereview.chromium.org/135213005/diff/1010001/media/base/android/jav... media/base/android/java/src/org/chromium/media/VideoCapture.java:178: } On 2014/02/14 22:58:07, wjia wrote: > I thought this over again. Need some clarification from you: How will those > SupportedFormats be used? Will they be sent to renderer process (or even JS), or > some other places? > > The renderer process (or JS) doesn't need to know raw supported color formats by > camera, unless there is a plan to support multiple color formats over IPC. In > current implementation, renderer process always sees one color format, i.e., > I420. The supported formats are cached in VideoCaptureManager and made available to renderer process for the constraint resolution process. Many other parts of this infra are in place, also thanks to your reviews, perkj@ is working on its use, and all other platforms are implemented, except Win DirectShow. In time, but imminently, it will also be published to JS via the source API (I don't have much details about it yet). I only know of those two. I think the pixel format is necessary, for at least two use cases: - The camera supports YUV420 or 422 and MJPEG, for the same resolution. Our constraint resolution algorithm might like to specify which one to use, in light of associated fps and excepted CPU usage (for instance, it could choose yuv422 and lower fps, to save cpu in the colorspace conversion). - Very soon, Android video capture will support texture capture. sheu@ is working on this, supporting textures as another pixel format. The supported formats will reflect this possibility, and the renderer will decide to capture via buffer or GPU texture, in light of the available hardware.
Thanks for the explanation! Look forward to seeing patches utilizing this feature. lgtm.
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/135213005/1010001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/135213005/1010001
Message was sent while issue was closed.
Change committed as 251857 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
