Change paused frame of WebMediaPlayerMS from YV12 to I420

Change paused frame of WebMediaPlayerMS from YV12 to I420

We copy the last frame in a pause for media stream.
- Changed the copied frame to I420 to be consistent with the general format assumption.
- I use libyuv rather than direct memsets for copy and conversion.

BUG=541728
Committed: https://crrev.com/60a8bbef3d7b1c3e537c5b78d749cae3cf18b3b3
Cr-Commit-Position: refs/heads/master@{#355098}

[emircan, 2015-10-13 05:37:57]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-13 05:39:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394783004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394783004/20001

[commit-bot: I haz the power, 2015-10-13 05:53:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-13 05:53:07]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[emircan, 2015-10-13 19:52:56]

Patchset #1 (id:1) has been deleted

[emircan, 2015-10-13 20:05:35]

Patchset #1 (id:20001) has been deleted

[emircan, 2015-10-13 20:09:40]

emircan@chromium.org changed reviewers:
+ fbarchard@chromium.org, mcasas@chromium.org

[emircan, 2015-10-13 20:09:52]

PTAL.

[emircan, 2015-10-13 22:21:46]

Patchset #1 (id:40001) has been deleted

[mcasas, 2015-10-13 22:44:09]

LGTM. You'll need Owners though.

[fbarchard, 2015-10-14 06:47:36]

Its good to weed out the YV12 references... its really YU12.
Looks okay, but I'd like to check the rowbytes is not needed, and that Android
wants ARGB not ABGR.
Thanks for doing this!

https://chromiumcodereview.appspot.com/1394783004/diff/60001/content/renderer...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://chromiumcodereview.appspot.com/1394783004/diff/60001/content/renderer...
content/renderer/media/webmediaplayer_ms.cc:82:
libyuv::ConvertToI420(reinterpret_cast<uint8*>(bitmap.getPixels()),
Consider calling ARGBToI420.

ConvertToI420 expects a contiguous buffer, where bytes per row is 4 * width.
The original function media::CopyRGBToVideoFrame took bitmap.rowBytes(),
allowing for row padding.
ARGBToI420 takes rowbytes:

// Convert ARGB To I420.
LIBYUV_API
int ARGBToI420(const uint8* src_argb, int src_stride_argb,
               uint8* dst_y, int dst_stride_y,
               uint8* dst_u, int dst_stride_u,
               uint8* dst_v, int dst_stride_v,
               int width, int height);

https://chromiumcodereview.appspot.com/1394783004/diff/60001/content/renderer...
content/renderer/media/webmediaplayer_ms.cc:95: libyuv::kRotate0,
libyuv::FOURCC_ARGB);
Most code in media expects ABGR for Android.  Can you confirm this instance
always wants ARGB?

[emircan, 2015-10-14 21:02:32]

https://codereview.chromium.org/1394783004/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/1394783004/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:82:
libyuv::ConvertToI420(reinterpret_cast<uint8*>(bitmap.getPixels()),
On 2015/10/14 06:47:36, fbarchard wrote:
> Consider calling ARGBToI420.
> 
> ConvertToI420 expects a contiguous buffer, where bytes per row is 4 * width.
> The original function media::CopyRGBToVideoFrame took bitmap.rowBytes(),
> allowing for row padding.
> ARGBToI420 takes rowbytes:
> 
> // Convert ARGB To I420.
> LIBYUV_API
> int ARGBToI420(const uint8* src_argb, int src_stride_argb,
>                uint8* dst_y, int dst_stride_y,
>                uint8* dst_u, int dst_stride_u,
>                uint8* dst_v, int dst_stride_v,
>                int width, int height);  

Done.

https://codereview.chromium.org/1394783004/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:95: libyuv::kRotate0,
libyuv::FOURCC_ARGB);
On 2015/10/14 06:47:36, fbarchard wrote:
> Most code in media expects ABGR for Android.  Can you confirm this instance
> always wants ARGB?

Sorry, I couldn't understand what you mean there. Can you elaborate?

We want output to be I420. It boils down to
VideoResourceUpdater::CreateForSoftwarePlanes() for compositing.

AFAIK, the input is RGB32. The earlier function used there was an RGB32
conversion. SKBitmap outputs ARGB as well.

[fbarchard, 2015-10-15 17:43:26]

I dont have a benchmark for the old media function, but the new one is very fast
For 720p on a Sandy Bridge ARGBToI420_Opt 0.324 ms/frame.
A memcpy is I420ToI420_Opt 0.121 ms/frame.

https://codereview.chromium.org/1394783004/diff/60001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/1394783004/diff/60001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:95: libyuv::kRotate0,
libyuv::FOURCC_ARGB);
On 2015/10/14 21:02:32, emircan wrote:
> On 2015/10/14 06:47:36, fbarchard wrote:
> > Most code in media expects ABGR for Android.  Can you confirm this instance
> > always wants ARGB?
> 
> Sorry, I couldn't understand what you mean there. Can you elaborate?
> 
> We want output to be I420. It boils down to
> VideoResourceUpdater::CreateForSoftwarePlanes() for compositing.
> 
> AFAIK, the input is RGB32. The earlier function used there was an RGB32
> conversion. SKBitmap outputs ARGB as well.

