Adding VP8 Alpha support in Media Source

Adding VP8 Alpha support in Media Source

Adding support for playback of VP8 videos with Alpha Channel through the
Media Source API.

BUG=242357
TEST=VP8 Alpha Streams play properly via Media Source

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

[vignesh, 2013-05-20 23:28:42]

Initial working version of VP8 Alpha playback in Media Source. Please go ahead
and do a review. I will be adding unittests and pipeline integration tests.
shortly.

[fgalligan1, 2013-05-21 00:02:43]

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

https://codereview.chromium.org/15342004/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:41: if (side_data_size > 0)
nit: Check side_data_size_

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:255: block_additional_data_.reset(new
uint8[size + 8]);
Make this 8 a constant.

[acolwell GONE FROM CHROMIUM, 2013-05-21 17:14:11]

Looks pretty good. Just a few comments.

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

https://codereview.chromium.org/15342004/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:36: return;
Should there be a CHECK(!side_data) here? I'm assuming side_data only buffers
are not allowed.

https://codereview.chromium.org/15342004/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:42: memcpy(side_data_.get(), side_data,
side_data_size_);
Why is this change needed? Are you seeing negative values passed in? If you are
worried about this, just put DCHECK_GE(side_data_size, 0); at the top of the
method.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:226: uint64 block_add_id =
base::HostToNet64(block_add_id_);
Please move this into the case block so that it is closer to the code that
actually uses it.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:255: block_additional_data_.reset(new
uint8[size + 8]);
On 2013/05/21 00:02:43, fgalligan1 wrote:
> Make this 8 a constant.
The constant should probably be initialized with sizeof(block_add_id) instead of
8.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:258: block_additional_data_size_ = size + 8;
nit: move this above the block_additional_data_ allocation so that it can be
used in that statement.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_constants.h
File media/webm/webm_constants.h (right):

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_constants.h#n...
media/webm/webm_constants.h:159: const int kWebMIdAlphaMode = 0x53C0;
nit: Please place this in alphabetical order.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_tracks_parser.cc
File media/webm/webm_tracks_parser.cc (right):

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_tracks_parser...
media/webm/webm_tracks_parser.cc:49: max_block_additional_id_ = -1;
nit: Remove since this doesn't appear to be used anywhere.

[vignesh, 2013-05-21 18:57:56]

have addressed all the comments and have added the pipeline integration test.

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

https://codereview.chromium.org/15342004/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:36: return;
On 2013/05/21 17:14:11, acolwell wrote:
> Should there be a CHECK(!side_data) here? I'm assuming side_data only buffers
> are not allowed.

Done.

https://codereview.chromium.org/15342004/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:41: if (side_data_size > 0)
On 2013/05/21 00:02:43, fgalligan1 wrote:
> nit: Check side_data_size_

Done.

https://codereview.chromium.org/15342004/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:42: memcpy(side_data_.get(), side_data,
side_data_size_);
On 2013/05/21 17:14:11, acolwell wrote:
> Why is this change needed? Are you seeing negative values passed in? If you
are
> worried about this, just put DCHECK_GE(side_data_size, 0); at the top of the
> method.

I made this change because i replaced the private constructor in
StreamParserBuffer to take side_data and side_data_size by default (the callers
will pass NULL if there is no side_data). So StreamParserBuffer always calls the
4 arg constructor of DecoderBuffer which gives rise to a possibility of
side_data_size being -1. This possibility didn't exist before. 

In fact, if StreamParserBuffer is the only class that inherits from
DecoderBuffer, we can actually go ahead and remove the 2 arg constructor (the
one without side_data) of DecoderBuffer and make the callers pass NULL if there
is no side_data there as well.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:226: uint64 block_add_id =
base::HostToNet64(block_add_id_);
On 2013/05/21 17:14:11, acolwell wrote:
> Please move this into the case block so that it is closer to the code that
> actually uses it.

that's how i had it initially, but the compiler wasn't happy about declaring
variables within case statements (jump to case labels, crosses initialization
error). is there a workaround for this?

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:255: block_additional_data_.reset(new
uint8[size + 8]);
On 2013/05/21 17:14:11, acolwell wrote:
> On 2013/05/21 00:02:43, fgalligan1 wrote:
> > Make this 8 a constant.
> The constant should probably be initialized with sizeof(block_add_id) instead
of
> 8.

Done.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_cluster_parse...
media/webm/webm_cluster_parser.cc:258: block_additional_data_size_ = size + 8;
On 2013/05/21 17:14:11, acolwell wrote:
> nit: move this above the block_additional_data_ allocation so that it can be
> used in that statement.

Done.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_constants.h
File media/webm/webm_constants.h (right):

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_constants.h#n...
media/webm/webm_constants.h:159: const int kWebMIdAlphaMode = 0x53C0;
On 2013/05/21 17:14:11, acolwell wrote:
> nit: Please place this in alphabetical order.

Done.

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_tracks_parser.cc
File media/webm/webm_tracks_parser.cc (right):

https://codereview.chromium.org/15342004/diff/1/media/webm/webm_tracks_parser...
media/webm/webm_tracks_parser.cc:49: max_block_additional_id_ = -1;
On 2013/05/21 17:14:11, acolwell wrote:
> nit: Remove since this doesn't appear to be used anywhere.

Done.

[acolwell GONE FROM CHROMIUM, 2013-05-21 19:47:48]

LGTM

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

https://codereview.chromium.org/15342004/diff/14001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:42: if (side_data_size_ > 0)
nit: Per offline discussion. s/side_data_size_/side_data/ since that is the real
reason we can't call memcpy.

https://codereview.chromium.org/15342004/diff/14001/media/webm/webm_tracks_pa...
File media/webm/webm_tracks_parser.h (right):

https://codereview.chromium.org/15342004/diff/14001/media/webm/webm_tracks_pa...
media/webm/webm_tracks_parser.h:83: int64 max_block_additional_id_;
remove this since it doesn't appear to be used anywhere.

[vignesh, 2013-05-21 21:05:21]

have addressed the comments, PTAL. If it's okay, i'll try to land this once the
test file lands.

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

https://codereview.chromium.org/15342004/diff/14001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:42: if (side_data_size_ > 0)
On 2013/05/21 19:47:48, acolwell wrote:
> nit: Per offline discussion. s/side_data_size_/side_data/ since that is the
real
> reason we can't call memcpy.

Done.

https://codereview.chromium.org/15342004/diff/14001/media/webm/webm_tracks_pa...
File media/webm/webm_tracks_parser.h (right):

https://codereview.chromium.org/15342004/diff/14001/media/webm/webm_tracks_pa...
media/webm/webm_tracks_parser.h:83: int64 max_block_additional_id_;
On 2013/05/21 19:47:48, acolwell wrote:
> remove this since it doesn't appear to be used anywhere.

Done.

[acolwell GONE FROM CHROMIUM, 2013-05-21 21:22:00]

lgtm

[commit-bot: I haz the power, 2013-05-21 21:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vigneshv@chromium.org/15342004/26001

[commit-bot: I haz the power, 2013-05-22 12:43:23]

Change committed as 201496