Motown: Video renderer library code

Motown: Video renderer library code
We don't currently have a good way to get video frames into Mozart under
the covers, so an application that wants to render video essentially
needs to *be* a video renderer. This CL adds library code that makes
that easy. This should go away once Motown and Mozart have come to
terms. There are three new classes:
1) VideoRenderer, which implements MediaRenderer, MediaConsumer,
   MediaTimelineControlSite and TimelineConsumer and coughs up RGBA
   video with pretty good synchronization.
2) VideoConverter, which converts YV12 payloads produced by Motown into
   interleaved RGBA. This should be done by the GPU one day. It involves
   a 64MB colorspace conversion table that's built at runtime. Yay!
   It's hacky but temporary.
3) VideoPacketLayout, which understands how the 'planar' data in video
   packet payloads is laid out. This is largely lifted from
   services/media/framework/types/video_stream_type.*. The code
   duplication should go away once the conversion is done on the GPU.

R=kulakowski@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/97eb9c38342b0646097e30f07dd7309452dca363

[dalesat, 2016-06-15 23:13:42]

Description was changed from

==========
Motown: Video renderer library code
We don't currently have a good way to get video frames into Mozart under
the covers, so an application that wants to render video essentially
needs to *be* a video renderer. This CL adds library code that makes
that easy. This should go away once Motown and Mozart have come to
terms. There are three new classes:
1) VideoRenderer, which implements MediaRenderer, MediaConsumer,
   MediaTimelineControlSite and TimelineConsumer and coughs up RGBA
   video with pretty good synchronization.
2) VideoConverter, which converts YV12 payloads produced by Motown into
   interleaved RGBA. This should be done by the GPU one day. It involves
   a 64MB colorspace conversion table that's built at runtime. Yay!
   It's hacky but temporary.
3) VideoPacketLayout, which understands how the 'planar' data in video
   packet payloads is laid out. This is largely lifted from
   services/media/framework/types/video_stream_type.*. The code
   duplication should go away once the conversion is done on the GPU.
==========

to

==========
Motown: Video renderer library code
We don't currently have a good way to get video frames into Mozart under
the covers, so an application that wants to render video essentially
needs to *be* a video renderer. This CL adds library code that makes
that easy. This should go away once Motown and Mozart have come to
terms. There are three new classes:
1) VideoRenderer, which implements MediaRenderer, MediaConsumer,
   MediaTimelineControlSite and TimelineConsumer and coughs up RGBA
   video with pretty good synchronization.
2) VideoConverter, which converts YV12 payloads produced by Motown into
   interleaved RGBA. This should be done by the GPU one day. It involves
   a 64MB colorspace conversion table that's built at runtime. Yay!
   It's hacky but temporary.
3) VideoPacketLayout, which understands how the 'planar' data in video
   packet payloads is laid out. This is largely lifted from
   services/media/framework/types/video_stream_type.*. The code
   duplication should go away once the conversion is done on the GPU.
==========

[dalesat, 2016-06-15 23:13:42]

dalesat@chromium.org changed reviewers:
+ kulakowski@chromium.org

[dalesat, 2016-06-15 23:13:55]

Please take a look. Thanks!

