Add support for setting OPUS header gain.

Add support for setting OPUS header gain.

BUG=178302
TEST=gain is correct in bug test cases.
NOTRY=true

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

[DaleCurtis, 2013-12-16 21:24:29]

Turns out this was pretty easy to do after all.

[acolwell GONE FROM CHROMIUM, 2013-12-16 21:37:07]

lgtm % nits

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

https://codereview.chromium.org/104873011/diff/1/media/filters/opus_audio_dec...
media/filters/opus_audio_decoder.cc:223: ReadLE16<int16_t>(data, data_size,
kOpusExtraDataGainOffset);
nit: How about just putting a static_cast<int16_t>() here instead of making
ReadLE16() a template. It seems like that would result in less code and avoid
the implicit casting that is going on in your template (ie there is no int16_t
version of ByteSwapToLE16).

https://codereview.chromium.org/104873011/diff/1/media/filters/opus_audio_dec...
media/filters/opus_audio_decoder.cc:517: << opus_strerror(status);
nit: Shouldn't we return false here since this is a sign of invalid content?

[DaleCurtis, 2013-12-16 22:21:04]

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

https://codereview.chromium.org/104873011/diff/1/media/filters/opus_audio_dec...
media/filters/opus_audio_decoder.cc:223: ReadLE16<int16_t>(data, data_size,
kOpusExtraDataGainOffset);
On 2013/12/16 21:37:08, acolwell wrote:
> nit: How about just putting a static_cast<int16_t>() here instead of making
> ReadLE16() a template. It seems like that would result in less code and avoid
> the implicit casting that is going on in your template (ie there is no int16_t
> version of ByteSwapToLE16).

Nice catch, I've reverted this in place of just using a static_cast.

https://codereview.chromium.org/104873011/diff/1/media/filters/opus_audio_dec...
media/filters/opus_audio_decoder.cc:517: << opus_strerror(status);
On 2013/12/16 21:37:08, acolwell wrote:
> nit: Shouldn't we return false here since this is a sign of invalid content?

While the docs don't make it clear, errors will be returned when the gain value
is outside of [-32768, 32767].  The test files I have play fine, so I've made it
an error.

[vignesh, 2013-12-16 22:24:51]

lgtm % comment.

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

https://codereview.chromium.org/104873011/diff/1/media/filters/opus_audio_dec...
media/filters/opus_audio_decoder.cc:517: << opus_strerror(status);
On 2013/12/16 22:21:04, DaleCurtis wrote:
> On 2013/12/16 21:37:08, acolwell wrote:
> > nit: Shouldn't we return false here since this is a sign of invalid content?
> 
> While the docs don't make it clear, errors will be returned when the gain
value
> is outside of [-32768, 32767].  The test files I have play fine, so I've made
it
> an error.

We do strict checking on other fields (like codec_delay). So i think we should
be strict here as well and return false.

[DaleCurtis, 2013-12-16 22:28:02]

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

https://codereview.chromium.org/104873011/diff/1/media/filters/opus_audio_dec...
media/filters/opus_audio_decoder.cc:517: << opus_strerror(status);
On 2013/12/16 22:24:51, vignesh wrote:
> On 2013/12/16 22:21:04, DaleCurtis wrote:
> > On 2013/12/16 21:37:08, acolwell wrote:
> > > nit: Shouldn't we return false here since this is a sign of invalid
content?
> > 
> > While the docs don't make it clear, errors will be returned when the gain
> value
> > is outside of [-32768, 32767].  The test files I have play fine, so I've
made
> it
> > an error.
> 
> We do strict checking on other fields (like codec_delay). So i think we should
> be strict here as well and return false.

The bounds are int16_min -> int16_max, it's enforced by virtue of being an int16
type.

[commit-bot: I haz the power, 2013-12-16 22:35:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/104873011/20001

[commit-bot: I haz the power, 2013-12-17 00:16:22]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 01:37:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/104873011/20001

[commit-bot: I haz the power, 2013-12-17 02:06:09]

Change committed as 241102