Remove redundant I420-to-YV12 conversion in MediaStreamVideoTrack.

Remove redundant I420-to-YV12 conversion in MediaStreamVideoTrack.

The I420 and YV12 formats both are almost identical except for the order
of U and V planes in memory. They are effectively the same for renderers
that access U and V plane data via MediaFrame::data(size_t plane).

BUG=341452
BUG=355763
TEST=trybot
R=danakj@chromium.org, dmichael@chromium.org, fischman@chromium.org, perkj@chromium.org, piman@chromium.org

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

[hshi1, 2014-04-02 17:32:09]

PTAL - I propose that we treat I420 the same as YV12 everywhere that accesses
the U and V planes via method VideoFrame::data(size_t plane), because this hides
the order of U and V planes in memory layout, and the two formats are
effectively identical.

CC yzshen@ and penghuang@ to comment whether Pepper can support I420 format.
From my code inspection, I believe it is already supported.

[danakj, 2014-04-02 17:37:56]

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
cc/resources/video_resource_updater.cc:64: case media::VideoFrame::I420:
Won't you need to make some changes in the gl_renderer/software_renderer, or
elsewhere to get the planes in the correct order?

Can you add a pixel test with this video format? See
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/renderer...

[hshi1, 2014-04-02 17:47:33]

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
cc/resources/video_resource_updater.cc:64: case media::VideoFrame::I420:
On 2014/04/02 17:37:57, danakj wrote:
> Won't you need to make some changes in the gl_renderer/software_renderer, or
> elsewhere to get the planes in the correct order?

No changes are necessary in gl_renderer/software_renderer because they all go
through media::VideoFrame::data(size_t plane) - which handles YV12 and I420
correctly - to obtain the pointers to the U and V planes. They never rely on any
assumption on how Y,U and V planes are ordered in memory.

For example see skcanvas_video_renderer.cc: function ConvertVideoFrameToBitmap()
calls media::ConvertYUVToRGB32() using separate
video_frame->data(media::VideoFrame::kYPlane),
video_frame->data(media::VideoFrame::kUPlane) and
video_frame->data(media::VideoFrame::kVPlane) pointers.

For example see gl_video_renderer.cc: GlVideoRenderer::Paint() sets up
glTexImage2D for each of the Y,U,V planes, also using video_frame->data(i).

That's why I claimed that YV12 and I420 are effectively the same. It only makes
a difference if the renderer tries to manually calculate U and V plane addresses
based on surface width/height and the base address, which it should NEVER do.

> 
> Can you add a pixel test with this video format? See
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/renderer...

[hshi1, 2014-04-02 17:51:16]

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
cc/resources/video_resource_updater.cc:64: case media::VideoFrame::I420:
On 2014/04/02 17:37:57, danakj wrote:
> Can you add a pixel test with this video format? See
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/renderer...

Thanks for the suggestion and I looked at the renderer_pixeltest.cc, however I'm
afraid this is already testing at a lower level and the test itself is agnostic
of the "media" video formats (e.g. YV12 vs I420). You can see in the test that
it doesn't use media::VideoFrame class. The CreateTestYUVVideoDrawQuad() sets up
the Y, U and V planes directly.

[danakj, 2014-04-02 17:51:59]

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/222563002/diff/1/cc/resources/video_resource_...
cc/resources/video_resource_updater.cc:64: case media::VideoFrame::I420:
On 2014/04/02 17:47:34, hshi1 wrote:
> On 2014/04/02 17:37:57, danakj wrote:
> > Won't you need to make some changes in the gl_renderer/software_renderer, or
> > elsewhere to get the planes in the correct order?
> 
> No changes are necessary in gl_renderer/software_renderer because they all go
> through media::VideoFrame::data(size_t plane) - which handles YV12 and I420
> correctly - to obtain the pointers to the U and V planes. They never rely on
any
> assumption on how Y,U and V planes are ordered in memory.
> 
> For example see skcanvas_video_renderer.cc: function
ConvertVideoFrameToBitmap()
> calls media::ConvertYUVToRGB32() using separate
> video_frame->data(media::VideoFrame::kYPlane),
> video_frame->data(media::VideoFrame::kUPlane) and
> video_frame->data(media::VideoFrame::kVPlane) pointers.
> 
> For example see gl_video_renderer.cc: GlVideoRenderer::Paint() sets up
> glTexImage2D for each of the Y,U,V planes, also using video_frame->data(i).
> 
> That's why I claimed that YV12 and I420 are effectively the same. It only
makes
> a difference if the renderer tries to manually calculate U and V plane
addresses
> based on surface width/height and the base address, which it should NEVER do.
> 
> > 
> > Can you add a pixel test with this video format? See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/renderer...
> 

