Fix support for yuv_422 pixel format.

Fix support for yuv_422 pixel format.

Added pix_fmt field to the VideoDecoderConfig class. The pixel format is passed to the
codec_context_ and used to correctly initialize VideoFrames.

BUG=95642
TEST=

[shadi, 2011-09-27 18:52:46]

Hi Andrew,

Here is the CL for fixing the yuv422 (YV16) color format bug. 

Can you please review?

--Shadi

[scherkus (not reviewing), 2011-09-28 17:32:17]

looking pretty good!

http://codereview.chromium.org/8052002/diff/2002/media/base/video_decoder_con...
File media/base/video_decoder_config.h (right):

http://codereview.chromium.org/8052002/diff/2002/media/base/video_decoder_con...
media/base/video_decoder_config.h:10: #include "libavutil/pixfmt.h"
in order to prevent FFmpeg types leaking into the rest of chrome code, we try to
limit its exposure

In this case, instead of using FFmpeg's PixelFormat we should be using
VideoFrame::Format and convert between the two using utility functions inside
media/ffmpeg/ffmpeg_common.*

It seems a bit silly to have to convert between the two, but it really helps to
keep modules isolated

http://codereview.chromium.org/8052002/diff/2002/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8052002/diff/2002/media/base/video_frame.cc#ne...
media/base/video_frame.cc:121: size_t alloc_height = RoundUp(height_, 2);
want to fix this up to be "y_height" and use rows(VideoFrame::kYPlane) ?

http://codereview.chromium.org/8052002/diff/2002/media/base/video_frame.cc#ne...
media/base/video_frame.cc:122: size_t y_bytes_per_row = RoundUp(width_, 4);
ditto and maybe rename y_stride to be consistent with other variable names

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:225: // TODO(shadi): Should YV12 be the default?
nope I'd rather return INVALID

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:226: NOTREACHED();
could you add an error message to this:

NOTREACHED() << "Unsupported PixelFormat: " << pix_fmt;

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:84: // Converts FFmpeg's pixel formats to the
corresponding supported video format.
nit: add one blank line before this (in general comments have a blank line
before them)

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:85: VideoFrame::Format
PixFmtToVideoFormat(PixelFormat pix_fmt);
nit: PixFmt -> PixelFormat

http://codereview.chromium.org/8052002/diff/2002/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8052002/diff/2002/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:61: // (shadi): yes! YV16 and Y12 have
different memory format.
haha!

if these TODOs are sufficiently addressed then we can remove them

http://codereview.chromium.org/8052002/diff/2002/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:254: int uv_rows =
video_frame->rows(VideoFrame::kUPlane);
nice :)

[scherkus (not reviewing), 2011-09-28 17:33:59]

also... how are we testing this?

we can either rely on a layout test getting checked into WebKit, or we can test
at a unit level by adding a test to
media/video/ffmpeg_video_decode_engine_unittest.cc

[shadi1, 2011-09-29 18:31:03]

Hi Andrew,

I updated the comments you sent me. Can you please take a look? 

Thanks,
Shadi

http://codereview.chromium.org/8052002/diff/2002/media/base/video_decoder_con...
File media/base/video_decoder_config.h (right):

http://codereview.chromium.org/8052002/diff/2002/media/base/video_decoder_con...
media/base/video_decoder_config.h:10: #include "libavutil/pixfmt.h"
On 2011/09/28 17:32:17, scherkus wrote:
> in order to prevent FFmpeg types leaking into the rest of chrome code, we try
to
> limit its exposure
> 
> In this case, instead of using FFmpeg's PixelFormat we should be using
> VideoFrame::Format and convert between the two using utility functions inside
> media/ffmpeg/ffmpeg_common.*
> 
> It seems a bit silly to have to convert between the two, but it really helps
to
> keep modules isolated

Done.

http://codereview.chromium.org/8052002/diff/2002/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/8052002/diff/2002/media/base/video_frame.cc#ne...
media/base/video_frame.cc:121: size_t alloc_height = RoundUp(height_, 2);
On 2011/09/28 17:32:17, scherkus wrote:
> want to fix this up to be "y_height" and use rows(VideoFrame::kYPlane) ?