In media/blink/skcanvas_video_renderer.cc there are ifdefs like this:

// Skia internal format depends on a platform. On Android it is ABGR, on others
// it is ARGB.
#if SK_B32_SHIFT == 0 && SK_G32_SHIFT == 8 && SK_R32_SHIFT == 16 && \
    SK_A32_SHIFT == 24
#define LIBYUV_I420_TO_ARGB libyuv::I420ToARGB
#else
#define LIBYUV_I420_TO_ARGB libyuv::I420ToABGR
..
So I just wanted to check that you are sure, on Android, that you want the same
format as other platforms, which is ARGB.

In libyuv terminology, ARGB ordering follows fourcc.org/directx, where
ARGB is BGRA in memory, or ARGB in a register
ABGR is RGBA in memory
RGB24 is BGR in memory
RAW is RGB in memory
https://code.google.com/p/libyuv/wiki/Formats

[fbarchard, 2015-10-15 17:43:59]

lgtm

[emircan, 2015-10-16 17:37:12]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-10-16 17:37:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
fbarchard@chromium.org
Link to the patchset: https://codereview.chromium.org/1394783004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-16 17:37:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394783004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394783004/100001

[commit-bot: I haz the power, 2015-10-16 17:45:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-16 17:46:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[fbarchard, 2015-10-16 17:49:33]

Changed description from NV12 to YV12, to match code.

[emircan, 2015-10-16 21:36:57]

emircan@chromium.org changed reviewers:
+ sievers@chromium.org

[emircan, 2015-10-16 21:36:58]

Thanks for the correction fbarchard@. I just realized that I also need OWNERS
review on BUILD.gn.

sievers@chromium.org: Please review changes in BUILD.gn

[no sievers, 2015-10-16 22:12:24]

https://codereview.chromium.org/1394783004/diff/100001/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/1394783004/diff/100001/content/renderer/BUILD...
content/renderer/BUILD.gn:105: "//third_party/libyuv",
This is not an android change, so why here?
Also it doesn't match gyp where it's added as a dependency for webrtc and
plugins enabled. (At least the former already happens below.)

[emircan, 2015-10-16 22:27:31]

https://codereview.chromium.org/1394783004/diff/100001/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/1394783004/diff/100001/content/renderer/BUILD...
content/renderer/BUILD.gn:105: "//third_party/libyuv",
On 2015/10/16 22:12:23, sievers wrote:
> This is not an android change, so why here?
> Also it doesn't match gyp where it's added as a dependency for webrtc and
> plugins enabled. (At least the former already happens below.)

Initially android bots failed [0] and I looked for the android section to add
the libyuv dependency. But I just realized (!enable_webrtc && is_android)
section as well. Should I move it there?

https://codereview.chromium.org/1394783004/#msg4

[no sievers, 2015-10-16 22:51:31]

On 2015/10/16 22:27:31, emircan wrote:
>
https://codereview.chromium.org/1394783004/diff/100001/content/renderer/BUILD.gn
> File content/renderer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1394783004/diff/100001/content/renderer/BUILD...
> content/renderer/BUILD.gn:105: "//third_party/libyuv",
> On 2015/10/16 22:12:23, sievers wrote:
> > This is not an android change, so why here?
> > Also it doesn't match gyp where it's added as a dependency for webrtc and
> > plugins enabled. (At least the former already happens below.)
> 
> Initially android bots failed [0] and I looked for the android section to add
> the libyuv dependency. But I just realized (!enable_webrtc && is_android)
> section as well. Should I move it there?
> 
> https://codereview.chromium.org/1394783004/#msg4

Still don't get what this has to do with android.

[emircan, 2015-10-16 22:57:16]

Patchset #4 (id:120001) has been deleted

[emircan, 2015-10-19 18:30:27]

Patchset #4 (id:140001) has been deleted

[emircan, 2015-10-19 18:33:51]

On 2015/10/16 22:51:31, sievers wrote:
>
> Still don't get what this has to do with android.

Take a look at PS#4 where I removed the gn changes. AFAIU, libyuv is included
inside WebRTC, and when WebRTC isn't enabled, it is not attached and results in
compile errors. There fore, I added libyuv as a dependancy on (!enable_webrtc &&
is_android) case on PS#5. WDYT?

[no sievers, 2015-10-19 20:30:40]

On 2015/10/19 18:33:51, emircan wrote:
> On 2015/10/16 22:51:31, sievers wrote:
> >
> > Still don't get what this has to do with android.
> 
> Take a look at PS#4 where I removed the gn changes. AFAIU, libyuv is included
> inside WebRTC, and when WebRTC isn't enabled, it is not attached and results
in
> compile errors. There fore, I added libyuv as a dependancy on (!enable_webrtc
&&
> is_android) case on PS#5. WDYT?

I'm just wondering if this all somehow just ends up accidentally linking
correctly in the different configs, and you're adding one more tweak on top.
Doesn't content_renderer just always have to explicitly depend on libyuv on all
platforms regardless of the config, because it always compiles the source file
in which you are introducing the libyuv dependency?

[emircan, 2015-10-19 22:51:02]

On 2015/10/19 20:30:40, sievers wrote:
> I'm just wondering if this all somehow just ends up accidentally linking
> correctly in the different configs, and you're adding one more tweak on top.
> Doesn't content_renderer just always have to explicitly depend on libyuv on
all
> platforms regardless of the config, because it always compiles the source file
> in which you are introducing the libyuv dependency?

As we discussed, I removed all conditional dependencies and added a general
dependency. The bots seem to be fine. PTAL.

[fbarchard, 2015-10-20 01:02:41]

lgtm

[no sievers, 2015-10-20 14:27:09]

On 2015/10/19 22:51:02, emircan wrote:
> On 2015/10/19 20:30:40, sievers wrote:
> > I'm just wondering if this all somehow just ends up accidentally linking
> > correctly in the different configs, and you're adding one more tweak on top.
> > Doesn't content_renderer just always have to explicitly depend on libyuv on
> all
> > platforms regardless of the config, because it always compiles the source
file
> > in which you are introducing the libyuv dependency?
> 
> As we discussed, I removed all conditional dependencies and added a general
> dependency. The bots seem to be fine. PTAL.

lgtm, thanks!

[emircan, 2015-10-20 17:30:54]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-10-20 17:30:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1394783004/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-10-20 17:32:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394783004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394783004/200001

[commit-bot: I haz the power, 2015-10-20 17:51:23]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2015-10-20 17:53:06]

Patchset 6 (id:??) landed as
https://crrev.com/60a8bbef3d7b1c3e537c5b78d749cae3cf18b3b3
Cr-Commit-Position: refs/heads/master@{#355098}