Use libyuv in Android VideoCaptureController (not for Android WebView)

Use libyuv in Android VideoCaptureController.

Added libyuv support and use in all other builds _except_ Android WebView build (trybot android_aosp, different from Chromium WebView builds). Android WebView does _not_ use video capture at all.

Libyuv is not compiled and not used in iOS builds, neither is any video capture related files.


BUG=292400
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226907

[mcasas, 2013-09-23 15:26:45]

Hi wjia@, could you PTAL at video_capture_controller.cc changes? Thanks!

[wjia(left Chromium), 2013-09-23 18:43:49]

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:354: vplane +=
letterbox_width/2;
This handles only landscape case. What about portrait mode?

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:515:
rotation_temp_buffer_.reset(new uint8[frame_info_.width * frame_info_.height *
12 / 8]);
80 chars per line.
2 space indent for a new line.
This rotation_temp_buffer_ is not used anywhere in this file.

[mcasas, 2013-09-24 07:17:06]

Addressed wjia@'s comments, thanks for the quick reply!

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:354: vplane +=
letterbox_width/2;
The landscape case needs all these extra modifications because of the
circumstances described in the comment immediately above, the portrait cases
(angle==0 or angle==180) do not suffer from those complications, and is dealt
with the variables unchanged values - assigned in lines 260-268.

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:515:
rotation_temp_buffer_.reset(new uint8[frame_info_.width * frame_info_.height *
12 / 8]);
> This rotation_temp_buffer_ is not used anywhere in this file.
Absolutely, I removed it locally but forgot to cl upload it.
Done.

[wjia(left Chromium), 2013-09-25 00:38:19]

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/13001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:354: vplane +=
letterbox_width/2;
On 2013/09/24 07:17:06, miguelao wrote:
> The landscape case needs all these extra modifications because of the
> circumstances described in the comment immediately above, the portrait cases
> (angle==0 or angle==180) do not suffer from those complications, and is dealt
> with the variables unchanged values - assigned in lines 260-268.

Sorry, I wasn't clear. My question is: what if the input/output frames are both
in portrait mode? That means the input frame will be rotated to become
landscape, hence cropping left/right, padding top/bottom, and leaving the center
to fit in output frame.

Take a look at media::RotatePlaneByPixels()
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_u...),
it handles both width>height and height>width cases when rotation is 90 or 270.

[mcasas, 2013-09-25 12:10:20]

Hi wjia@, PTAL I added some minor changes to support the situation you
described. PTAL.

(Please note there has been a bunch of intermediate cl uploads having nothing to
do with VCC, but with the WebView build instead).

[wjia(left Chromium), 2013-09-25 18:50:24]

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:260: #if
!defined(AVOID_LIBYUV_FOR_ANDROID_WEBVIEW)
The exclusion here will create video frames with incorrect content, since
rotation is not done. There should be an "else" branch for that case.

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:355: uplane +=
letterbox_thickness/2;
nit: one space before and after operator "/". This one and below.

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:363: }
Please make sure both cases are tested and work correctly.

https://codereview.chromium.org/23444072/diff/76001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/76001/content/content_browser.g...
content/content_browser.gypi:1298: 'defines':
['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
Considering the condition for this branch is 'android_webview_build==1 or
OS=="ios"', this macro should be AVOID_LIBYUV or something like that. It's not
specific for android webview.

[mcasas, 2013-09-25 19:14:38]

Hi wjia@, I replied to two of your comments, and I'll address the nit and the
confusing #ifdef name tomorrow. Thanks!

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:260: #if
!defined(AVOID_LIBYUV_FOR_ANDROID_WEBVIEW)
On 2013/09/25 18:50:25, wjia wrote:
> The exclusion here will create video frames with incorrect content, since
> rotation is not done. There should be an "else" branch for that case.

After much debate with the WebView team, we agreed that, as a temporary stage
before integrating WebRTC in Android WebView build and, given the fact that in
Android WebView there is no Video Capture using this path, it does not matter to
pass around what is effectively black screens. Hence the lack of "#else" path. 

Please note that Android WebView build (trybot android_aosp) is different from
normal chromium WebView build.

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:363: }
On 2013/09/25 18:50:25, wjia wrote:
> Please make sure both cases are tested and work correctly.