Done.

http://codereview.chromium.org/8052002/diff/2002/media/base/video_frame.cc#ne...
media/base/video_frame.cc:122: size_t y_bytes_per_row = RoundUp(width_, 4);
On 2011/09/28 17:32:17, scherkus wrote:
> ditto and maybe rename y_stride to be consistent with other variable names

Done.

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:225: // TODO(shadi): Should YV12 be the default?
On 2011/09/28 17:32:17, scherkus wrote:
> nope I'd rather return INVALID

Done.

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:226: NOTREACHED();
On 2011/09/28 17:32:17, scherkus wrote:
> could you add an error message to this:
> 
> NOTREACHED() << "Unsupported PixelFormat: " << pix_fmt;

Done.

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:84: // Converts FFmpeg's pixel formats to the
corresponding supported video format.
On 2011/09/28 17:32:17, scherkus wrote:
> nit: add one blank line before this (in general comments have a blank line
> before them)

Done.

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:84: // Converts FFmpeg's pixel formats to the
corresponding supported video format.
On 2011/09/28 17:32:17, scherkus wrote:
> nit: add one blank line before this (in general comments have a blank line
> before them)

Done.

http://codereview.chromium.org/8052002/diff/2002/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:85: VideoFrame::Format
PixFmtToVideoFormat(PixelFormat pix_fmt);
On 2011/09/28 17:32:17, scherkus wrote:
> nit: PixFmt -> PixelFormat

Done.

http://codereview.chromium.org/8052002/diff/2002/media/video/ffmpeg_video_dec...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/8052002/diff/2002/media/video/ffmpeg_video_dec...
media/video/ffmpeg_video_decode_engine.cc:61: // (shadi): yes! YV16 and Y12 have
different memory format.
On 2011/09/28 17:32:17, scherkus wrote:
> haha!
> 
> if these TODOs are sufficiently addressed then we can remove them

Done.

[scherkus (not reviewing), 2011-09-29 20:48:05]

LGTM w/ one indentation nit

also -- what's our testing plan? layout tests?

http://codereview.chromium.org/8052002/diff/14001/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/8052002/diff/14001/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:232: case VideoFrame::YV16:
fix indentation

[shadi1, 2011-09-30 00:29:35]

I submitted two layout tests, one for yuv422 and another for yuv420.

On Thu, Sep 29, 2011 at 1:48 PM, <scherkus@chromium.org> wrote:

> LGTM w/ one indentation nit
>
> also -- what's our testing plan? layout tests?
>
>
> http://codereview.chromium.**org/8052002/diff/14001/media/**
>
ffmpeg/ffmpeg_common.cc<http://codereview.chromium.org/8052002/diff/14001/media/ffmpeg/ffmpeg_common.cc>
>
> File media/ffmpeg/ffmpeg_common.cc (right):
>
> http://codereview.chromium.**org/8052002/diff/14001/media/**
>
ffmpeg/ffmpeg_common.cc#**newcode232<http://codereview.chromium.org/8052002/diff/14001/media/ffmpeg/ffmpeg_common.cc#newcode232>
> media/ffmpeg/ffmpeg_common.cc:**232: case VideoFrame::YV16:
> fix indentation
>
>
>
http://codereview.chromium.**org/8052002/<http://codereview.chromium.org/8052...
>

[scherkus (not reviewing), 2011-10-04 19:30:35]

Committed as r103961.

[fbarchard, 2011-10-05 01:38:46]

http://codereview.chromium.org/8052002/diff/18001/media/base/video_frame.cc
File media/base/video_frame.cc (left):

http://codereview.chromium.org/8052002/diff/18001/media/base/video_frame.cc#o...
media/base/video_frame.cc:132: strides_[VideoFrame::kYPlane] = y_bytes_per_row;
FYI I'd suggest using 'stride' terminology consistently, which we still do for
uv_stride.  stride indicates that it is how much we advance after each row,
which can be negative, and/or allows cropping.