Ahh, okay. Gotcha now. The pixel test I pointed to would be no different then
since it's not using VideoResourceUpdater.

We should have some pixel-test coverage of this to ensure the
VideoResourceUpdater does the right thing forever. Think you can modify those
tests to use VideoResourceUpdater instead of doing it manually?

[hshi1, 2014-04-02 18:00:16]

On 2014/04/02 17:51:59, danakj wrote:
> Ahh, okay. Gotcha now. The pixel test I pointed to would be no different then
> since it's not using VideoResourceUpdater.
> 
> We should have some pixel-test coverage of this to ensure the
> VideoResourceUpdater does the right thing forever. Think you can modify those
> tests to use VideoResourceUpdater instead of doing it manually?

I'm not really sure how I can test VideoResourceUpdater in the
renderer_pixeltest.cc. 

I could add YV12 and I420 tests in video_resource_updater_unittest.cc to ensure
that these 2 formats are supported, but if I were to add a pixel test coverage
I'd be not really knowing what I'm doing.

[danakj, 2014-04-02 18:16:15]

On Wed, Apr 2, 2014 at 2:00 PM, <hshi@chromium.org> wrote:

> On 2014/04/02 17:51:59, danakj wrote:
>
>> Ahh, okay. Gotcha now. The pixel test I pointed to would be no different
>> then
>> since it's not using VideoResourceUpdater.
>>
>
>  We should have some pixel-test coverage of this to ensure the
>> VideoResourceUpdater does the right thing forever. Think you can modify
>> those
>> tests to use VideoResourceUpdater instead of doing it manually?
>>
>
> I'm not really sure how I can test VideoResourceUpdater in the
> renderer_pixeltest.cc.
>
> I could add YV12 and I420 tests in video_resource_updater_unittest.cc to
> ensure
> that these 2 formats are supported, but if I were to add a pixel test
> coverage
> I'd be not really knowing what I'm doing.
>

As you said, right now it creates all the resources and does SetPixels in
the test manually. Instead, you'd make a media::VideoFrame in the test and
a VideoResourceUpdater, then call CreateExternalResourcesFromVideoFrame
with the media::VideoFrame.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/video...

This returns a VideoFrameExternalResources struct which has a number of
TextureMailboxes on it. Then you'd do as VideoLayerImpl does to produce a
quad from that by
a) making resources
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...
b) making a quad
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/video_la...

If you have any questions I'd be happy to help. I'd just hate to see us
have no test coverage for a format and then one day we'll definitely break
it. Video has historically been one of the things that breaks pretty often
in cc since we've had poor test coverage - which is why I'm trying to push
for a test here, if it's possible for you to do that.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-04-02 19:56:30]

LGTM % Dana's comments.

[hshi1, 2014-04-02 20:13:03]

danakj: I uploaded patch set #2 that uses the VideoResourceUpdater to create YUV
resources.

Unfortunately the tests are failing because all the pixels are mismatching
expected values.

For example the SimpleYUVRect test reports actual RGBA for all pixels are
(0,135,0,0) while the expected values should form a stripe pattern. Any ideas?

[hshi1, 2014-04-02 20:24:41]

On 2014/04/02 20:13:03, hshi1 wrote:
> danakj: I uploaded patch set #2 that uses the VideoResourceUpdater to create
YUV
> resources.
> 
> Unfortunately the tests are failing because all the pixels are mismatching
> expected values.
> 
> For example the SimpleYUVRect test reports actual RGBA for all pixels are
> (0,135,0,0) while the expected values should form a stripe pattern. Any ideas?

See patch set #3. I moved the set pixels code up before calling the updated as
suggested by danakj@.