[kulakowski, 2016-06-17 21:15:40]

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/video_converter.cc (right):

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:19: if (f < 0.0f) {
I'm paranoid having once seen someone spend days debugging such a function. I
believe that you correctly handle the undefined behavior via this clamping.
Double checking that it also appropriately handles -0. / denormals / NaNs.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:30: size_t
ColorspaceTableOffset(uint8_t y, uint8_t u, uint8_t v) {
Why do this math yourself and not write [y][u][v]?

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:41: for (float y = 0.0f; y <
256.0f; y += 1.0f) {
I hope you typed these constants correctly :)

I believe that the for loop bounds are correct at floats this size, but man,
it's a little spooky seeing loop bounds being float comparisons.

I'm also curious if the preferred specification of this color table is in terms
of real math, floating point math, or fixed point math.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:54: *p = r | g << 8u | b <<
16u | 255u << 24u;
I and someone looking over my shoulder both are unsure off the top of our heads
if the precedence of this is correct, so parens please.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/video_converter.h (right):

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.h:45: std::unique_ptr<uint32_t[]>
colorspace_table_;
I don't see why this has to be a separate allocation. You don't (yet) abstract
it in such a way that it could be shared between multiple VideoConverters.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/video_packet_layout.cc (right):

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_packet_layout.cc:131: // The extra line of
UV being allocated is because h264 chroma MC overreads
Perhaps have a short version of this comment in one of .cc/.h referencing a full
comment in the other. I spent a bit wondering why in the other file.

[kulakowski, 2016-06-17 21:20:16]

On 2016/06/17 21:15:40, kulakowski wrote:
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> File mojo/services/media/common/cpp/video_converter.cc (right):
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> mojo/services/media/common/cpp/video_converter.cc:19: if (f < 0.0f) {
> I'm paranoid having once seen someone spend days debugging such a function. I
> believe that you correctly handle the undefined behavior via this clamping.
> Double checking that it also appropriately handles -0. / denormals / NaNs.
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> mojo/services/media/common/cpp/video_converter.cc:30: size_t
> ColorspaceTableOffset(uint8_t y, uint8_t u, uint8_t v) {
> Why do this math yourself and not write [y][u][v]?
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> mojo/services/media/common/cpp/video_converter.cc:41: for (float y = 0.0f; y <
> 256.0f; y += 1.0f) {
> I hope you typed these constants correctly :)
> 
> I believe that the for loop bounds are correct at floats this size, but man,
> it's a little spooky seeing loop bounds being float comparisons.
> 
> I'm also curious if the preferred specification of this color table is in
terms
> of real math, floating point math, or fixed point math.
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> mojo/services/media/common/cpp/video_converter.cc:54: *p = r | g << 8u | b <<
> 16u | 255u << 24u;
> I and someone looking over my shoulder both are unsure off the top of our
heads
> if the precedence of this is correct, so parens please.
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> File mojo/services/media/common/cpp/video_converter.h (right):
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> mojo/services/media/common/cpp/video_converter.h:45:
std::unique_ptr<uint32_t[]>
> colorspace_table_;
> I don't see why this has to be a separate allocation. You don't (yet) abstract
> it in such a way that it could be shared between multiple VideoConverters.
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> File mojo/services/media/common/cpp/video_packet_layout.cc (right):
> 
>
https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
> mojo/services/media/common/cpp/video_packet_layout.cc:131: // The extra line
of
> UV being allocated is because h264 chroma MC overreads
> Perhaps have a short version of this comment in one of .cc/.h referencing a
full
> comment in the other. I spent a bit wondering why in the other file.

I think a response to some of my comments could be a more explicit delineation
of what code here is "temporary" and will be done in hardware later.

[dalesat, 2016-06-18 00:07:09]

PTAL

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/video_converter.cc (right):

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:19: if (f < 0.0f) {
On 2016/06/17 21:15:40, kulakowski wrote:
> I'm paranoid having once seen someone spend days debugging such a function. I
> believe that you correctly handle the undefined behavior via this clamping.
> Double checking that it also appropriately handles -0. / denormals / NaNs.

This isn't terribly critical, being temporary code. Also, the consequences of a
bug here is that, on rare occasions, the wrong color pixel will be shown.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:30: size_t
ColorspaceTableOffset(uint8_t y, uint8_t u, uint8_t v) {
On 2016/06/17 21:15:40, kulakowski wrote:
> Why do this math yourself and not write [y][u][v]?

I'm just not familiar with using arrays of arrays of arrays. Isn't that an array
of pointers to arrays of pointers to arrays? Is there a unique_ptr that'll clean
that up properly? Given that this code is temporary, this felt less likely to
misbehave.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:41: for (float y = 0.0f; y <
256.0f; y += 1.0f) {
On 2016/06/17 21:15:40, kulakowski wrote:
> I hope you typed these constants correctly :)
> 
> I believe that the for loop bounds are correct at floats this size, but man,
> it's a little spooky seeing loop bounds being float comparisons.
> 
> I'm also curious if the preferred specification of this color table is in
terms
> of real math, floating point math, or fixed point math.

Doing the iteration with ints rather than floats makes sense.

I copied the comments from an article written by an amateur. This isn't intended
to be definitive. I don't think we'll be building a 64MB table for this in
production.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_converter.cc:54: *p = r | g << 8u | b <<
16u | 255u << 24u;
On 2016/06/17 21:15:40, kulakowski wrote:
> I and someone looking over my shoulder both are unsure off the top of our
heads
> if the precedence of this is correct, so parens please.

Done.

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
File mojo/services/media/common/cpp/video_packet_layout.cc (right):

https://codereview.chromium.org/2068043005/diff/20001/mojo/services/media/com...
mojo/services/media/common/cpp/video_packet_layout.cc:131: // The extra line of
UV being allocated is because h264 chroma MC overreads
On 2016/06/17 21:15:40, kulakowski wrote:
> Perhaps have a short version of this comment in one of .cc/.h referencing a
full
> comment in the other. I spent a bit wondering why in the other file.

Done.

[kulakowski, 2016-06-20 17:19:36]

lgtm

[dalesat, 2016-06-20 17:43:58]

Description was changed from

==========
Motown: Video renderer library code
We don't currently have a good way to get video frames into Mozart under
the covers, so an application that wants to render video essentially
needs to *be* a video renderer. This CL adds library code that makes
that easy. This should go away once Motown and Mozart have come to
terms. There are three new classes:
1) VideoRenderer, which implements MediaRenderer, MediaConsumer,
   MediaTimelineControlSite and TimelineConsumer and coughs up RGBA
   video with pretty good synchronization.
2) VideoConverter, which converts YV12 payloads produced by Motown into
   interleaved RGBA. This should be done by the GPU one day. It involves
   a 64MB colorspace conversion table that's built at runtime. Yay!
   It's hacky but temporary.
3) VideoPacketLayout, which understands how the 'planar' data in video
   packet payloads is laid out. This is largely lifted from
   services/media/framework/types/video_stream_type.*. The code
   duplication should go away once the conversion is done on the GPU.
==========

to

==========
Motown: Video renderer library code
We don't currently have a good way to get video frames into Mozart under
the covers, so an application that wants to render video essentially
needs to *be* a video renderer. This CL adds library code that makes
that easy. This should go away once Motown and Mozart have come to
terms. There are three new classes:
1) VideoRenderer, which implements MediaRenderer, MediaConsumer,
   MediaTimelineControlSite and TimelineConsumer and coughs up RGBA
   video with pretty good synchronization.
2) VideoConverter, which converts YV12 payloads produced by Motown into
   interleaved RGBA. This should be done by the GPU one day. It involves
   a 64MB colorspace conversion table that's built at runtime. Yay!
   It's hacky but temporary.
3) VideoPacketLayout, which understands how the 'planar' data in video
   packet payloads is laid out. This is largely lifted from
   services/media/framework/types/video_stream_type.*. The code
   duplication should go away once the conversion is done on the GPU.

R=kulakowski@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/97eb9c38342b0646097e30f07dd...
==========

[dalesat, 2016-06-20 17:43:59]

Committed patchset #3 (id:40001) manually as
97eb9c38342b0646097e30f07dd7309452dca363 (presubmit successful).