They do, but if I understand it correctly, in normal operating conditions case
height>width cannot be exercised since all the modes supported by getUserMedia
are landscape, right?

[mcasas, 2013-09-26 11:28:56]

Hi wjia@'s I addressed your other two comments. Could you PTAL? Thanks!

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/76001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:355: uplane +=
letterbox_thickness/2;
On 2013/09/25 18:50:25, wjia wrote:
> nit: one space before and after operator "/". This one and below.

Done.

https://codereview.chromium.org/23444072/diff/76001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/76001/content/content_browser.g...
content/content_browser.gypi:1298: 'defines':
['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
On 2013/09/25 18:50:25, wjia wrote:
> Considering the condition for this branch is 'android_webview_build==1 or
> OS=="ios"', this macro should be AVOID_LIBYUV or something like that. It's not
> specific for android webview.

Done.

[wjia(left Chromium), 2013-09-26 20:31:34]

https://codereview.chromium.org/23444072/diff/96001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/96001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:260: #if
!defined(AVOID_LIBYUV_FOR_ANDROID_WEBVIEW)
Please add #else branch and assert if (rotation!=0 || flip_* ||
frame_info_.color != I420), since color conversion and rotation is not performed
for that case. This can alert people something is needed here when they add
video capture fully into webview.

https://codereview.chromium.org/23444072/diff/96001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/96001/content/content_browser.g...
content/content_browser.gypi:1300: 'defines':
['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
This won't work, considering how you use AVOID_LIBYUV_FOR_ANDROID_WEBVIEW in VCC
code. For IOS, android_webview_build is 0, but libyuv.h is included in VCC.

[mcasas, 2013-09-27 08:32:35]

Hi wjia@: thanks for the review, please review next iteration, thanks!.

Hi jam@: could you PTAL at content/content_browser.gypi? Thanks!

https://codereview.chromium.org/23444072/diff/96001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/23444072/diff/96001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:260: #if
!defined(AVOID_LIBYUV_FOR_ANDROID_WEBVIEW)
I added a comment in here, a crbug with this and a NOTREACHED().

https://codereview.chromium.org/23444072/diff/96001/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/96001/content/content_browser.g...
content/content_browser.gypi:1300: 'defines':
['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
Yes but iOS builds do not compile anything related to video capture, please note
line 1246 or [1].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/content_br...

[jam, 2013-09-27 17:55:43]

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
content/content_browser.gypi:1299: }, { # android_webview_build!=0
why not just android_webview_build==1?

[mcasas, 2013-09-30 17:57:46]

Hi jam@ thanks for your time, could you please take another look?

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
content/content_browser.gypi:1299: }, { # android_webview_build!=0
On 2013/09/27 17:55:43, jam wrote:
> why not just android_webview_build==1?

The only explicit setting of webview is =0 at [1] so to be correct, we have to
treat all cases when webview != 0, IOW, it could be -1 or 1 or anything like
that (and I suspect it might) :).

Anyway this code will disappear after http://crbug.com/299611 .

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...

[wjia(left Chromium), 2013-09-30 18:14:25]

lgtm on video_capture_controller.cc.

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
content/content_browser.gypi:1299: }, { # android_webview_build!=0
On 2013/09/30 17:57:46, miguelao wrote:
> On 2013/09/27 17:55:43, jam wrote:
> > why not just android_webview_build==1?
> 
> The only explicit setting of webview is =0 at [1] so to be correct, we have to
> treat all cases when webview != 0, IOW, it could be -1 or 1 or anything like
> that (and I suspect it might) :).
> 
> Anyway this code will disappear after http://crbug.com/299611 .
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...

I think jam might also mean using android_webiew_build!=0 and removing the empty
body branch.

