Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23903032:
Move NV21 colorspace treatment from VideoCapture java to C++ (Closed)
|
Created:
7 years, 3 months ago by mcasas Modified:
7 years, 3 months ago Reviewers:
tommi (sloooow) - chröme, Jói, bulach, benm (inactive), mnaganov (inactive), wjia(left Chromium), scherkus (not reviewing) CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org, ncarter (slow), Ami GONE FROM CHROMIUM, qinmin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMove NV21 colorspace treatment from VideoCapture java to C++
- VideoCapture.java: Removed NV21 explicit treatment. Added getColorspace() method.
- Added VideoCaptureDeviceAndroid::GetColorspace(), translates between Android and C++ colorspaces.
- Added support for NV21 conversion and rotations in Android to VideoCaptureController.
- Unrelated to the rest, removed unused unused GetIntField in VideoCaptureDeviceAndroid anonymous namespace.
Tested by forcing NV21 in VideoCapture.java [1]
[1] https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android/java/src/org/chromium/media/VideoCapture.java&q=deviceimageformathack&sq=package:chromium&l=55
BUG=288567
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223886
Patch Set 1 #Patch Set 2 : Removed unused GetIntField in VCDA; protected nv21_intermediate_buffer allocation in VCC #
Total comments: 8
Patch Set 3 : joi's comments addressed #Patch Set 4 : reload CL #
Total comments: 4
Patch Set 5 : Added java-c++ automatic enum correspondence #Patch Set 6 : Addressed tommi@'s comment and some other nits. #Patch Set 7 : Try again uploading - didn't work :( #
Total comments: 16
Patch Set 8 : Comments addressed #Patch Set 9 : Rebased. #Patch Set 10 : Try again uploading... #Patch Set 11 : Rebased again - with more attention to ncarter VCC refactoring #Patch Set 12 : Added missing file #
Total comments: 14
Patch Set 13 : Addressed scherkus@'s comments #Patch Set 14 : Added media_android_imageformat_list target to all android webview targets #
Total comments: 2
Patch Set 15 : Added ImageFormat.java automatic generation to android_webview/Android.mk, missed before. #
Total comments: 7
Messages
Total messages: 35 (0 generated)
Hi joi could PTAL? Thanks!
Sorry for the delay reviewing this. Note that I'm not an OWNER for any of this and there are probably more suitable reviewers for the pixel format stuff, but I'm happy to do initial reviews of anything at all as long as you find it helpful. Cheers, Jói https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:444: &nv21_intermediate_buffer_[0] + num_pixels * 5 / 4 , Perhaps I'm reading the NV21 spec wrong, but I thought these were interleaved byte for byte, so vplane should simply point to one byte further than uplane? https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:621: new uint8[frame_info_.width * frame_info_.height * 12 / 8]); For a 1x1 frame, this calculation yields 1, whereas I think we would need it to yield 2. Missing +1 at the end? Or are we guaranteed that width*height is an even number? Also, I'm no authority on this, but I'm wondering if we have code e.g. in VideoFrame or some related classes to do these calculations? If not, should it perhaps be there? https://codereview.chromium.org/23903032/diff/4001/media/base/android/java/sr... File media/base/android/java/src/org/chromium/media/VideoCapture.java (left): https://codereview.chromium.org/23903032/diff/4001/media/base/android/java/sr... media/base/android/java/src/org/chromium/media/VideoCapture.java:425: private void convertNV21ToYV12(byte[] data) { phew, glad that this is gone :) https://codereview.chromium.org/23903032/diff/4001/media/video/capture/androi... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/23903032/diff/4001/media/video/capture/androi... media/video/capture/android/video_capture_device_android.cc:253: if (current_capture_colorspace == With all the continuation indents, I think this if/else if/else if/else statement would have enhanced readability by using curly braces everywhere.
@joi: thanks for the comments!
@bulach: could you PTAL at
media/base/android/java/src/org/chromium/media/VideoCapture.java ? Thanks!
@tommi: could you PTAL at
content/browser/renderer_host/media/video_capture_controller.{cc,h} ? Thanks!
https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/media/video_capture_controller.cc (right):
https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller.cc:444:
&nv21_intermediate_buffer_[0] + num_pixels * 5 / 4 ,
|data| is NV21 and follows your description.
nv21_intermediate_buffer is actually a YUV I420 planar, hence the pointers. I'll
change the name to i420_intermediate_buffer.
https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/media/video_capture_controller.cc:621: new
uint8[frame_info_.width * frame_info_.height * 12 / 8]);
On 2013/09/10 16:31:18, Jói wrote:
> For a 1x1 frame, this calculation yields 1, whereas I think we would need it
to
> yield 2. Missing +1 at the end? Or are we guaranteed that width*height is an
> even number?
It should be an even number, this is enforced right after this lines -- I'll
move it.
>
> Also, I'm no authority on this, but I'm wondering if we have code e.g. in
> VideoFrame or some related classes to do these calculations? If not, should it
> perhaps be there?
Yes absolutely, but this proved a bit controversial in the past.
https://codereview.chromium.org/23903032/diff/4001/media/base/android/java/sr...
File media/base/android/java/src/org/chromium/media/VideoCapture.java (left):
https://codereview.chromium.org/23903032/diff/4001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/VideoCapture.java:425: private
void convertNV21ToYV12(byte[] data) {
Ack!
https://codereview.chromium.org/23903032/diff/4001/media/video/capture/androi...
File media/video/capture/android/video_capture_device_android.cc (right):
https://codereview.chromium.org/23903032/diff/4001/media/video/capture/androi...
media/video/capture/android/video_capture_device_android.cc:253: if
(current_capture_colorspace ==
On 2013/09/10 16:31:18, Jói wrote:
> With all the continuation indents, I think this if/else if/else if/else
> statement would have enhanced readability by using curly braces everywhere.
Done.
On 2013/09/11 09:03:27, miguelao wrote:
> @joi: thanks for the comments!
>
> @bulach: could you PTAL at
> media/base/android/java/src/org/chromium/media/VideoCapture.java ? Thanks!
>
> @tommi: could you PTAL at
> content/browser/renderer_host/media/video_capture_controller.{cc,h} ? Thanks!
>
>
https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
>
>
https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h...
> content/browser/renderer_host/media/video_capture_controller.cc:444:
> &nv21_intermediate_buffer_[0] + num_pixels * 5 / 4 ,
> |data| is NV21 and follows your description.
>
> nv21_intermediate_buffer is actually a YUV I420 planar, hence the pointers.
I'll
> change the name to i420_intermediate_buffer.
>
>
https://codereview.chromium.org/23903032/diff/4001/content/browser/renderer_h...
> content/browser/renderer_host/media/video_capture_controller.cc:621: new
> uint8[frame_info_.width * frame_info_.height * 12 / 8]);
> On 2013/09/10 16:31:18, Jói wrote:
> > For a 1x1 frame, this calculation yields 1, whereas I think we would need it
> to
> > yield 2. Missing +1 at the end? Or are we guaranteed that width*height is an
> > even number?
>
> It should be an even number, this is enforced right after this lines -- I'll
> move it.
>
> >
> > Also, I'm no authority on this, but I'm wondering if we have code e.g. in
> > VideoFrame or some related classes to do these calculations? If not, should
it
> > perhaps be there?
>
> Yes absolutely, but this proved a bit controversial in the past.
>
>
https://codereview.chromium.org/23903032/diff/4001/media/base/android/java/sr...
> File media/base/android/java/src/org/chromium/media/VideoCapture.java (left):
>
>
https://codereview.chromium.org/23903032/diff/4001/media/base/android/java/sr...
> media/base/android/java/src/org/chromium/media/VideoCapture.java:425: private
> void convertNV21ToYV12(byte[] data) {
> Ack!
>
>
https://codereview.chromium.org/23903032/diff/4001/media/video/capture/androi...
> File media/video/capture/android/video_capture_device_android.cc (right):
>
>
https://codereview.chromium.org/23903032/diff/4001/media/video/capture/androi...
> media/video/capture/android/video_capture_device_android.cc:253: if
> (current_capture_colorspace ==
> On 2013/09/10 16:31:18, Jói wrote:
> > With all the continuation indents, I think this if/else if/else if/else
> > statement would have enhanced readability by using curly braces everywhere.
>
> Done.
Looks like you're going to have to upload the cl again. I can't see the diffs.
CL uploaded again - should be fine now :)
just one question - if I'm not missing something then please just fix &buffer[0]+offset to be &buffer[offset] and this lgtm. https://codereview.chromium.org/23903032/diff/11001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/11001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:443: &i420_intermediate_buffer_[0] + num_pixels, what's the difference between this and &i420_intermediate_buffer_[num_pixels]?
https://codereview.chromium.org/23903032/diff/11001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/23903032/diff/11001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.h:161: scoped_ptr<uint8[]> i420_intermediate_buffer_; nit: shouldn't this be a scoped_array instead? or even better, how about just a vector<uint8> ? https://codereview.chromium.org/23903032/diff/11001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/23903032/diff/11001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:249: jclass cls = env->FindClass("android/graphics/ImageFormat"); please, never write JNI directly yourself.... ;)) afaict, you just want to map the value and share the constants between C++ and java, right? here's how to do it: 1. write a "ImageFormat.template", like: https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/... this file will be pre-processed and generated as a java file. 2. write the const values like: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/memory... 3. make Java_VideoCapture_getColorspace map and return the value from (1) 4. use directly the enum here. 5. profit!! :) reason being: although at first it seems slightly more complicated, it's statically typed, so won't be a runtime error should "android/graphics/ImageFormat" change, or NV12 field go missing, or whatever.... all these checks will be done at build time.. then the API between java and c++ is far clearer too, since Java_VideoCapture_getColorSpace will return the mapped value with nice comments saying where the enum is coming from, etc..
Drive by comment https://codereview.chromium.org/23903032/diff/11001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/23903032/diff/11001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.h:161: scoped_ptr<uint8[]> i420_intermediate_buffer_; On 2013/09/11 14:59:07, bulach wrote: > nit: shouldn't this be a scoped_array instead? > or even better, how about just a vector<uint8> ? FWIW This looks fine to me. scoped_array doesn't even exist anymore, scoped_ptr of an array type is the preferred idiom nowadays. ( https://codereview.chromium.org/14081006 was where scoped_array was removed from chromium)
@bulach, I followed your wise instructions and think I cleaned the java-c++ enumeration interfacing. Could you please have another look? Thanks! @tommi: comment taken, thanks!
android jni bits lgtm, thanks!
(I added qinmin@ since he owns the media on android and may want to take a look..)
@bulach: thanks a lot! @scherkus: could you PTAL at media/media.gyp please? Thanks!
drive-by comments. https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:263: #if !defined(OS_IOS) && !defined(OS_ANDROID) Why not use libyuv on clank? I suspect you propagated the meme that "no libyuv on android" introduced in https://codereview.chromium.org/11366084. But that was for the webview build, not clank (ironically, before clank had webrtc support). Further, the OS_IOS check is even more strange because bling doesn't even ship webrtc. I suspect there are performance (and probably correctness) wins to be had by using libyuv everywhere this function might be called. How would you feel about replacing this line with an #ifndef AVOID_LIBYUV_FOR_ANDROID_WEBVIEW ...current libyuv-using-impl #else ...alternative, non-libyuv-using-impl #endif and add to the gyp target building this file a condition like ['OS=="android" and android_webview_build==1', { 'defines': ['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'], }] ? https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... File media/video/capture/android/imageformat_list.h (right): https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/imageformat_list.h:15: DEFINE_ANDROID_IMAGEFORMAT(ANDROID_IMAGEFORMAT_JPEG, 0) If you used the same values that the SDK uses (i.e. JPEG is 256, UNKNOWN is 0, and so on) then VideoCapture.getColorspace() would become a simple return mImageFormat; instead of the switch that it is today. https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:262: // NOTE(mcasas): NV16, JPEG, RGB565 not supported in VideoFormatPixel. indent is strange :) https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:263: default: omitting the default case will let the compiler warn future developers who extend the enum but forget to update this code.
https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:263: #if !defined(OS_IOS) && !defined(OS_ANDROID) On 2013/09/12 17:03:16, Ami Fischman wrote: > Why not use libyuv on clank? > I suspect you propagated the meme that "no libyuv on android" introduced in > https://codereview.chromium.org/11366084. But that was for the webview build, > not clank (ironically, before clank had webrtc support). Further, the OS_IOS > check is even more strange because bling doesn't even ship webrtc. > I suspect there are performance (and probably correctness) wins to be had by > using libyuv everywhere this function might be called. > > How would you feel about replacing this line with an > #ifndef AVOID_LIBYUV_FOR_ANDROID_WEBVIEW > ...current libyuv-using-impl > #else > ...alternative, non-libyuv-using-impl > #endif > and add to the gyp target building this file a condition like > ['OS=="android" and android_webview_build==1', { > 'defines': ['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'], > }] > ? +1 it's also unfortunate to introduce i420_intermediate_buffer_ even though its goes unused when libyuv is being used if we go the #define route we should guard use of that variable https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:636: i420_intermediate_buffer_.reset( does this mean we're allocating memory for i420_intermediate_buffer_ but never using it when libyuv is being used?
@scherkus, @fischman: Thanks for the comments I answered some of them but will take me a couple of days to produce code changes, we're moving office space and my linux box is in, well, a box :) https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:263: #if !defined(OS_IOS) && !defined(OS_ANDROID) It all sounds good and I had a secret post-it to check the use of libyuv in Android. I asked other chromiumers and found indeed not concluding reason why it shouldn't be used, being more performant than the current implementation. I suggest doing the change in another CL and crbug, this one already has a couple of things going on (jni bits etc). FTR: Libyuv works out of the box :) but rotations are not correctly letterboxed. https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:636: i420_intermediate_buffer_.reset( I should guard it with the #ifdef - sigh. https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... File media/video/capture/android/imageformat_list.h (right): https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/imageformat_list.h:15: DEFINE_ANDROID_IMAGEFORMAT(ANDROID_IMAGEFORMAT_JPEG, 0) Do you mean hardcoding the ImageFormat.xyz values??? If I were to do so I wouldn't need all this JNI bricolage :) :)
https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... File media/video/capture/android/imageformat_list.h (right): https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/imageformat_list.h:15: DEFINE_ANDROID_IMAGEFORMAT(ANDROID_IMAGEFORMAT_JPEG, 0) On 2013/09/13 08:42:51, miguelao wrote: > Do you mean hardcoding the ImageFormat.xyz values??? You are already hard-coding values of -1..5. I'm just suggesting that instead of inventing your own values for these things here, you should use the same ones the SDK uses; benefits of doing so include: 1) VideoCapture.getColorspace() becomes a simple getter instead of a translator 2) Lower likelihood of e.g. confusing 0==UNKNOWN in the SDK with 0==JPEG here, and so on. > If I were to do so I wouldn't need all this JNI bricolage :) :) Not sure what you mean by that; I think you'll still need the JNI bridging. (but, of course, if you can avoid that in a way I'm missing, that'd be even better)
scherkus@, fischman@ thanks for the comments, I tried to address them all :) https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:263: #if !defined(OS_IOS) && !defined(OS_ANDROID) FWIW, http://crbug.com/292400 tracks the introduction of libyuv in android. https://codereview.chromium.org/23903032/diff/54001/content/browser/renderer_... content/browser/renderer_host/media/video_capture_controller.cc:636: i420_intermediate_buffer_.reset( On 2013/09/13 08:42:51, miguelao wrote: > I should guard it with the #ifdef - sigh. Done. https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... File media/video/capture/android/imageformat_list.h (right): https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/imageformat_list.h:15: DEFINE_ANDROID_IMAGEFORMAT(ANDROID_IMAGEFORMAT_JPEG, 0) Done not making up new numbers for the imageformats, but I don't think the java VideoCapture::getColorspace() should be a simple getter: this whole .template autogeneration is there to ensure that java and c++ use the same values for the same colorspaces. Note that the ImageFormat constant values _may_ change among Android API's :( https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... File media/video/capture/android/video_capture_device_android.cc (right): https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:262: // NOTE(mcasas): NV16, JPEG, RGB565 not supported in VideoFormatPixel. On 2013/09/12 17:03:16, Ami Fischman wrote: > indent is strange :) Done. https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/video_capture_device_android.cc:263: default: Problem is: is not an enum :( but an integer; we need to add the default and trust the (future) developers to do the "right thing" (TM).
https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... File media/video/capture/android/imageformat_list.h (right): https://codereview.chromium.org/23903032/diff/54001/media/video/capture/andro... media/video/capture/android/imageformat_list.h:15: DEFINE_ANDROID_IMAGEFORMAT(ANDROID_IMAGEFORMAT_JPEG, 0) On 2013/09/13 17:29:00, Ami Fischman wrote: > On 2013/09/13 08:42:51, miguelao wrote: > > Do you mean hardcoding the ImageFormat.xyz values??? > > You are already hard-coding values of -1..5. I'm just suggesting that instead > of inventing your own values for these things here, you should use the same ones > the SDK uses; benefits of doing so include: > 1) VideoCapture.getColorspace() becomes a simple getter instead of a translator > 2) Lower likelihood of e.g. confusing 0==UNKNOWN in the SDK with 0==JPEG here, > and so on. > > > If I were to do so I wouldn't need all this JNI bricolage :) :) > > Not sure what you mean by that; I think you'll still need the JNI bridging. > (but, of course, if you can avoid that in a way I'm missing, that'd be even > better) hey Ami, Miguel, I think I should clarify a bit more here... :) 1) Sharing the same value across java and c++ is indeed convoluted, each solution we tried was lacking in some aspect. 2) using the same value as the SDK is fine, but not having the map could be risky: how stable the SDK values are? if the values are stable, then yes, just using them directly here would be nice! we could potentially have "asserts" in the java getter (it gets compiled out on Release) to ensure the values are the same? 3) hopefully this mapping is not in the critical path :) so this provides the "safest" mechanism of sharing such values.. this pattern is used in other places too:(https://code.google.com/p/chromium/codesearch#search/&q=java_cpp_template.gypi&sq=package:chromium&type=cs)
> > 2) using the same value as the SDK is fine, but not having the map could > be risky: how stable the SDK values are? They're part of the public Android SDK; they're probably more stable than anything in the chromium codebase ;) > if the values are stable, then yes, just using them directly here would be > nice! we could potentially > have "asserts" in the java getter (it gets compiled out on Release) to > ensure the values are the same? > (probably preaching to the choir here, but just in case) Java asserts are not "compiled out" like C++ asserts are (by the c preprocessor) but rather are always compiled into the class bytecode and are conditionally executed based on JVM options, at runtime. On android enabling Java asserts is a matter of setting dalvik properties, and AFAIK none of the chromium bots do this. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:145: // http://crbug.com/292400 . nit: remove trailing " ." https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:149: remove extra blank line https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:280: #if !defined(OS_IOS) && !defined(OS_ANDROID) what about fischman's proposal for a more accurate/descriptive #define such as AVOID_LIBYUV_FOR_ANDROID_WEBVIEW ? https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:461: &i420_intermediate_buffer_[0], yplane, uplane, vplane, i420_intermedia_buffer_.get() ? https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:508: remove extra blank line here https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:642: if ((frame_info_.color == media::PIXEL_FORMAT_NV21) && drop extra ( ) around comparisons https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:643: (i420_intermediate_buffer_.get() == NULL)) { scoped_ptr<T> allows pointers to be testable, so you can rewrite this as: !i420_intermediate_buffer_
scherkus@ comments addressed, thanks for the quick reply! https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:145: // http://crbug.com/292400 . On 2013/09/17 01:52:56, scherkus wrote: > nit: remove trailing " ." Done. https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:149: On 2013/09/17 01:52:56, scherkus wrote: > remove extra blank line Done. https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:280: #if !defined(OS_IOS) && !defined(OS_ANDROID) On 2013/09/17 01:52:56, scherkus wrote: > what about fischman's proposal for a more accurate/descriptive #define such as > AVOID_LIBYUV_FOR_ANDROID_WEBVIEW ? Totally accepted but not done in this cl, tracked in http://crbug.com/292400 and ongoing in http://crrev.com/23444072. https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:461: &i420_intermediate_buffer_[0], yplane, uplane, vplane, On 2013/09/17 01:52:56, scherkus wrote: > i420_intermedia_buffer_.get() ? Done. https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:508: On 2013/09/17 01:52:56, scherkus wrote: > remove extra blank line here Done. https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:642: if ((frame_info_.color == media::PIXEL_FORMAT_NV21) && On 2013/09/17 01:52:56, scherkus wrote: > drop extra ( ) around comparisons Done. https://codereview.chromium.org/23903032/diff/2001/content/browser/renderer_h... content/browser/renderer_host/media/video_capture_controller.cc:643: (i420_intermediate_buffer_.get() == NULL)) { On 2013/09/17 01:52:56, scherkus wrote: > scoped_ptr<T> allows pointers to be testable, so you can rewrite this as: > > !i420_intermediate_buffer_ Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/23903032/85001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_aosp. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
@benm, could you PTAL at android_webview/all_webview.gyp ? Thanks!
Hi mnaganov@, could you PTAL? Thanks!
https://codereview.chromium.org/23903032/diff/97001/android_webview/all_webvi... File android_webview/all_webview.gyp (right): https://codereview.chromium.org/23903032/diff/97001/android_webview/all_webvi... android_webview/all_webview.gyp:23: '../media/media.gyp:media_android_imageformat_list', This should also be added into android_webview/Android.mk (sorry for the trouble) Also, I suspect, this isn't actually used in WebView. Is it possible to get rid of it (either in a subsequent CL)?
Mikhail comments addressed, thanks for the quick reply! https://codereview.chromium.org/23903032/diff/97001/android_webview/all_webvi... File android_webview/all_webview.gyp (right): https://codereview.chromium.org/23903032/diff/97001/android_webview/all_webvi... android_webview/all_webview.gyp:23: '../media/media.gyp:media_android_imageformat_list', On 2013/09/18 12:22:33, Mikhail Naganov (Chromium) wrote: > This should also be added into android_webview/Android.mk (sorry for the > trouble) > Should be done now. > Also, I suspect, this isn't actually used in WebView. Is it possible to get rid > of it (either in a subsequent CL)? Absolutely, see http://crbug.com/292400
LGTM, thanks! android_aosp seems to be happy now. Looking forward for actually removing this dependency.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/23903032/38001
aw/ lgtm
Message was sent while issue was closed.
Change committed as 223886
Message was sent while issue was closed.
Please use "git blame" to figure out reviewer for some files. It'd be great to not have a premature check-in in the future. If this patch is about performance improvement, the performance data comparison should be included in the description. https://chromiumcodereview.appspot.com/23903032/diff/38001/content/browser/re... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://chromiumcodereview.appspot.com/23903032/diff/38001/content/browser/re... content/browser/renderer_host/media/video_capture_controller.cc:462: rotation, flip_vert, flip_horiz); Do you have the performance data comparison between these 2 approaches (i.e., conversion in java and c++)? https://chromiumcodereview.appspot.com/23903032/diff/38001/media/base/android... File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): https://chromiumcodereview.appspot.com/23903032/diff/38001/media/base/android... media/base/android/java/src/org/chromium/media/VideoCapture.java:253: return AndroidImageFormatList.ANDROID_IMAGEFORMAT_RGB_565; Only YV12 and NV21 are used. The rest should be added only when they are in use. https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... File media/video/capture/android/video_capture_device_android.cc (right): https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... media/video/capture/android/video_capture_device_android.cc:142: DCHECK_NE(current_settings_.color, media::PIXEL_FORMAT_UNKNOWN); no need to have media:: https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... media/video/capture/android/video_capture_device_android.cc:251: switch (current_capture_colorspace){ nit: one space between ) and {. https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... media/video/capture/android/video_capture_device_android.cc:252: case ANDROID_IMAGEFORMAT_YV12: Some reviewer has mentioned this before. Please follow http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit... for indent of this switch. https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... media/video/capture/android/video_capture_device_android.cc:260: case ANDROID_IMAGEFORMAT_RGB_565: Only NV21 and YV12 are used. Please remove unused color formats. https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... File media/video/capture/android/video_capture_device_android.h (right): https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... media/video/capture/android/video_capture_device_android.h:61: }; This enum should be moved into cc file.
Message was sent while issue was closed.
On 2013/09/18 18:35:26, wjia wrote: > Please use "git blame" to figure out reviewer for some files. It'd be great to > not have a premature check-in in the future. > > If this patch is about performance improvement, the performance data comparison > should be included in the description. > > https://chromiumcodereview.appspot.com/23903032/diff/38001/content/browser/re... > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > https://chromiumcodereview.appspot.com/23903032/diff/38001/content/browser/re... > content/browser/renderer_host/media/video_capture_controller.cc:462: rotation, > flip_vert, flip_horiz); > Do you have the performance data comparison between these 2 approaches (i.e., > conversion in java and c++)? > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/base/android... > File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/base/android... > media/base/android/java/src/org/chromium/media/VideoCapture.java:253: return > AndroidImageFormatList.ANDROID_IMAGEFORMAT_RGB_565; > Only YV12 and NV21 are used. The rest should be added only when they are in use. > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > File media/video/capture/android/video_capture_device_android.cc (right): > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > media/video/capture/android/video_capture_device_android.cc:142: > DCHECK_NE(current_settings_.color, media::PIXEL_FORMAT_UNKNOWN); > no need to have media:: > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > media/video/capture/android/video_capture_device_android.cc:251: switch > (current_capture_colorspace){ > nit: one space between ) and {. > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > media/video/capture/android/video_capture_device_android.cc:252: case > ANDROID_IMAGEFORMAT_YV12: > Some reviewer has mentioned this before. Please follow > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit... > for indent of this switch. > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > media/video/capture/android/video_capture_device_android.cc:260: case > ANDROID_IMAGEFORMAT_RGB_565: > Only NV21 and YV12 are used. Please remove unused color formats. > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > File media/video/capture/android/video_capture_device_android.h (right): > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > media/video/capture/android/video_capture_device_android.h:61: }; > This enum should be moved into cc file. I disagree about the enum comments: the Android device supports 6 color spaces (or pixel formats), and so is implemented. VideoCaptureController supports only NV21, YUY2 and YV12 and this is reflected in the code (other colorspaces default to PIXEL_FORMAT_UNKNOWN in video_capture_device_android.cc). I will also dig up some performance comparison and address the other comments.
Message was sent while issue was closed.
On 2013/09/18 19:22:44, miguelao wrote: > On 2013/09/18 18:35:26, wjia wrote: > > Please use "git blame" to figure out reviewer for some files. It'd be great to > > not have a premature check-in in the future. > > > > If this patch is about performance improvement, the performance data > comparison > > should be included in the description. > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/content/browser/re... > > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/content/browser/re... > > content/browser/renderer_host/media/video_capture_controller.cc:462: rotation, > > flip_vert, flip_horiz); > > Do you have the performance data comparison between these 2 approaches (i.e., > > conversion in java and c++)? > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/base/android... > > File media/base/android/java/src/org/chromium/media/VideoCapture.java (right): > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/base/android... > > media/base/android/java/src/org/chromium/media/VideoCapture.java:253: return > > AndroidImageFormatList.ANDROID_IMAGEFORMAT_RGB_565; > > Only YV12 and NV21 are used. The rest should be added only when they are in > use. > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > > File media/video/capture/android/video_capture_device_android.cc (right): > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > > media/video/capture/android/video_capture_device_android.cc:142: > > DCHECK_NE(current_settings_.color, media::PIXEL_FORMAT_UNKNOWN); > > no need to have media:: > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > > media/video/capture/android/video_capture_device_android.cc:251: switch > > (current_capture_colorspace){ > > nit: one space between ) and {. > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > > media/video/capture/android/video_capture_device_android.cc:252: case > > ANDROID_IMAGEFORMAT_YV12: > > Some reviewer has mentioned this before. Please follow > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit... > > for indent of this switch. > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > > media/video/capture/android/video_capture_device_android.cc:260: case > > ANDROID_IMAGEFORMAT_RGB_565: > > Only NV21 and YV12 are used. Please remove unused color formats. > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > > File media/video/capture/android/video_capture_device_android.h (right): > > > > > https://chromiumcodereview.appspot.com/23903032/diff/38001/media/video/captur... > > media/video/capture/android/video_capture_device_android.h:61: }; > > This enum should be moved into cc file. > > I disagree about the enum comments: the Android device supports 6 color spaces > (or pixel formats), and so is implemented. VideoCaptureController supports only > NV21, YUY2 and YV12 and this is reflected in the code (other colorspaces default > to PIXEL_FORMAT_UNKNOWN in video_capture_device_android.cc). > > I will also dig up some performance comparison and address the other comments. Android has those color formats defined, but we are not using all of them. The current code only uses NV21 and YV12. There is no point to add non-used color formats. They should be added only when needed. |
