media: Opus support for WebM in Media Source

media: Opus support for WebM in Media Source

Matroska's specification of Opus has been standardized here:
http://wiki.xiph.org/MatroskaOpus. This CL adds support for the
new Matroska elements related to Opus and enables Opus playback
in WebM files through Media Source API. It also adds support for
end trimming.

This is a first CL in a sequence of CLs that will attempt to add
various features towards fully functional working of Opus in WebM
(both media source and video tag).

BUG=

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

[vignesh, 2013-08-26 19:09:51]

I am working on coming up with tests for end trimming. In the meantime, this CL
is ready for first round of reviews. I'll add more people from the video stack
team as reviewers after we do an in-team review first.

[fgalligan1, 2013-08-26 21:10:06]

https://codereview.chromium.org/23014009/diff/1/media/base/audio_decoder_conf...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/1/media/base/audio_decoder_conf...
media/base/audio_decoder_config.h:112: int64 codec_delay_;
I think you need to add comments about what these members are, what are their
units,...

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

https://codereview.chromium.org/23014009/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:85: << " encrypted: " << (decrypt_config_ != NULL);
Add discard padding.

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

https://codereview.chromium.org/23014009/diff/1/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:108: const int64 discard_padding() const {
First const is not necessary.

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:460: skip_samples_ =
opus_header.skip_samples;
Is this the same value as below? I.e. skipped_bytes = opus_header.skip_samples *
bytesPerSample * numChannels?

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:561:
output_buffer->get()->TrimStart(dropped_size);
I'm assuming this works on samples? Does it take into account numChannels?

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

https://codereview.chromium.org/23014009/diff/1/media/webm/webm_audio_client....
media/webm/webm_audio_client.cc:73: config->set_seek_pre_roll(seek_pre_roll);
All of the other vales are passed in Initialize. Any reason these are not passed
in like the other values?

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

https://codereview.chromium.org/23014009/diff/1/media/webm/webm_parser.cc#new...
media/webm/webm_parser.cc:122: {UINT, kWebMIdDiscardPadding},
DiscardPadding should come after CodecState.

https://codereview.chromium.org/23014009/diff/1/media/webm/webm_parser.cc#new...
media/webm/webm_parser.cc:164: {UINT, kWebMIdSeekPreRoll},
These should be switched according to the spec layout.
http://matroska.org/technical/specs/index.html

Also CodecDelay should come after TrackOverlay.

[vignesh, 2013-08-26 21:44:23]

addressing comments.

https://codereview.chromium.org/23014009/diff/1/media/base/audio_decoder_conf...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/1/media/base/audio_decoder_conf...
media/base/audio_decoder_config.h:112: int64 codec_delay_;
On 2013/08/26 21:10:06, fgalligan1 wrote:
> I think you need to add comments about what these members are, what are their
> units,...

Done.

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

https://codereview.chromium.org/23014009/diff/1/media/base/decoder_buffer.cc#...
media/base/decoder_buffer.cc:85: << " encrypted: " << (decrypt_config_ != NULL);
On 2013/08/26 21:10:06, fgalligan1 wrote:
> Add discard padding.

Done.

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