[mcasas, 2013-10-01 11:32:28]

Cleaned up content_browser.gypi according to jam@ and wjia@'s comments. PTAL!

[jam, 2013-10-02 21:13:56]

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
content/content_browser.gypi:1299: }, { # android_webview_build!=0
On 2013/09/30 17:57:46, miguelao wrote:
> On 2013/09/27 17:55:43, jam wrote:
> > why not just android_webview_build==1?
> 
> The only explicit setting of webview is =0 at [1] so to be correct, we have to
> treat all cases when webview != 0, IOW, it could be -1 or 1 or anything like
> that (and I suspect it might) :).
> 
> Anyway this code will disappear after http://crbug.com/299611 .
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...

i don't understand what you mean. why would it be -1?

it seems other places check for ==1 already:
https://code.google.com/p/chromium/codesearch#search/&q=%22android_webview_bu...

[mcasas, 2013-10-02 21:20:04]

On 2013/10/02 21:13:56, jam wrote:
>
https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
> File content/content_browser.gypi (right):
> 
>
https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
> content/content_browser.gypi:1299: }, { # android_webview_build!=0
> On 2013/09/30 17:57:46, miguelao wrote:
> > On 2013/09/27 17:55:43, jam wrote:
> > > why not just android_webview_build==1?
> > 
> > The only explicit setting of webview is =0 at [1] so to be correct, we have
to
> > treat all cases when webview != 0, IOW, it could be -1 or 1 or anything like
> > that (and I suspect it might) :).
> > 
> > Anyway this code will disappear after http://crbug.com/299611 .
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
> 
> i don't understand what you mean. why would it be -1?
> 
> it seems other places check for ==1 already:
>
https://code.google.com/p/chromium/codesearch#search/&q=%2522android_webview_...

Doesn't matter since I changed it to read like:

   ['android_webview_build!=0', {
     'defines': ['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
   }],

[mcasas, 2013-10-02 21:20:06]

On 2013/10/02 21:13:56, jam wrote:
>
https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
> File content/content_browser.gypi (right):
> 
>
https://codereview.chromium.org/23444072/diff/104001/content/content_browser....
> content/content_browser.gypi:1299: }, { # android_webview_build!=0
> On 2013/09/30 17:57:46, miguelao wrote:
> > On 2013/09/27 17:55:43, jam wrote:
> > > why not just android_webview_build==1?
> > 
> > The only explicit setting of webview is =0 at [1] so to be correct, we have
to
> > treat all cases when webview != 0, IOW, it could be -1 or 1 or anything like
> > that (and I suspect it might) :).
> > 
> > Anyway this code will disappear after http://crbug.com/299611 .
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
> 
> i don't understand what you mean. why would it be -1?
> 
> it seems other places check for ==1 already:
>
https://code.google.com/p/chromium/codesearch#search/&q=%2522android_webview_...

Doesn't matter since I changed it to read like:

   ['android_webview_build!=0', {
     'defines': ['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
   }],

[jam, 2013-10-03 00:36:06]

> > i don't understand what you mean. why would it be -1?
> > 
> > it seems other places check for ==1 already:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%252522android_webvie...
> 
> Doesn't matter since I changed it to read like:
> 
>    ['android_webview_build!=0', {
>      'defines': ['AVOID_LIBYUV_FOR_ANDROID_WEBVIEW'],
>    }],

can you please explain why android_webview_build==1 doesn't work? we use it in
other gypis. double negatives are harder to read imo

[mcasas, 2013-10-03 11:38:29]

Hi jam@, I changed it to direct comparisons and it works fine. I think I was
confused before with some other changes I made. I think that
|android_webview_build| has only values 0 and 1 :)

['android_webview_build==1', {
...

[jam, 2013-10-03 21:26:47]

lgtm

[commit-bot: I haz the power, 2013-10-03 21:44:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/23444072/126001

[commit-bot: I haz the power, 2013-10-04 00:19:26]

Change committed as 226907