media: Add support for playback of VP8 Alpha video streams

media: Add support for playback for VP8 Alpha video streams.

BUG=147355
TEST=VP8 Alpha video streams play

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

[vignesh, 2013-02-19 18:32:50]

Addressing frank's comments (from here:
https://chromiumcodereview.appspot.com/12157002/#ps3001 ).

Other minor fixes.

[scherkus (not reviewing), 2013-02-22 23:18:01]

apologies for slacking on this CR :(

hclam: can you look over the YUVA CSC code? If you're busy let me know and
perhaps we can find someone else.

acolwell: I'd like you to review this CL as well. It touches a lot of stuff and
I'd appreciate your $0.02 (or more, if you've got it)

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer....
media/base/decoder_buffer.cc:99: return side_data_.get();
add DCHECK(!IsEndOfStream());

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer....
media/base/decoder_buffer.cc:103: return side_data_size_;
add DCHECK(!IsEndOfStream());

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer....
media/base/decoder_buffer.h:80: virtual void SetSideData(const uint8* data, int
size);
I've been trying to move away from these data setter functions.

Can you refactor this to intoduce an overloaded CopyFrom() function that accepts
the side data?

https://codereview.chromium.org/12263013/diff/5001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/12263013/diff/5001/media/base/video_frame.h#n...
media/base/video_frame.h:47: YV12A = 13,// 20bpp YUVA planar 1x1 Y, 2x2 VU, 1x1
A samples
nit: two spaces before // comment begin

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:31: #include
"third_party/ffmpeg/libavcodec/avcodec.h"
this include should go inside ffmpeg_common.h

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:77: 
remove extra blank line

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:95: // Get the alpha data if any
nit: I'm sure side data is used for more than just alpha -- I'd rephrase this to
talk about side data and include an example of VP8 alpha

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:211:
if(IsEitherYV12OrYV12AOrYV16(video_frame->format())) {
add space between if and (

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:236: switch(video_frame->format()) {
add space between switch and (

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:250: case media::VideoFrame::YV12A:
nit: add blank line before case

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:264: case
media::VideoFrame::NATIVE_TEXTURE:
nit: add blank line before case

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:268: default:
nit: add blank line before default

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:104: format_ = config.format();
it looks like the only reason for storing format_ is to check against YV12A in
other functions

in the interest of avoiding redundant member variables, could we check for
whether vpx_codec_alpha_ is non-null instead of having format_?

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:109: can_handle = true;
de-indent by 2

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:116: return false;
de-indent by 2

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:139: if(format_ == VideoFrame::YV12A) {
add space between if and (

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:336: if(format_ == VideoFrame::YV12A) {
add space between if and (

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:337: 
nit: remove extra blank line

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:343: for (size_t i = 1; i < 8; ++i) {
what's all this stuff doing?

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:349: buffer->GetSideData() + 8,
what's the magic +8 for?

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:420: if(format_ == VideoFrame::YV12A) {
add space between if and (

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:421: if(vpx_image_alpha) {
are we guaranteed to always produce an alpha image when decoding VP8A content?

i.e., do we need the opaque-setting code below?

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:427: /* there is no alpha. set it to opaque
*/
use // for comments

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:431: malloc(alpha_plane_size));
this seems a bit silly: we're allocating a frame, clearing it to opaque, then
copying that data

perhaps we should add a Clear[YUVA]Plane() function?

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
File media/filters/vpx_video_decoder.h (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.h:81: vpx_codec_ctx* vpx_codec_;
hmm.. it looks like both of these can use scoped_ptr<T>

[vignesh, 2013-02-25 21:51:42]

Have added a patch set that addresses scherkus@'s comments on patch set 2. There
are a few inline comments that need resolution, kindly look into it.

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer....
media/base/decoder_buffer.cc:99: return side_data_.get();
On 2013/02/22 23:18:01, scherkus wrote:
> add DCHECK(!IsEndOfStream());

Done.

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer....
media/base/decoder_buffer.cc:103: return side_data_size_;
On 2013/02/22 23:18:01, scherkus wrote:
> add DCHECK(!IsEndOfStream());

Done.

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/5001/media/base/decoder_buffer....
media/base/decoder_buffer.h:80: virtual void SetSideData(const uint8* data, int
size);
On 2013/02/22 23:18:01, scherkus wrote:
> I've been trying to move away from these data setter functions.
> 
> Can you refactor this to intoduce an overloaded CopyFrom() function that
accepts
> the side data?

Done.

https://codereview.chromium.org/12263013/diff/5001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/12263013/diff/5001/media/base/video_frame.h#n...
media/base/video_frame.h:47: YV12A = 13,// 20bpp YUVA planar 1x1 Y, 2x2 VU, 1x1
A samples
On 2013/02/22 23:18:01, scherkus wrote:
> nit: two spaces before // comment begin

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:31: #include
"third_party/ffmpeg/libavcodec/avcodec.h"
On 2013/02/22 23:18:01, scherkus wrote:
> this include should go inside ffmpeg_common.h

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:77: 
On 2013/02/22 23:18:01, scherkus wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:95: // Get the alpha data if any
On 2013/02/22 23:18:01, scherkus wrote:
> nit: I'm sure side data is used for more than just alpha -- I'd rephrase this
to
> talk about side data and include an example of VP8 alpha

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:211:
if(IsEitherYV12OrYV12AOrYV16(video_frame->format())) {
On 2013/02/22 23:18:01, scherkus wrote:
> add space between if and (

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:236: switch(video_frame->format()) {
On 2013/02/22 23:18:01, scherkus wrote:
> add space between switch and (

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:250: case media::VideoFrame::YV12A:
On 2013/02/22 23:18:01, scherkus wrote:
> nit: add blank line before case

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:264: case
media::VideoFrame::NATIVE_TEXTURE:
On 2013/02/22 23:18:01, scherkus wrote:
> nit: add blank line before case

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer.cc:268: default:
On 2013/02/22 23:18:01, scherkus wrote:
> nit: add blank line before default

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:104: format_ = config.format();
On 2013/02/22 23:18:01, scherkus wrote:
> it looks like the only reason for storing format_ is to check against YV12A in
> other functions
> 
> in the interest of avoiding redundant member variables, could we check for
> whether vpx_codec_alpha_ is non-null instead of having format_?

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:109: can_handle = true;
On 2013/02/22 23:18:01, scherkus wrote:
> de-indent by 2

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:116: return false;
On 2013/02/22 23:18:01, scherkus wrote:
> de-indent by 2

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:139: if(format_ == VideoFrame::YV12A) {
On 2013/02/22 23:18:01, scherkus wrote:
> add space between if and (

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:336: if(format_ == VideoFrame::YV12A) {
On 2013/02/22 23:18:01, scherkus wrote:
> add space between if and (

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:337: 
On 2013/02/22 23:18:01, scherkus wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:343: for (size_t i = 1; i < 8; ++i) {
On 2013/02/22 23:18:01, scherkus wrote:
> what's all this stuff doing?

first 8 bytes of side_data is side_data_id in big endian format. this code
merely reads it. have added a comment.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:349: buffer->GetSideData() + 8,
On 2013/02/22 23:18:01, scherkus wrote:
> what's the magic +8 for?

first 8 bytes of side_data is side_data_id and the rest of it is actual data.
hence the +8.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:420: if(format_ == VideoFrame::YV12A) {
On 2013/02/22 23:18:01, scherkus wrote:
> add space between if and (

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:421: if(vpx_image_alpha) {
On 2013/02/22 23:18:01, scherkus wrote:
> are we guaranteed to always produce an alpha image when decoding VP8A content?
> 
> i.e., do we need the opaque-setting code below?

yes. because there could be video files with no alpha on few frames and with
alpha on few others. so in general, if we detect presence of alpha and if there
is no alpha associated with a single frame, we set it to be opaque.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:427: /* there is no alpha. set it to opaque
*/
On 2013/02/22 23:18:01, scherkus wrote:
> use // for comments

Done.

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.cc:431: malloc(alpha_plane_size));
On 2013/02/22 23:18:01, scherkus wrote:
> this seems a bit silly: we're allocating a frame, clearing it to opaque, then
> copying that data
> 
> perhaps we should add a Clear[YUVA]Plane() function?

Adding a ClearAPlane function. Although, the notion of clearing A plane could be
confusing as it sets all the values to 255 and not 0 (255 = opaque). So perhaps
ClearAPlane isn't such a good name. Thoughts?

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
File media/filters/vpx_video_decoder.h (right):

https://codereview.chromium.org/12263013/diff/5001/media/filters/vpx_video_de...
media/filters/vpx_video_decoder.h:81: vpx_codec_ctx* vpx_codec_;
On 2013/02/22 23:18:01, scherkus wrote:
> hmm.. it looks like both of these can use scoped_ptr<T>

To do this, the libvpx includes should be moved from to this file from the .cc
file, thereby exposing the libvpx files in the global namespace (this is needed
because the exact type is required at compile time for template expansion). Will
this be fine?

[Tom Finegan, 2013-02-25 22:56:05]

These are all nits, imo.

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:75: side_data_size));
Align side_data_size with data; will side_data and side_data_size fit on the
same line? Do that if they fit. :)

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer...
media/base/decoder_buffer.h:55: int side_data_size);
Align per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer...
media/base/decoder_buffer.h:94: int side_data_size);
data and size on same line, side_data and side_data_size on the same line.

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
File media/base/simd/convert_yuv_to_rgb_c.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
media/base/simd/convert_yuv_to_rgb_c.cc:73: const int kFractionMask = ((1 <<
kFractionBits) - 1);
Where are these used?

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
File media/base/simd/convert_yuv_to_rgb_x86.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
media/base/simd/convert_yuv_to_rgb_x86.cc:56: for (int y = 0; y < height; y++) {
Pre-increment (consistent with other places in this file).

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.cc...
media/base/video_frame.cc:178: // following alpha plane that is aligned and
sized in the same manner as the
How about: 

YV12A is the same as YV12, but with an additional alpha plane that has the same
size and alignment as the Y plane.

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.cc...
media/base/video_frame.cc:250: plane == kAPlane;
Fix indent.

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.h#...
media/base/video_frame.h:47: YV12A = 13,  // 20bpp YUVA planar 1x1 Y, 2x2 VU,
1x1 A samples
Missing period.

https://codereview.chromium.org/12263013/diff/18001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/video_util.h#n...
media/base/video_util.h:35: MEDIA_EXPORT void ClearAPlane(int stride, int rows,
VideoFrame* frame);
Hmm... 

s/ClearAPlane/x/ ?

Where x is:

OpaqueAPlane
MakeOpaqueAPlane

Per the description, "Clear" seems like the wrong name. I prefer the
MakeOpaqueAPlane (very readable re intent at the call site).

https://codereview.chromium.org/12263013/diff/18001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/ffmpeg/ffmpeg_commo...
media/ffmpeg/ffmpeg_common.h:32: #include <libavcodec/avcodec.h>
You already have this on line 24, remove.

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:73: uint8_t* side_data;
uint8

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:103: if (side_data_size) {
side_data_size > 0 ?

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:105: side_data_size);
It would be nice if side_data and side_data_size were on the same line.

https://codereview.chromium.org/12263013/diff/18001/media/filters/skcanvas_vi...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/filters/skcanvas_vi...
media/filters/skcanvas_video_renderer.cc:263: media::YV12);
Do we need a comment here about why this is YV12? I know I had to look to
explain it to myself...

https://codereview.chromium.org/12263013/diff/18001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.h:40: 
This and the vpx includes above are why there was no scoper for vpx_codec_ctx in
the initial CL... not sure about this one, but seems a bit nasty to pull libvpx
includes in at this scope. Maybe there's a cleaner way to do this, and this is
my ignorance showing... ?

[vignesh, 2013-02-25 23:33:14]

Adding a patchset to fix nits pointed out by tomfinegan@.

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:75: side_data_size));
On 2013/02/25 22:56:06, Tom Finegan wrote:
> Align side_data_size with data; will side_data and side_data_size fit on the
> same line? Do that if they fit. :)

Done.

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer...
media/base/decoder_buffer.h:55: int side_data_size);
On 2013/02/25 22:56:06, Tom Finegan wrote:
> Align per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

deceived by eclipse as it bolds the type. :)

Done.

https://codereview.chromium.org/12263013/diff/18001/media/base/decoder_buffer...
media/base/decoder_buffer.h:94: int side_data_size);
On 2013/02/25 22:56:06, Tom Finegan wrote:
> data and size on same line, side_data and side_data_size on the same line.

Done.

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
File media/base/simd/convert_yuv_to_rgb_c.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
media/base/simd/convert_yuv_to_rgb_c.cc:73: const int kFractionMask = ((1 <<
kFractionBits) - 1);
On 2013/02/25 22:56:06, Tom Finegan wrote:
> Where are these used?

they aren't used. some experimental code that stuck around by mistake. removed.

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
File media/base/simd/convert_yuv_to_rgb_x86.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/simd/convert_y...
media/base/simd/convert_yuv_to_rgb_x86.cc:56: for (int y = 0; y < height; y++) {
On 2013/02/25 22:56:06, Tom Finegan wrote:
> Pre-increment (consistent with other places in this file).

Done.

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.cc...
media/base/video_frame.cc:178: // following alpha plane that is aligned and
sized in the same manner as the
On 2013/02/25 22:56:06, Tom Finegan wrote:
> How about: 
> 
> YV12A is the same as YV12, but with an additional alpha plane that has the
same
> size and alignment as the Y plane.

Done.

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.cc...
media/base/video_frame.cc:250: plane == kAPlane;
On 2013/02/25 22:56:06, Tom Finegan wrote:
> Fix indent.

Done.

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/video_frame.h#...
media/base/video_frame.h:47: YV12A = 13,  // 20bpp YUVA planar 1x1 Y, 2x2 VU,
1x1 A samples
On 2013/02/25 22:56:06, Tom Finegan wrote:
> Missing period.

Done.

https://codereview.chromium.org/12263013/diff/18001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/base/video_util.h#n...
media/base/video_util.h:35: MEDIA_EXPORT void ClearAPlane(int stride, int rows,
VideoFrame* frame);
On 2013/02/25 22:56:06, Tom Finegan wrote:
> Hmm... 
> 
> s/ClearAPlane/x/ ?
> 
> Where x is:
> 
> OpaqueAPlane
> MakeOpaqueAPlane
> 
> Per the description, "Clear" seems like the wrong name. I prefer the
> MakeOpaqueAPlane (very readable re intent at the call site).

I prefer MakeOpaqueAPlane as it seems consistent with the rest of the file
(starting with a verb, ending with APlane).

https://codereview.chromium.org/12263013/diff/18001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

https://codereview.chromium.org/12263013/diff/18001/media/ffmpeg/ffmpeg_commo...
media/ffmpeg/ffmpeg_common.h:32: #include <libavcodec/avcodec.h>
On 2013/02/25 22:56:06, Tom Finegan wrote:
> You already have this on line 24, remove.

Done.

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:73: uint8_t* side_data;
On 2013/02/25 22:56:06, Tom Finegan wrote:
> uint8

Done.

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:103: if (side_data_size) {
On 2013/02/25 22:56:06, Tom Finegan wrote:
> side_data_size > 0 ?

Done.

https://codereview.chromium.org/12263013/diff/18001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:105: side_data_size);
On 2013/02/25 22:56:06, Tom Finegan wrote:
> It would be nice if side_data and side_data_size were on the same line.

Done.

[scherkus (not reviewing), 2013-02-27 07:28:26]

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer...
media/base/decoder_buffer.cc:51: if(side_data_size_ > 0) {
space between if and (

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer...
media/base/decoder_buffer.h:52: // |data| must not be NULL and |size| >= 0.
mention side_data and side_data_size here OR use general verbiage (e.g., "Data
pointers must not be NULL and sizes must be >= 0")

don't have a strong preference here

https://codereview.chromium.org/12263013/diff/17006/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/video_frame.h#...
media/base/video_frame.h:170: bool HasAlpha() const {
this method is a bit deceiving

for example ... if format == NATIVE_TEXTURE, it would *seem* that this would be
an OK method to call when in reality it will hit a NOTREACHED().

where is this method used? I actually couldn't find a caller but I could have
skipped over it

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.cc
File media/base/video_util.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.cc#...
media/base/video_util.cc:68: void MakeOpaqueAPlane(int stride, int rows,
VideoFrame *frame) {
pointers go with types

VideoFrame* frame

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.h#n...
media/base/video_util.h:35: MEDIA_EXPORT void MakeOpaqueAPlane(int stride, int
rows, VideoFrame* frame);
Not to bikeshed too much on the naming ... but "MakeAPlaneOpaque" is more proper

alternatively, "to opacify" is a verb to make something opaque:
http://wordnetweb.princeton.edu/perl/webwn?s=opacify

... but I just discovered that and I don't think most people will understand it
:)

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:75: AVPacket* pkt;
nit: this is C++! we can move these closer to where we use them ;)

(i.e., lines 93-95)

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:93: pkt = packet.get();
nit: can we inline the packet.get() calls instead of storing it as a local
variable?

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:96: AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL,
is it safe to call this method with this enum for non-matroska formats? (e.g.,
mp3, mp4, avi, wav, ogg)

what happens when we do?

https://codereview.chromium.org/12263013/diff/17006/media/filters/skcanvas_vi...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/filters/skcanvas_vi...
media/filters/skcanvas_video_renderer.cc:82: media::YUVType yuv_type =
video_frame->format() == media::VideoFrame::YV12 ||
is this change required?

it seems like assigning media::YV12 to |yuv_type| is handled by the condition on
lines 86-90

https://codereview.chromium.org/12263013/diff/17006/media/filters/skcanvas_vi...
media/filters/skcanvas_video_renderer.cc:238: case media::VideoFrame::YV16:
I'm not a fan of duplicating the format checks and having uninitialized
frame_clip_{yuv} variables -- if this code shuffles around it'll be easy to
mismatch the conditions and introduce bugs / crashes


I think this code is pretty old and can be simplified. It seems to go out of its
way to avoid duplicating calls to ConvertYUVToRGB32() when in fact I think
duplicating those function calls would be clearer. For example, can we replace
the frame_clip_{yuv} values with calls to video_frame->data(...)?

Can you try converting to a switch w/ each format having its own call to
ConvertYUVToRGB32?

https://codereview.chromium.org/12263013/diff/17006/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:392: if(vpx_image_alpha) {
add space between if and (

[vignesh, 2013-03-28 21:45:12]

Doing a rebase. Also addressed the comments on patchset 4. 

Sorry about doing these two together, I somehow screwed up my old tree and had
to do a manual rebase. So the diffs from previous patchset may be a little
screwed up. Could not help it.

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer...
media/base/decoder_buffer.cc:51: if(side_data_size_ > 0) {
On 2013/02/27 07:28:26, scherkus wrote:
> space between if and (

Done.

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/decoder_buffer...
media/base/decoder_buffer.h:52: // |data| must not be NULL and |size| >= 0.
On 2013/02/27 07:28:26, scherkus wrote:
> mention side_data and side_data_size here OR use general verbiage (e.g., "Data
> pointers must not be NULL and sizes must be >= 0")
> 
> don't have a strong preference here

Done.

https://codereview.chromium.org/12263013/diff/17006/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/video_frame.h#...
media/base/video_frame.h:170: bool HasAlpha() const {
On 2013/02/27 07:28:26, scherkus wrote:
> this method is a bit deceiving
> 
> for example ... if format == NATIVE_TEXTURE, it would *seem* that this would
be
> an OK method to call when in reality it will hit a NOTREACHED().
> 
> where is this method used? I actually couldn't find a caller but I could have
> skipped over it

sorry, this was a method  i used while debugging. it's not used anywhere now.
removing.

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.cc
File media/base/video_util.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.cc#...
media/base/video_util.cc:68: void MakeOpaqueAPlane(int stride, int rows,
VideoFrame *frame) {
On 2013/02/27 07:28:26, scherkus wrote:
> pointers go with types
> 
> VideoFrame* frame

Done.

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/17006/media/base/video_util.h#n...
media/base/video_util.h:35: MEDIA_EXPORT void MakeOpaqueAPlane(int stride, int
rows, VideoFrame* frame);
On 2013/02/27 07:28:26, scherkus wrote:
> Not to bikeshed too much on the naming ... but "MakeAPlaneOpaque" is more
proper
> 
> alternatively, "to opacify" is a verb to make something opaque:
> http://wordnetweb.princeton.edu/perl/webwn?s=opacify
> 
> ... but I just discovered that and I don't think most people will understand
it
> :)

I went with naming it like other functions in this file (ending with "APlane").
Anyway, MakeOpaqueAPlane seems much better. Have changed it. :)

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:75: AVPacket* pkt;
On 2013/02/27 07:28:26, scherkus wrote:
> nit: this is C++! we can move these closer to where we use them ;)
> 
> (i.e., lines 93-95)

yeah, i was writing this along side when i made the ffmpeg change. hence the C
mindset :)

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:93: pkt = packet.get();
On 2013/02/27 07:28:26, scherkus wrote:
> nit: can we inline the packet.get() calls instead of storing it as a local
> variable?

Done.

https://codereview.chromium.org/12263013/diff/17006/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:96: AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL,
On 2013/02/27 07:28:26, scherkus wrote:
> is it safe to call this method with this enum for non-matroska formats? (e.g.,
> mp3, mp4, avi, wav, ogg)
> 
> what happens when we do?

we are requesting a very specific type of side data (which won't be present in
non-matroska formats of course). so, the method will return a NULL, setting
side_data_size to 0. So it is safe.

https://codereview.chromium.org/12263013/diff/17006/media/filters/skcanvas_vi...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/filters/skcanvas_vi...
media/filters/skcanvas_video_renderer.cc:82: media::YUVType yuv_type =
video_frame->format() == media::VideoFrame::YV12 ||
On 2013/02/27 07:28:26, scherkus wrote:
> is this change required?
> 
> it seems like assigning media::YV12 to |yuv_type| is handled by the condition
on
> lines 86-90

Done.

https://codereview.chromium.org/12263013/diff/17006/media/filters/skcanvas_vi...
media/filters/skcanvas_video_renderer.cc:238: case media::VideoFrame::YV16:
On 2013/02/27 07:28:26, scherkus wrote:
> I'm not a fan of duplicating the format checks and having uninitialized
> frame_clip_{yuv} variables -- if this code shuffles around it'll be easy to
> mismatch the conditions and introduce bugs / crashes
> 
> 
> I think this code is pretty old and can be simplified. It seems to go out of
its
> way to avoid duplicating calls to ConvertYUVToRGB32() when in fact I think
> duplicating those function calls would be clearer. For example, can we replace
> the frame_clip_{yuv} values with calls to video_frame->data(...)?
> 
> Can you try converting to a switch w/ each format having its own call to
> ConvertYUVToRGB32?

Done.

https://codereview.chromium.org/12263013/diff/17006/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/17006/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:392: if(vpx_image_alpha) {
On 2013/02/27 07:28:26, scherkus wrote:
> add space between if and (

Done.

[scherkus (not reviewing), 2013-04-04 00:36:17]

getting there!

https://codereview.chromium.org/12263013/diff/33001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/33001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:151: << " encrypted: " << (decrypt_config_ !=
NULL);
emit side data size

https://codereview.chromium.org/12263013/diff/33001/media/base/simd/convert_y...
File media/base/simd/convert_yuva_to_argb_mmx.inc (right):

https://codereview.chromium.org/12263013/diff/33001/media/base/simd/convert_y...
media/base/simd/convert_yuva_to_argb_mmx.inc:1: ; Copyright (c) 2011 The
Chromium Authors. All rights reserved.
FYI I have no idea if this code is correct

it looks like it was mostly copied + changed to support alpha. Is that correct?

https://codereview.chromium.org/12263013/diff/33001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/33001/media/base/video_util.h#n...
media/base/video_util.h:34: // Sets A Plane values to be completely opaque (all
255's)
nit: add blank line before // comments

nit: "A Plane" -> "alpha plane"

https://codereview.chromium.org/12263013/diff/33001/media/base/video_util.h#n...
media/base/video_util.h:36: // |plane| is one of VideoFrame::kYPlane,
VideoFrame::kUPlane,
nit: add blank line before //

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:106: vpx_codec_dec_cfg_t vpx_config = {0};
seems like this code is duplicated

how about refactoring it into a tiny static helper method?

i.e., 

scoped_ptr<vpx_codec_ctx, VpxDeleter> InitializeVpxContext( ... ) {
  // code
  return context.Pass();
}

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:111: vpx_codec_err_t status =
vpx_codec_dec_init(vpx_codec_.get(),
nit: AFAIK you don't need all the .get()s for scoped_ptr<>

mind removing? (note: I could be wrong here)

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:309: // First 8 bytes of side data is
side_data_id in big endian.
nit: blank lines before //

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:313: side_data_id |= side_data_id_buf[i];
this assumes side data is at least 8 bytes

you'll need to add some checks

not sure but you may find base/sys_byteorder.h might haev some useful stuff

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:391: if (vpx_codec_alpha_.get()) {
nit: I like to have early-returns when possible

if (!vpx_codec_alpha_)
  return;
if (!vpx_image_alpha) {
  MakeOpaqueAPlane(...);
  return;
}
CopyAPlane(...);

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.h (right):

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.h:83: void CopyVpxImageTo(const struct
vpx_image* vpx_image,
nit: do we need to have "struct" keyword here?

pretty sure C++ doesn't require it

[vignesh, 2013-04-04 18:17:51]

Have addressed the comments on patch set 5.

https://codereview.chromium.org/12263013/diff/33001/media/base/decoder_buffer.cc
File media/base/decoder_buffer.cc (right):

https://codereview.chromium.org/12263013/diff/33001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:151: << " encrypted: " << (decrypt_config_ !=
NULL);
On 2013/04/04 00:36:17, scherkus wrote:
> emit side data size

Done.

https://codereview.chromium.org/12263013/diff/33001/media/base/simd/convert_y...
File media/base/simd/convert_yuva_to_argb_mmx.inc (right):

https://codereview.chromium.org/12263013/diff/33001/media/base/simd/convert_y...
media/base/simd/convert_yuva_to_argb_mmx.inc:1: ; Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2013/04/04 00:36:17, scherkus wrote:
> FYI I have no idea if this code is correct
> 
> it looks like it was mostly copied + changed to support alpha. Is that
correct?

yes, that is correct. Although, I have to admit that I did not write this code.
Most of it was written by jfaust@ (who used to work on this project before he
left Google). I just made it work along with our chromium setup by looking at
the equivalent YUV to RGB conversion code.

https://codereview.chromium.org/12263013/diff/33001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/33001/media/base/video_util.h#n...
media/base/video_util.h:34: // Sets A Plane values to be completely opaque (all
255's)
On 2013/04/04 00:36:17, scherkus wrote:
> nit: add blank line before // comments
> 
> nit: "A Plane" -> "alpha plane"

Done.

https://codereview.chromium.org/12263013/diff/33001/media/base/video_util.h#n...
media/base/video_util.h:36: // |plane| is one of VideoFrame::kYPlane,
VideoFrame::kUPlane,
On 2013/04/04 00:36:17, scherkus wrote:
> nit: add blank line before // 

Done.

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:106: vpx_codec_dec_cfg_t vpx_config = {0};
On 2013/04/04 00:36:17, scherkus wrote:
> seems like this code is duplicated
> 
> how about refactoring it into a tiny static helper method?
> 
> i.e., 
> 
> scoped_ptr<vpx_codec_ctx, VpxDeleter> InitializeVpxContext( ... ) {
>   // code
>   return context.Pass();
> }

Done.

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:111: vpx_codec_err_t status =
vpx_codec_dec_init(vpx_codec_.get(),
On 2013/04/04 00:36:17, scherkus wrote:
> nit: AFAIK you don't need all the .get()s for scoped_ptr<>
> 
> mind removing? (note: I could be wrong here)

Nope. As it turns out, the .get()s are required. :)

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:309: // First 8 bytes of side data is
side_data_id in big endian.
On 2013/04/04 00:36:17, scherkus wrote:
> nit: blank lines before //

Done.

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:313: side_data_id |= side_data_id_buf[i];
On 2013/04/04 00:36:17, scherkus wrote:
> this assumes side data is at least 8 bytes
> 
> you'll need to add some checks

not sure what checks you want me to add, could you please elaborate?

> 
> not sure but you may find base/sys_byteorder.h might haev some useful stuff

couldn't find anything useful there.

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:391: if (vpx_codec_alpha_.get()) {
On 2013/04/04 00:36:17, scherkus wrote:
> nit: I like to have early-returns when possible
> 
> if (!vpx_codec_alpha_)
>   return;
> if (!vpx_image_alpha) {
>   MakeOpaqueAPlane(...);
>   return;
> }
> CopyAPlane(...);

Done.

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.h (right):

https://codereview.chromium.org/12263013/diff/33001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.h:83: void CopyVpxImageTo(const struct
vpx_image* vpx_image,
On 2013/04/04 00:36:17, scherkus wrote:
> nit: do we need to have "struct" keyword here?
> 
> pretty sure C++ doesn't require it

It doesn't compile without the "struct" keyword. Probably because they are
structures that come from a C include file inside of libvpx.

[Tom Finegan, 2013-04-04 19:40:44]

Two comments:

1) (nit) Update the CL description. My preference would be:

media: Add support for playback for VP8 Alpha video streams.

BUG=147355
TEST=VP8 Alpha video streams play

2) Does the cc/ file change belong in this CL? I don't think there are any cc/
OWNERS reviewing this CL. Rebase into the cc/ CL?

[vignesh, 2013-04-04 20:04:07]

On 2013/04/04 19:40:44, Tom Finegan wrote:
> Two comments:
> 
> 1) (nit) Update the CL description. My preference would be:
> 
> media: Add support for playback for VP8 Alpha video streams.
> 
> BUG=147355
> TEST=VP8 Alpha video streams play

Done.

> 
> 2) Does the cc/ file change belong in this CL? I don't think there are any cc/
> OWNERS reviewing this CL. Rebase into the cc/ CL?

No, this particular cc/ file is dependent on a change in media/ files. That's
the reason i'm having this file alone in this CL.

The plan is to land the cc/ files CL (https://codereview.chromium.org/12157002/)
first. It will add functionality that is self sufficient (i.e.) will build but
will never be used until this CL lands. This CL is dependent on that one landing
first.

[Tom Finegan, 2013-04-04 22:20:45]

On 2013/04/04 20:04:07, vignesh wrote:
> On 2013/04/04 19:40:44, Tom Finegan wrote:
> > Two comments:
> > 
> > 1) (nit) Update the CL description. My preference would be:
> > 
> > media: Add support for playback for VP8 Alpha video streams.
> > 

s/playback for/playback of/

Yeah, I suck. Sorry!

> > BUG=147355
> > TEST=VP8 Alpha video streams play
> 
> Done.
> 
> > 
> > 2) Does the cc/ file change belong in this CL? I don't think there are any
cc/
> > OWNERS reviewing this CL. Rebase into the cc/ CL?
> 
> No, this particular cc/ file is dependent on a change in media/ files. That's
> the reason i'm having this file alone in this CL.
> 
> The plan is to land the cc/ files CL
(https://codereview.chromium.org/12157002/)
> first. It will add functionality that is self sufficient (i.e.) will build but
> will never be used until this CL lands. This CL is dependent on that one
landing
> first.

[scherkus (not reviewing), 2013-04-04 22:54:00]

https://codereview.chromium.org/12263013/diff/41001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/41001/media/base/decoder_buffer...
media/base/decoder_buffer.h:53: static scoped_refptr<DecoderBuffer>
CopyFrom(const uint8* data, int size,
can you add unit tests?

https://codereview.chromium.org/12263013/diff/41001/media/base/decoder_buffer...
media/base/decoder_buffer.h:92: DecoderBuffer(const uint8* data, int size);
given these are private constructors, I would remove this one and have existing
clients call DecodeBuffer() w/ NULL/0 side data

no need to duplicate

https://codereview.chromium.org/12263013/diff/41001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/41001/media/base/video_util.h#n...
media/base/video_util.h:35: // Sets alpha plane values to be completely opaque
(all 255's)
nit: comments end w/ periods

https://codereview.chromium.org/12263013/diff/41001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/41001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:115: uint8* side_data;
nit: I'd declare + assign on same line

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:84: scoped_ptr<vpx_codec_ctx, VpxDeleter>
InitializeVpxContext(
should be static

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:87: vpx_codec_err_t* status) {
how about LOG(ERROR) the status code inside this function and return a NULL
vpx_codec_ctx to signal error?

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:310: // First 8 bytes of side data is
side_data_id in big endian.
I think you can use base/sys_byteorder.h

uint64 side_data_id =
base::NetToHost64(*reinterpret_cast<uint64>(buffer->GetSideData()));

again, this assumes that buffer->GetSideDataSize() >= 8

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:311: uint64_t side_data_id =
side_data_id_buf[0];
nit: drop the _t suffix -- chromium code uses uint64

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:314: side_data_id |= side_data_id_buf[i];
right now the code always assumes GetData() is non-NULL and GetSideData() is at
least 8 bytes when vpx_codec_alpha_ is non-NULL

you should add a check that the data is at least 8 bytes large before proceeding
with alpha decoding

[vignesh, 2013-04-05 19:06:58]

Addressing scherkus@'s comments on patchset 6.

https://codereview.chromium.org/12263013/diff/41001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/12263013/diff/41001/media/base/decoder_buffer...
media/base/decoder_buffer.h:53: static scoped_refptr<DecoderBuffer>
CopyFrom(const uint8* data, int size,
On 2013/04/04 22:54:00, scherkus wrote:
> can you add unit tests?

Done.

https://codereview.chromium.org/12263013/diff/41001/media/base/decoder_buffer...
media/base/decoder_buffer.h:92: DecoderBuffer(const uint8* data, int size);
On 2013/04/04 22:54:00, scherkus wrote:
> given these are private constructors, I would remove this one and have
existing
> clients call DecodeBuffer() w/ NULL/0 side data

This is a protected constructor, not private. I see atleast one subclass
(stream_parser_buffer.cc) that calls this constructor. So, I think it's better
off to leave it as it is, rather than going around and changing the subclasses
as well. Thoughts?

> 
> no need to duplicate

https://codereview.chromium.org/12263013/diff/41001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/12263013/diff/41001/media/base/video_util.h#n...
media/base/video_util.h:35: // Sets alpha plane values to be completely opaque
(all 255's)
On 2013/04/04 22:54:00, scherkus wrote:
> nit: comments end w/ periods

Done.

https://codereview.chromium.org/12263013/diff/41001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/41001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:115: uint8* side_data;
On 2013/04/04 22:54:00, scherkus wrote:
> nit: I'd declare + assign on same line

Done.

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:84: scoped_ptr<vpx_codec_ctx, VpxDeleter>
InitializeVpxContext(
On 2013/04/04 22:54:00, scherkus wrote:
> should be static

Done.

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:87: vpx_codec_err_t* status) {
On 2013/04/04 22:54:00, scherkus wrote:
> how about LOG(ERROR) the status code inside this function and return a NULL
> vpx_codec_ctx to signal error?

Done.

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:310: // First 8 bytes of side data is
side_data_id in big endian.
On 2013/04/04 22:54:00, scherkus wrote:
> I think you can use base/sys_byteorder.h
> 
> uint64 side_data_id =
> base::NetToHost64(*reinterpret_cast<uint64>(buffer->GetSideData()));
> 
> again, this assumes that buffer->GetSideDataSize() >= 8

Thanks :) Done.

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:311: uint64_t side_data_id =
side_data_id_buf[0];
On 2013/04/04 22:54:00, scherkus wrote:
> nit: drop the _t suffix -- chromium code uses uint64

Done.

https://codereview.chromium.org/12263013/diff/41001/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:314: side_data_id |= side_data_id_buf[i];
On 2013/04/04 22:54:00, scherkus wrote:
> right now the code always assumes GetData() is non-NULL and GetSideData() is
at
> least 8 bytes when vpx_codec_alpha_ is non-NULL
> 
> you should add a check that the data is at least 8 bytes large before
proceeding
> with alpha decoding

Done.

[scherkus (not reviewing), 2013-04-05 19:21:55]

few last nits

https://codereview.chromium.org/12263013/diff/57002/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/57002/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:118: packet.get(),
indent here is off

these args should be 4-space indent

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:102: context.reset(NULL);
reset() works too (no need for NULL)

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:129: if (vpx_codec_ == NULL)
nit: prefer boolean checks instead

if (!vpx_codec.get())
  return false;

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:134: if (vpx_codec_alpha_ == NULL)
ditto

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:303: CHECK_GT(buffer->GetSideDataSize(), 8);
this will crash the process

are you sure we don't want to handle this gracefully?

do you know if it's easy/trivial to construct a webm/mkv file that has side data
with <8 bytes of data to hit this line of code?

[vignesh, 2013-04-05 21:46:16]

https://codereview.chromium.org/12263013/diff/57002/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/57002/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:118: packet.get(),
On 2013/04/05 19:21:56, scherkus wrote:
> indent here is off
> 
> these args should be 4-space indent

Done.

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:102: context.reset(NULL);
On 2013/04/05 19:21:56, scherkus wrote:
> reset() works too (no need for NULL)

Done.

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:129: if (vpx_codec_ == NULL)
On 2013/04/05 19:21:56, scherkus wrote:
> nit: prefer boolean checks instead
> 
> if (!vpx_codec.get())
>   return false;

Done.

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:134: if (vpx_codec_alpha_ == NULL)
On 2013/04/05 19:21:56, scherkus wrote:
> ditto

Done.

https://codereview.chromium.org/12263013/diff/57002/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:303: CHECK_GT(buffer->GetSideDataSize(), 8);
On 2013/04/05 19:21:56, scherkus wrote:
> this will crash the process
> 
> are you sure we don't want to handle this gracefully?
> 
> do you know if it's easy/trivial to construct a webm/mkv file that has side
data
> with <8 bytes of data to hit this line of code?

i did this already and it did crash the process (i thought that was the expected
behavior since the video file is invalid). to handle this gracefully, i have
made the alpha plane opaque in this case. is this fine? or do you have some
other suggestion?

[scherkus (not reviewing), 2013-04-08 14:21:00]

can we get some pipeline integration tests included that cover VP8A playback?

check out media/filters/pipeline_integration_test.cc -- tomfinegan@ should be
able to help out

https://codereview.chromium.org/12263013/diff/68004/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/68004/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:302: if (vpx_codec_alpha_.get() &&
buffer->GetSideDataSize() > 8) {
nit: >= 8? is passing in a 0-sized buffer acceptable?

[vignesh, 2013-04-09 23:25:25]

Added the pipeline integration test for VP8A playback. Will upload the data file
in a separate CL.

https://codereview.chromium.org/12263013/diff/68004/media/filters/vpx_video_d...
File media/filters/vpx_video_decoder.cc (right):

https://codereview.chromium.org/12263013/diff/68004/media/filters/vpx_video_d...
media/filters/vpx_video_decoder.cc:302: if (vpx_codec_alpha_.get() &&
buffer->GetSideDataSize() > 8) {
On 2013/04/08 14:21:00, scherkus wrote:
> nit: >= 8? is passing in a 0-sized buffer acceptable?

yes, passing in a zero sized buffer will hit the condition on line 317.

Done.

[scherkus (not reviewing), 2013-04-10 00:22:24]

lgtm w/ nits

feel free to get someone to land that binary file for you so you can make use of
the try bots and CQ for this patch

thanks vignesh!

https://codereview.chromium.org/12263013/diff/77001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/77001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:98: 
remove extra blank line

https://codereview.chromium.org/12263013/diff/77001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/12263013/diff/77001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:901: // Verify that VP8 video with
alpha channel can be played back
comments end with periods

https://codereview.chromium.org/12263013/diff/77001/webkit/media/filter_helpe...
File webkit/media/filter_helpers.cc (right):

https://codereview.chromium.org/12263013/diff/77001/webkit/media/filter_helpe...
webkit/media/filter_helpers.cc:53:
filter_collection->GetVideoDecoders()->push_back(ffmpeg_video_decoder);
FYI this means that we'll no longer use FFmpeg's VP8 decoder when one of those
two flags are enabled.

Not something you need to worry about for now ... but one day :)

[vignesh, 2013-04-10 17:59:16]

https://codereview.chromium.org/12263013/diff/77001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/12263013/diff/77001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.cc:98: 
On 2013/04/10 00:22:24, scherkus wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/12263013/diff/77001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/12263013/diff/77001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:901: // Verify that VP8 video with
alpha channel can be played back
On 2013/04/10 00:22:24, scherkus wrote:
> comments end with periods

Done.

https://codereview.chromium.org/12263013/diff/77001/webkit/media/filter_helpe...
File webkit/media/filter_helpers.cc (right):

https://codereview.chromium.org/12263013/diff/77001/webkit/media/filter_helpe...
webkit/media/filter_helpers.cc:53:
filter_collection->GetVideoDecoders()->push_back(ffmpeg_video_decoder);
On 2013/04/10 00:22:24, scherkus wrote:
> FYI this means that we'll no longer use FFmpeg's VP8 decoder when one of those
> two flags are enabled.
> 
> Not something you need to worry about for now ... but one day :)

That's not really true, there are checks in vpx_video_decoder.cc (lines 115-126)
to make sure that it handles the right format of videos when the right flags are
passed. For instance, a VP8 video without alpha will always use FFmpeg's VP8
decoder.

[vignesh, 2013-04-12 20:54:39]

+brettw 

Brett, could you please do an owner's review of the non-media/ changes? Thanks.

[vignesh, 2013-04-12 21:02:01]

+ccameron

could you please do an owner's review of the cc/ file change? thanks.

[ccameron, 2013-04-12 21:03:49]

lgtm

[vignesh, 2013-04-16 21:01:12]

+sky

could you please do an owner's review of stuff in content/browser/*. Thanks.

[sky, 2013-04-16 21:42:28]

content/browser LGTM

[Tom Finegan, 2013-04-16 22:20:51]

Committed patchset #15 manually as r194465 (presubmit successful).