https://codereview.chromium.org/23014009/diff/1/media/base/decoder_buffer.h#n...
media/base/decoder_buffer.h:108: const int64 discard_padding() const {
On 2013/08/26 21:10:06, fgalligan1 wrote:
> First const is not necessary. 

Done.

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:460: skip_samples_ =
opus_header.skip_samples;
On 2013/08/26 21:10:06, fgalligan1 wrote:
> Is this the same value as below? I.e. skipped_bytes = opus_header.skip_samples
*
> bytesPerSample * numChannels?

No it is not. If you look below (Line 542 in the red diff), i have modified the
Start Trimming code to make use of AudioBuffer.TrimStart() rather than having
our own implementation of it. The TrimStart function expects number of samples
and not number of bytes. So that conversion is not necessary anymore.

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:561:
output_buffer->get()->TrimStart(dropped_size);
On 2013/08/26 21:10:06, fgalligan1 wrote:
> I'm assuming this works on samples? Does it take into account numChannels?

Yes, it works on samples. I am unsure if it takes numChannels into account. Will
get back on this shortly.

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

https://codereview.chromium.org/23014009/diff/1/media/webm/webm_audio_client....
media/webm/webm_audio_client.cc:73: config->set_seek_pre_roll(seek_pre_roll);
On 2013/08/26 21:10:06, fgalligan1 wrote:
> All of the other vales are passed in Initialize. Any reason these are not
passed
> in like the other values?

Initialize is a generic function which is used at various parts of the codebase.
So i didn't want to change all of them by changing Initialize's signature. If
you think that's the right way to do it, then i can change all the calls to
Initialize with 0 as codec_delay.

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

https://codereview.chromium.org/23014009/diff/1/media/webm/webm_parser.cc#new...
media/webm/webm_parser.cc:122: {UINT, kWebMIdDiscardPadding},
On 2013/08/26 21:10:06, fgalligan1 wrote:
> DiscardPadding should come after CodecState.

Done.

https://codereview.chromium.org/23014009/diff/1/media/webm/webm_parser.cc#new...
media/webm/webm_parser.cc:164: {UINT, kWebMIdSeekPreRoll},
On 2013/08/26 21:10:06, fgalligan1 wrote:
> These should be switched according to the spec layout.
> http://matroska.org/technical/specs/index.html
> 
> Also CodecDelay should come after TrackOverlay. 

Done.

[fgalligan1, 2013-08-26 22:11:53]

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:460: skip_samples_ =
opus_header.skip_samples;
On 2013/08/26 21:44:23, vignesh wrote:
> On 2013/08/26 21:10:06, fgalligan1 wrote:
> > Is this the same value as below? I.e. skipped_bytes =
opus_header.skip_samples
> *
> > bytesPerSample * numChannels?
> 
> No it is not. If you look below (Line 542 in the red diff), i have modified
the
> Start Trimming code to make use of AudioBuffer.TrimStart() rather than having
> our own implementation of it. The TrimStart function expects number of samples
> and not number of bytes. So that conversion is not necessary anymore.

Sorry I was just asking if opus_header.skip_samples included numChannels or not

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

https://codereview.chromium.org/23014009/diff/1/media/webm/webm_audio_client....
media/webm/webm_audio_client.cc:73: config->set_seek_pre_roll(seek_pre_roll);
On 2013/08/26 21:44:23, vignesh wrote:
> On 2013/08/26 21:10:06, fgalligan1 wrote:
> > All of the other vales are passed in Initialize. Any reason these are not
> passed
> > in like the other values?
> 
> Initialize is a generic function which is used at various parts of the
codebase.
> So i didn't want to change all of them by changing Initialize's signature. If
> you think that's the right way to do it, then i can change all the calls to
> Initialize with 0 as codec_delay.

It looks like it, but you could wait and see what videostack says.

[vignesh, 2013-08-26 23:55:47]

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:460: skip_samples_ =
opus_header.skip_samples;
On 2013/08/26 22:11:53, fgalligan1 wrote:
> On 2013/08/26 21:44:23, vignesh wrote:
> > On 2013/08/26 21:10:06, fgalligan1 wrote:
> > > Is this the same value as below? I.e. skipped_bytes =
> opus_header.skip_samples
> > *
> > > bytesPerSample * numChannels?
> > 
> > No it is not. If you look below (Line 542 in the red diff), i have modified
> the
> > Start Trimming code to make use of AudioBuffer.TrimStart() rather than
having
> > our own implementation of it. The TrimStart function expects number of
samples
> > and not number of bytes. So that conversion is not necessary anymore.
> 
> Sorry I was just asking if opus_header.skip_samples included numChannels or
not

No it does not. Neither does the skip_samples_ variable in the else block. The
TrimStart and TrimEnd functions in AudioBuffer takes numChannels into account.
So we just call them with the number of samples.

https://codereview.chromium.org/23014009/diff/1/media/filters/opus_audio_deco...
media/filters/opus_audio_decoder.cc:561:
output_buffer->get()->TrimStart(dropped_size);
On 2013/08/26 21:44:23, vignesh wrote:
> On 2013/08/26 21:10:06, fgalligan1 wrote:
> > I'm assuming this works on samples? Does it take into account numChannels?
> 
> Yes, it works on samples. I am unsure if it takes numChannels into account.
Will
> get back on this shortly.

As mentioned in the other comment in this file, this function accoutns for
number of channels.

[Tom Finegan, 2013-08-28 01:35:34]

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:522: int samples_decoded =
opus_multistream_decode(opus_decoder_,
I thought you were going to handle S16 and F32 on a per platform basis-- did you
change your mind and decide to use S16 unconditionally?

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:71: channel_layout,
Ditto on the question re unconditional S16 for Opus.

[vignesh, 2013-08-28 17:17:15]

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:522: int samples_decoded =
opus_multistream_decode(opus_decoder_,
On 2013/08/28 01:35:35, Tom Finegan wrote:
> I thought you were going to handle S16 and F32 on a per platform basis-- did
you
> change your mind and decide to use S16 unconditionally?

I did not change my mind. I will be doing that change in a subsequent CL.

[acolwell GONE FROM CHROMIUM, 2013-08-28 20:44:46]

https://codereview.chromium.org/23014009/diff/16001/media/base/audio_decoder_...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/16001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:83: int64 seek_pre_roll() const { return
seek_pre_roll_; }
nit: Use base::TimeDelta for this and the codec_delay since these fields
represent time. Since Opus only uses 48kHz we should be able to get the rounding
to microseconds to work out ok in all cases.

Another option is to specify these in terms of samples instead so that
conversions don't have to be done in the decoders. If you go the samples round,
use int instead and signal an error if we encounter a value in the file that
would overflow an int.

https://codereview.chromium.org/23014009/diff/16001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:84: void set_seek_pre_roll(int64
seek_pre_roll) {
nit: These should be set in Initialize() and the constructor like all other
values.

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

https://codereview.chromium.org/23014009/diff/16001/media/base/decoder_buffer...
media/base/decoder_buffer.h:108: int64 discard_padding() const {
nit: This should be a TimeDelta as well.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (left):

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:253: output_bytes_to_drop_(0),
nit:It looks like you didn't remove this from the .h.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:460: skip_samples_ =
opus_header.skip_samples;
Why do we special case this?

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:463: skip_samples_ = config.codec_delay() *
config.samples_per_second() /
nit: This looks like it could potentially overflow. At a minimum you should
return false if skip_samples_ becomes negative.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:465: if (skip_samples_ !=
opus_header.skip_samples) {
Shouldn't the codec_delay() header always match the skip_samples field?

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:467: << "header. Using the value in
container.";
nit: return false here if the expectation that these should always match.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:567: 1000000000;
nit: This may need overflow protection as well.

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

https://codereview.chromium.org/23014009/diff/16001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:29: static const char
kAudioOnlyWebM[] = "video/webm; codecs=\"vorbis,opus\"";
nit: Use a separate constant for Opus. This signals that both Vorbis and Opus is
present, which I don't believe is what you intended.

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:36: } else if (codec_id == "A_OPUS" || codec_id
== "A_OPUS/EXPERIMENTAL") {
The spec only specifies A_OPUS. I'd prefer to force people to only use that
codec_id if they want to use their content w/ MediaSource.

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:363: if (discard_padding >= 0) {
nit: {} are unnecessary.

[vignesh, 2013-08-29 21:37:31]

have addressed most comments. PTAL.

https://codereview.chromium.org/23014009/diff/16001/media/base/audio_decoder_...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/16001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:83: int64 seek_pre_roll() const { return
seek_pre_roll_; }
On 2013/08/28 20:44:47, acolwell wrote:
> nit: Use base::TimeDelta for this and the codec_delay since these fields
> represent time. Since Opus only uses 48kHz we should be able to get the
rounding
> to microseconds to work out ok in all cases.
> 
> Another option is to specify these in terms of samples instead so that
> conversions don't have to be done in the decoders. If you go the samples
round,
> use int instead and signal an error if we encounter a value in the file that
> would overflow an int.

Done.

https://codereview.chromium.org/23014009/diff/16001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:84: void set_seek_pre_roll(int64
seek_pre_roll) {
On 2013/08/28 20:44:47, acolwell wrote:
> nit: These should be set in Initialize() and the constructor like all other
> values.

Done.

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

https://codereview.chromium.org/23014009/diff/16001/media/base/decoder_buffer...
media/base/decoder_buffer.h:108: int64 discard_padding() const {
On 2013/08/28 20:44:47, acolwell wrote:
> nit: This should be a TimeDelta as well.

Done.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (left):

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:253: output_bytes_to_drop_(0),
On 2013/08/28 20:44:47, acolwell wrote:
> nit:It looks like you didn't remove this from the .h.

Done.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:460: skip_samples_ =
opus_header.skip_samples;
On 2013/08/28 20:44:47, acolwell wrote:
> Why do we special case this?

In case of some old opus files, the container value might not exist. We can
still play those files by making use of the value from the codec private header.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:463: skip_samples_ = config.codec_delay() *
config.samples_per_second() /
On 2013/08/28 20:44:47, acolwell wrote:
> nit: This looks like it could potentially overflow. At a minimum you should
> return false if skip_samples_ becomes negative.

Done.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:465: if (skip_samples_ !=
opus_header.skip_samples) {
On 2013/08/28 20:44:47, acolwell wrote:
> Shouldn't the codec_delay() header always match the skip_samples field?

As of now, both the fields should match. But some tool might eventually screw it
up. So I thought we could be more permissive and somehow try to play the file.
But after a second thought, seems like being strict in this case makes sense. So
i'll make the change accordingly.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:467: << "header. Using the value in
container.";
On 2013/08/28 20:44:47, acolwell wrote:
> nit: return false here if the expectation that these should always match.

Done.

https://codereview.chromium.org/23014009/diff/16001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:567: 1000000000;
On 2013/08/28 20:44:47, acolwell wrote:
> nit: This may need overflow protection as well.

Done.

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

https://codereview.chromium.org/23014009/diff/16001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:29: static const char
kAudioOnlyWebM[] = "video/webm; codecs=\"vorbis,opus\"";
On 2013/08/28 20:44:47, acolwell wrote:
> nit: Use a separate constant for Opus. This signals that both Vorbis and Opus
is
> present, which I don't believe is what you intended.

Done.

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:36: } else if (codec_id == "A_OPUS" || codec_id
== "A_OPUS/EXPERIMENTAL") {
On 2013/08/28 20:44:47, acolwell wrote:
> The spec only specifies A_OPUS. I'd prefer to force people to only use that
> codec_id if they want to use their content w/ MediaSource.

Right now ffmpeg puts A_OPUS/EXPERIMENTAL. I thought we can allow such files to
playback (for now atleast) and remove this later once Opus becomes stable.
Please let me know if you think otherwise and I can change this.

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:71: channel_layout,
On 2013/08/28 01:35:35, Tom Finegan wrote:
> Ditto on the question re unconditional S16 for Opus.

Done.

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:363: if (discard_padding >= 0) {
On 2013/08/28 20:44:47, acolwell wrote:
> nit: {} are unnecessary.

Done.

[acolwell GONE FROM CHROMIUM, 2013-09-03 20:14:01]

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:36: } else if (codec_id == "A_OPUS" || codec_id
== "A_OPUS/EXPERIMENTAL") {
On 2013/08/29 21:37:31, vignesh wrote:
> On 2013/08/28 20:44:47, acolwell wrote:
> > The spec only specifies A_OPUS. I'd prefer to force people to only use that
> > codec_id if they want to use their content w/ MediaSource.
> 
> Right now ffmpeg puts A_OPUS/EXPERIMENTAL. I thought we can allow such files
to
> playback (for now atleast) and remove this later once Opus becomes stable.
> Please let me know if you think otherwise and I can change this.

I have concerns that a large amount of content will be generated with the
/EXPERIMENTAL suffix and we will never be able to drop it. For example, it would
make me really sad if YouTube reencoded all the content w/ the
A_OPUS/EXPERIMENTAL form. Any idea why FFMPEG isn't committing to just using
A_OPUS at this point?

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.cc:24: seek_pre_roll_(base::TimeDelta()),
nit: not needed here and the line below.

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.cc:91: sample_format_ != kUnknownSampleFormat;
Checks should be added here to make sure seek_preroll_ & codec_delay_ are >=
base::TimeDelta().

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:68: base::TimeDelta seek_pre_roll,
nit: s/seek_pre_roll/seek_preroll/ here and everywhere else.

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:111: // |seek_pre_roll_| is the duration in
nanoseconds of the data that the
nit: s/nanoseconds//

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:115: // |codec_delay_| is the overall delay
overhead in nanoseconds added by the
ditto

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

https://codereview.chromium.org/23014009/diff/30001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:15: discard_padding_(base::TimeDelta()) {
nit: not needed.

https://codereview.chromium.org/23014009/diff/30001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:23: discard_padding_(base::TimeDelta()) {
ditto

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:461: if
(!config.codec_delay().InMicroseconds()) {
Shouldn't this return false to trigger a decode error since it indicates that
there is a delay, but it isn't signalled in the header? It seems like allowing
this would mask invalid content.

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:467: static_cast<double>(1000000));
nit: 1000000.0 should remove the need for a static_cast here.

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:574: static_cast<double>(1000000));
ditto

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:579:
output_buffer->get()->TrimEnd(std::min(samples_decoded, discard_padding));
An error should probably be triggered if the discard_padding in the file is
actually larger than the number of samples in the coded frame.

https://codereview.chromium.org/23014009/diff/30001/media/filters/stream_pars...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/filters/stream_pars...
media/filters/stream_parser_factory.cc:40: HISTOGRAM_OPUS,
You need to update tools/metrics/histograms/histograms.xml as well.

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:73:
base::TimeDelta::FromMicroseconds(seek_pre_roll / 1000),
This code isn't checking for -1. I believe these fields should be set to 0 if
they aren't present in the file.

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.h (right):

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.h:34: const int64 seek_pre_roll,
nit: s/pre_roll/preroll & docs

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:188: dst = &discard_padding_;
Technically DiscardPadding can be negative, but the current parsing code does
not support parsing integers. For now change this code to the following.

if (discard_padding_set_)
    return false;
discard_padding_set_ = true;
discard_padding_ = val;
return true;

And update all discard_padding_ = -1 to also include discard_padding_set_ =
false. 

Also make sure that your decoder logic that drops samples discards the right
samples based on the sign of discard_padding_.

[vignesh, 2013-09-03 22:43:02]

have addressed all the comments. PTAL. thanks.

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/23014009/diff/16001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:36: } else if (codec_id == "A_OPUS" || codec_id
== "A_OPUS/EXPERIMENTAL") {
On 2013/09/03 20:14:01, acolwell wrote:
> On 2013/08/29 21:37:31, vignesh wrote:
> > On 2013/08/28 20:44:47, acolwell wrote:
> > > The spec only specifies A_OPUS. I'd prefer to force people to only use
that
> > > codec_id if they want to use their content w/ MediaSource.
> > 
> > Right now ffmpeg puts A_OPUS/EXPERIMENTAL. I thought we can allow such files
> to
> > playback (for now atleast) and remove this later once Opus becomes stable.
> > Please let me know if you think otherwise and I can change this.
> 
> I have concerns that a large amount of content will be generated with the
> /EXPERIMENTAL suffix and we will never be able to drop it. For example, it
would
> make me really sad if YouTube reencoded all the content w/ the
> A_OPUS/EXPERIMENTAL form. Any idea why FFMPEG isn't committing to just using
> A_OPUS at this point?

I understand your concern. I will remove the Experimental suffix. 

As to why ffmpeg isn't committing to A_OPUS at this point, i am unsure. Anyway,
I am going to try and push ffmpeg to drop the suffix, because now that the
matroska opus spec is finalized, it does not make sense to have the suffix
anymore.

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
File media/base/audio_decoder_config.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.cc:24: seek_pre_roll_(base::TimeDelta()),
On 2013/09/03 20:14:01, acolwell wrote:
> nit: not needed here and the line below.

Done.

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.cc:91: sample_format_ != kUnknownSampleFormat;
On 2013/09/03 20:14:01, acolwell wrote:
> Checks should be added here to make sure seek_preroll_ & codec_delay_ are >=
> base::TimeDelta().

Done.

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:68: base::TimeDelta seek_pre_roll,
On 2013/09/03 20:14:01, acolwell wrote:
> nit: s/seek_pre_roll/seek_preroll/ here and everywhere else.

Done.

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:111: // |seek_pre_roll_| is the duration in
nanoseconds of the data that the
On 2013/09/03 20:14:01, acolwell wrote:
> nit: s/nanoseconds//

Done.

https://codereview.chromium.org/23014009/diff/30001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:115: // |codec_delay_| is the overall delay
overhead in nanoseconds added by the
On 2013/09/03 20:14:01, acolwell wrote:
> ditto

Done.

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

https://codereview.chromium.org/23014009/diff/30001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:15: discard_padding_(base::TimeDelta()) {
On 2013/09/03 20:14:01, acolwell wrote:
> nit: not needed.

Done.

https://codereview.chromium.org/23014009/diff/30001/media/base/decoder_buffer...
media/base/decoder_buffer.cc:23: discard_padding_(base::TimeDelta()) {
On 2013/09/03 20:14:01, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:461: if
(!config.codec_delay().InMicroseconds()) {
On 2013/09/03 20:14:01, acolwell wrote:
> Shouldn't this return false to trigger a decode error since it indicates that
> there is a delay, but it isn't signalled in the header? It seems like allowing
> this would mask invalid content.

Agreed. Done.

But doing this stricter check will fail to play files through the regular media
pipeline (<video> tag) since the ffmpeg demuxer isn't capable of parsing the new
container values (yet!). So i will disable the Basic_Opus_playback pipeline
integration test for now. I will be fixing the ffmpeg demuxer soon and will
enable the test after i do that.

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:467: static_cast<double>(1000000));
On 2013/09/03 20:14:01, acolwell wrote:
> nit: 1000000.0 should remove the need for a static_cast here.

Done.

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:574: static_cast<double>(1000000));
On 2013/09/03 20:14:01, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:579:
output_buffer->get()->TrimEnd(std::min(samples_decoded, discard_padding));
On 2013/09/03 20:14:01, acolwell wrote:
> An error should probably be triggered if the discard_padding in the file is
> actually larger than the number of samples in the coded frame.

Done.

https://codereview.chromium.org/23014009/diff/30001/media/filters/stream_pars...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/filters/stream_pars...
media/filters/stream_parser_factory.cc:40: HISTOGRAM_OPUS,
On 2013/09/03 20:14:01, acolwell wrote:
> You need to update tools/metrics/histograms/histograms.xml as well.

Done.

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.cc:73:
base::TimeDelta::FromMicroseconds(seek_pre_roll / 1000),
On 2013/09/03 20:14:01, acolwell wrote:
> This code isn't checking for -1. I believe these fields should be set to 0 if
> they aren't present in the file.

Done.

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
File media/webm/webm_audio_client.h (right):

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_audio_cli...
media/webm/webm_audio_client.h:34: const int64 seek_pre_roll,
On 2013/09/03 20:14:01, acolwell wrote:
> nit: s/pre_roll/preroll & docs

Done.

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:188: dst = &discard_padding_;
On 2013/09/03 20:14:01, acolwell wrote:
> Technically DiscardPadding can be negative, but the current parsing code does
> not support parsing integers. For now change this code to the following.
> 
> if (discard_padding_set_)
>     return false;
> discard_padding_set_ = true;
> discard_padding_ = val;
> return true;
> 
> And update all discard_padding_ = -1 to also include discard_padding_set_ =
> false. 
> 
> Also make sure that your decoder logic that drops samples discards the right
> samples based on the sign of discard_padding_. 

Done.

[acolwell GONE FROM CHROMIUM, 2013-09-03 23:53:43]

You're real close. Just a few more comments.

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/23014009/diff/30001/media/filters/opus_audio_...
media/filters/opus_audio_decoder.cc:461: if
(!config.codec_delay().InMicroseconds()) {
On 2013/09/03 22:43:03, vignesh wrote:
> On 2013/09/03 20:14:01, acolwell wrote:
> > Shouldn't this return false to trigger a decode error since it indicates
that
> > there is a delay, but it isn't signalled in the header? It seems like
allowing
> > this would mask invalid content.
> 
> Agreed. Done.
> 
> But doing this stricter check will fail to play files through the regular
media
> pipeline (<video> tag) since the ffmpeg demuxer isn't capable of parsing the
new
> container values (yet!). So i will disable the Basic_Opus_playback pipeline
> integration test for now. I will be fixing the ffmpeg demuxer soon and will
> enable the test after i do that.

Hmm. I don't want people who have been experimenting with the existing FFmpeg
code path to file "Opus no worky" bugs when this lands. How about leaving this
in with a TODO to remove it as soon as 
you fix the FFmpeg code path. If there isn't a bug already for that work, please
file one and put the crbug URL in the comment. Also reenable the test please. :)

https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:111: // |seek_preroll_| is the duration in of
the data that the decoder must
nit: s/in//

https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
media/filters/stream_parser_factory.cc:212: if
(!cmd_line->HasSwitch(switches::kEnableEac3Playback))
Since Opus support is still behind a flag, you should have a similar flag check
like this one to avoid claiming we support Opus when the flag is not set. Once
you do this, you'll need to add code to media/base/run_all_unittests.cc to
enable the flag for unit tests, just like we do for Vp8Alpha support.

https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:165: discard_padding_);
nit: You should probably use 0 instead of -1 for discard_padding_'s unset state
so that this function doesn't set the discard_padding when there isn't any.
Otherwise you should add discard_padding_set_ ? discard_padding_ : 0 here.

https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:368: if (discard_padding >= 0) {
nit: s/>=/!=/ so that this code will be ready when negative paddings are
allowed.

[vignesh, 2013-09-04 00:09:52]

https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
media/base/audio_decoder_config.h:111: // |seek_preroll_| is the duration in of
the data that the decoder must
On 2013/09/03 23:53:43, acolwell wrote:
> nit: s/in//

Done.

https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
media/filters/stream_parser_factory.cc:212: if
(!cmd_line->HasSwitch(switches::kEnableEac3Playback))
On 2013/09/03 23:53:43, acolwell wrote:
> Since Opus support is still behind a flag, you should have a similar flag
check
> like this one to avoid claiming we support Opus when the flag is not set. Once
> you do this, you'll need to add code to media/base/run_all_unittests.cc to
> enable the flag for unit tests, just like we do for Vp8Alpha support.

Done. Just so you know, I just got rid of the VP8 Alpha flag a couple hours ago.
:)

https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:165: discard_padding_);
On 2013/09/03 23:53:43, acolwell wrote:
> nit: You should probably use 0 instead of -1 for discard_padding_'s unset
state
> so that this function doesn't set the discard_padding when there isn't any.
> Otherwise you should add discard_padding_set_ ? discard_padding_ : 0 here.

Done.

https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:368: if (discard_padding >= 0) {
On 2013/09/03 23:53:43, acolwell wrote:
> nit: s/>=/!=/ so that this code will be ready when negative paddings are
> allowed.

Done.

[acolwell GONE FROM CHROMIUM, 2013-09-04 00:38:41]

On 2013/09/04 00:09:52, vignesh wrote:
>
https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
> File media/base/audio_decoder_config.h (right):
> 
>
https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
> media/base/audio_decoder_config.h:111: // |seek_preroll_| is the duration in
of
> the data that the decoder must
> On 2013/09/03 23:53:43, acolwell wrote:
> > nit: s/in//
> 
> Done.
> 
>
https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
> File media/filters/stream_parser_factory.cc (right):
> 
>
https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
> media/filters/stream_parser_factory.cc:212: if
> (!cmd_line->HasSwitch(switches::kEnableEac3Playback))
> On 2013/09/03 23:53:43, acolwell wrote:
> > Since Opus support is still behind a flag, you should have a similar flag
> check
> > like this one to avoid claiming we support Opus when the flag is not set.
Once
> > you do this, you'll need to add code to media/base/run_all_unittests.cc to
> > enable the flag for unit tests, just like we do for Vp8Alpha support.
> 
> Done. Just so you know, I just got rid of the VP8 Alpha flag a couple hours
ago.
> :)
> 
>
https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
> File media/webm/webm_cluster_parser.cc (right):
> 
>
https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
> media/webm/webm_cluster_parser.cc:165: discard_padding_);
> On 2013/09/03 23:53:43, acolwell wrote:
> > nit: You should probably use 0 instead of -1 for discard_padding_'s unset
> state
> > so that this function doesn't set the discard_padding when there isn't any.
> > Otherwise you should add discard_padding_set_ ? discard_padding_ : 0 here.
> 
> Done.
> 
>
https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
> media/webm/webm_cluster_parser.cc:368: if (discard_padding >= 0) {
> On 2013/09/03 23:53:43, acolwell wrote:
> > nit: s/>=/!=/ so that this code will be ready when negative paddings are
> > allowed.
> 
> Done.

I think you need to upload again. The patch appears to be messed up.

[vignesh, 2013-09-04 17:07:36]

On 2013/09/04 00:38:41, acolwell wrote:
> On 2013/09/04 00:09:52, vignesh wrote:
> >
>
https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
> > File media/base/audio_decoder_config.h (right):
> > 
> >
>
https://codereview.chromium.org/23014009/diff/38001/media/base/audio_decoder_...
> > media/base/audio_decoder_config.h:111: // |seek_preroll_| is the duration in
> of
> > the data that the decoder must
> > On 2013/09/03 23:53:43, acolwell wrote:
> > > nit: s/in//
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
> > File media/filters/stream_parser_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/23014009/diff/38001/media/filters/stream_pars...
> > media/filters/stream_parser_factory.cc:212: if
> > (!cmd_line->HasSwitch(switches::kEnableEac3Playback))
> > On 2013/09/03 23:53:43, acolwell wrote:
> > > Since Opus support is still behind a flag, you should have a similar flag
> > check
> > > like this one to avoid claiming we support Opus when the flag is not set.
> Once
> > > you do this, you'll need to add code to media/base/run_all_unittests.cc to
> > > enable the flag for unit tests, just like we do for Vp8Alpha support.
> > 
> > Done. Just so you know, I just got rid of the VP8 Alpha flag a couple hours
> ago.
> > :)
> > 
> >
>
https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
> > File media/webm/webm_cluster_parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
> > media/webm/webm_cluster_parser.cc:165: discard_padding_);
> > On 2013/09/03 23:53:43, acolwell wrote:
> > > nit: You should probably use 0 instead of -1 for discard_padding_'s unset
> > state
> > > so that this function doesn't set the discard_padding when there isn't
any.
> > > Otherwise you should add discard_padding_set_ ? discard_padding_ : 0 here.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23014009/diff/38001/media/webm/webm_cluster_p...
> > media/webm/webm_cluster_parser.cc:368: if (discard_padding >= 0) {
> > On 2013/09/03 23:53:43, acolwell wrote:
> > > nit: s/>=/!=/ so that this code will be ready when negative paddings are
> > > allowed.
> > 
> > Done.
> 
> I think you need to upload again. The patch appears to be messed up.

Not sure why that happened. I have uploaded it again and it looks fine now.
Sorry about that.

[acolwell GONE FROM CHROMIUM, 2013-09-04 20:08:42]

lgtm % nits.

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

https://codereview.chromium.org/23014009/diff/58001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:959: TEST_F(PipelineIntegrationTest,
DISABLED_BasicPlayback_AudioOnly_Opus_WebM) {
nit: Please reinstate the necessary code in opus_audio_decoder.cc for this to
pass again. Put a TODO with that code saying to remove it once the FFmpeg code
properly populates the AudioDecoderConfig.

https://codereview.chromium.org/23014009/diff/58001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/58001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:172: discard_padding_ = discard_padding_set_ ?
discard_padding_ : 0;
nit: Sorry if I wasn't clear. I meant to place this in the call to ParseBlock().
I think the code this way will cause discard_padding to be set to -1 for formats
like Vorbis that may have block groups, but don't specify discard_padding.

The alternative is to always set_discard_padding_ = 0 everywhere in this file
that you set it to -1. Then discard_padding_set_ is only used to signal an error
if multiple DiscardPadding elements are encountered.

[vignesh, 2013-09-04 20:27:46]

Fixed all the nits. PTAL before i submit it.

Adding isherman@ for owners review of tools/*. This is a one line change. Please
take a look.

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

https://codereview.chromium.org/23014009/diff/58001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:959: TEST_F(PipelineIntegrationTest,
DISABLED_BasicPlayback_AudioOnly_Opus_WebM) {
On 2013/09/04 20:08:43, acolwell wrote:
> nit: Please reinstate the necessary code in opus_audio_decoder.cc for this to
> pass again. Put a TODO with that code saying to remove it once the FFmpeg code
> properly populates the AudioDecoderConfig.

Done.

https://codereview.chromium.org/23014009/diff/58001/media/webm/webm_cluster_p...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/23014009/diff/58001/media/webm/webm_cluster_p...
media/webm/webm_cluster_parser.cc:172: discard_padding_ = discard_padding_set_ ?
discard_padding_ : 0;
On 2013/09/04 20:08:43, acolwell wrote:
> nit: Sorry if I wasn't clear. I meant to place this in the call to
ParseBlock().
> I think the code this way will cause discard_padding to be set to -1 for
formats
> like Vorbis that may have block groups, but don't specify discard_padding.
> 
> The alternative is to always set_discard_padding_ = 0 everywhere in this file
> that you set it to -1. Then discard_padding_set_ is only used to signal an
error
> if multiple DiscardPadding elements are encountered.

Done.

[Ilya Sherman, 2013-09-04 20:54:42]

histograms.xml lgtm

[commit-bot: I haz the power, 2013-09-05 18:22:12]

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

[commit-bot: I haz the power, 2013-09-05 18:22:31]

Failed to apply patch for media/filters/stream_parser_factory.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/filters/stream_parser_factory.cc
  Hunk #2 FAILED at 63.
  Hunk #3 succeeded at 213 (offset 4 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
media/filters/stream_parser_factory.cc.rej

Patch:       media/filters/stream_parser_factory.cc
Index: media/filters/stream_parser_factory.cc
diff --git a/media/filters/stream_parser_factory.cc
b/media/filters/stream_parser_factory.cc
index
c1587699fef8d9148a94a64d17e8922a46270303..1a34f6a94975a3f94b29cead477d0c1b3c68bdc0
100644
--- a/media/filters/stream_parser_factory.cc
+++ b/media/filters/stream_parser_factory.cc
@@ -37,6 +37,7 @@ struct CodecInfo {
     HISTOGRAM_MPEG2AAC,
     HISTOGRAM_MPEG4AAC,
     HISTOGRAM_EAC3,
+    HISTOGRAM_OPUS,
     HISTOGRAM_MAX  // Must be the last entry.
   };
 
@@ -62,16 +63,20 @@ static const CodecInfo kVP9CodecInfo = { "vp9",
CodecInfo::VIDEO, NULL,
                                          CodecInfo::HISTOGRAM_VP9 };
 static const CodecInfo kVorbisCodecInfo = { "vorbis", CodecInfo::AUDIO, NULL,
                                             CodecInfo::HISTOGRAM_VORBIS };
+static const CodecInfo kOpusCodecInfo = { "opus", CodecInfo::AUDIO, NULL,
+                                          CodecInfo::HISTOGRAM_OPUS };
 
 static const CodecInfo* kVideoWebMCodecs[] = {
   &kVP8CodecInfo,
   &kVP9CodecInfo,
   &kVorbisCodecInfo,
+  &kOpusCodecInfo,
   NULL
 };
 
 static const CodecInfo* kAudioWebMCodecs[] = {
   &kVorbisCodecInfo,
+  &kOpusCodecInfo,
   NULL
 };
 
@@ -208,6 +213,11 @@ static bool VerifyCodec(
           return false;
       }
 #endif
+      if (codec_info->tag == CodecInfo::HISTOGRAM_OPUS) {
+        const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
+        if (!cmd_line->HasSwitch(switches::kEnableOpusPlayback))
+          return false;
+      }
       if (audio_codecs)
         audio_codecs->push_back(codec_info->tag);
       return true;

[commit-bot: I haz the power, 2013-09-05 20:22:57]

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

[commit-bot: I haz the power, 2013-09-05 20:49:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-05 22:20:01]

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

[commit-bot: I haz the power, 2013-09-06 05:09:15]

Change committed as 221574