Here's the errors I get prior to the pixel mismatches:

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from VideoGLRendererPixelTest
[ RUN      ] VideoGLRendererPixelTest.SimpleYUVRect
[15580:15581:0402/132124:1574622468978:ERROR:gles2_cmd_decoder.cc(10152)]
[GroupMarkerNotSet(crbug.com/242999)!:D083585DA9060000]GL ERROR
:GL_INVALID_OPERATION : glConsumeTextureCHROMIUM: invalid mailbox name
[15580:15581:0402/132124:1574622469061:ERROR:gles2_cmd_decoder.cc(10152)]
[GroupMarkerNotSet(crbug.com/242999)!:D083585DA9060000]GL ERROR
:GL_INVALID_OPERATION : glConsumeTextureCHROMIUM: invalid mailbox name
[15580:15581:0402/132124:1574622469103:ERROR:gles2_cmd_decoder.cc(10152)]
[GroupMarkerNotSet(crbug.com/242999)!:D083585DA9060000]GL ERROR
:GL_INVALID_OPERATION : glConsumeTextureCHROMIUM: invalid mailbox name
[15580:15581:0402/132124:1574622478047:ERROR:gles2_cmd_decoder.cc(6036)]
[GroupMarkerNotSet(crbug.com/242999)!:D083585DA9060000]RENDER WARNING: texture
bound to texture unit 3 is not renderable. It maybe non-power-of-2 and have
incompatible texture filtering or is not 'texture complete'
[15580:15581:0402/132124:1574622478096:ERROR:gles2_cmd_decoder.cc(6036)]
[GroupMarkerNotSet(crbug.com/242999)!:D083585DA9060000]RENDER WARNING: texture
bound to texture unit 2 is not renderable. It maybe non-power-of-2 and have
incompatible texture filtering or is not 'texture complete'
[15580:15581:0402/132124:1574622478113:ERROR:gles2_cmd_decoder.cc(6036)]
[GroupMarkerNotSet(crbug.com/242999)!:D083585DA9060000]RENDER WARNING: texture
bound to texture unit 1 is not renderable. It maybe non-power-of-2 and have
incompatible texture filtering or is not 'texture complete'

[hshi1, 2014-04-02 21:12:09]

PTAL patch set #5. The tests are passing now.

Previously they were failing because
(1) |updater| got destroyed -- I made it a member.
(2) we were using the wrong stride -- fixed.

[hshi1, 2014-04-02 21:15:59]

+fischman for the changes under media/ - thanks!

[danakj, 2014-04-02 21:22:11]

Thanks, cc LGTM

[Ami GONE FROM CHROMIUM, 2014-04-02 22:03:58]

LGTM % nits

https://codereview.chromium.org/222563002/diff/80009/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/222563002/diff/80009/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:543: media::VideoFrame::I420,
nit: right now this is the only mention of I420 in this file so could look like
a typo.  I'd point out either in a comment or in the test-case name that this is
intentional for coverage.

https://codereview.chromium.org/222563002/diff/80009/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/222563002/diff/80009/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:15: static bool
IsEitherYV12OrYV16(media::VideoFrame::Format format) {
This name is bad now.
IsYUV()
?

[hshi1, 2014-04-02 22:17:05]

https://codereview.chromium.org/222563002/diff/80009/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/222563002/diff/80009/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:543: media::VideoFrame::I420,
On 2014/04/02 22:03:58, Ami Fischman wrote:
> nit: right now this is the only mention of I420 in this file so could look
like
> a typo.  I'd point out either in a comment or in the test-case name that this
is
> intentional for coverage.

Done.

https://codereview.chromium.org/222563002/diff/80009/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/222563002/diff/80009/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:15: static bool
IsEitherYV12OrYV16(media::VideoFrame::Format format) {
On 2014/04/02 22:03:58, Ami Fischman wrote:
> This name is bad now.
> IsYUV()
> ?

WDYT of my new patch?

[Ami GONE FROM CHROMIUM, 2014-04-02 22:21:51]

PS#7 still LGTM.

[hshi1, 2014-04-02 22:23:15]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-04-02 22:25:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/222563002/110001

[commit-bot: I haz the power, 2014-04-03 00:45:49]

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

[commit-bot: I haz the power, 2014-04-03 00:45:50]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[hshi1, 2014-04-03 01:00:54]

sorry, +yzshen PTAL (OWNERS of ppapi/test)

penghuang@ confirmed in email that ppapi MediaStreamVideoTrack supports I420 as
well as YV12.

[perkj_chrome, 2014-04-03 07:12:47]

On 2014/04/03 01:00:54, hshi1 wrote:
> sorry, +yzshen PTAL (OWNERS of ppapi/test)
> 
> penghuang@ confirmed in email that ppapi MediaStreamVideoTrack supports I420
as
> well as YV12.

LGTM
Thanks a lot!

[hshi1, 2014-04-03 17:02:28]

+dmichael PTAL ppapi/ thanks! (I initially added yzshen@ but he's OOO).

[dmichael (off chromium), 2014-04-03 17:08:06]

ppapi lgtm

[hshi1, 2014-04-03 17:09:08]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-04-03 17:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/222563002/130001

[hshi1, 2014-04-03 19:11:08]

Committed patchset #9 manually as r261497 (presubmit successful).