751219 - Opus codec in <audio> and <video> should support gain header

Status: RESOLVED FIXED

Description

[Ralph Giles (:rillian) | needinfo me]

The Ogg Opus encapsulation (https://wiki.xiph.org/OggOpus) spec defines a manditory 'gain' header players are required to apply to the output of the decoder. This bug is about implementing that.

The gain is in fixed-point dB, so we just need to loop over the returned samples and apply sample *= pow(10., 20*mGain). The pow value should be precomputed.

Comment 1

[Ralph Giles (:rillian) | needinfo me]

Also, we'll want test files for this. I'm not sure how to verify output levels. Maybe we can use MozAudioAvailable?

Comment 2

[Josh Matthews [:jdm]]

Want to link to a source file this work would take place inside?

Comment 3

[Ralph Giles (:rillian) | needinfo me]

If you're asking for a test file, I would make one just by hexediting the output of opusenc, e.g. content/media/tests/detodos.opus. You can run it through rogg_crcfix to update the Ogg page crc values afterward.

Comment 4

[Josh Matthews [:jdm]]

No, I'm asking for pointers that someone interested in doing this work could use to begin doing just that.

Comment 5

[Ralph Giles (:rillian) | needinfo me]

Ah. In addition to the spec linked in the description, the code lives in nsOpusState and nsOggReader::DecodeOpus. These will be found in http://hg.mozilla.org/mozilla-central/file/tip/content/media/ogg/nsOggReader.cpp and http://hg.mozilla.org/mozilla-central/file/tip/content/media/ogg/nsOggCodecState.cpp after the next green merge, when bug 674225 finishes landing.

Comment 6

[Timothy B. Terriberry (:derf)]

Created attachment 628191 [details] [diff] [review]
Support header gain in Opus files

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 628191 [details] [diff] [review]
Support header gain in Opus files

Review of attachment 628191 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/ogg/nsOggCodecState.cpp
@@ +757,5 @@
>    mNominalRate(0),
>    mChannels(0),
>    mPreSkip(0),
> +#ifdef MOZ_SAMPLE_TYPE_FLOAT32
> +  mGain(1.0F),

Very minor, but prevailing style across the code base is to use lower case for float suffixes.  This applies to the other instance below, too.

@@ +852,5 @@
>  
>        mChannels= aPacket->packet[9];
>        mPreSkip = LEUint16(aPacket->packet + 10);
>        mNominalRate = LEUint32(aPacket->packet + 12);
> +      double gain_dB = LEUint16(aPacket->packet + 16) / 256.0;

Since the original format is Q7.8, I assume the high bit is a sign bit (the OggOpus encapsulation mentions +/-128dB), so this should be parsed as a signed 16-bit value.

@@ +857,5 @@
> +#ifdef MOZ_SAMPLE_TYPE_FLOAT32
> +      mGain = static_cast<float>(pow(10,0.05*gain_dB));
> +#else
> +      mGain_Q16 = static_cast<PRInt32>(NS_MIN(65536*pow(10,0.05*gain_dB)+0.5),
> +                                              static_cast<double>(INT32_MAX));

For consistency, either use PR_INT32_MAX here or make mGain_Q16 an int32_t (or int) and change the cast accordingly.

::: content/media/ogg/nsOggReader.cpp
@@ +463,5 @@
> +  // Apply the header gain if one was specified.
> +#ifdef MOZ_SAMPLE_TYPE_FLOAT32
> +  if (mOpusState->mGain != 1.0F) {
> +    float gain = mOpusState->mGain;
> +    int samples = frames * channels;

Does this (and the same code below, and the same code earlier in where AudioDataValue is allocated) need an overflow check or is opus_decoder_get_nb_samples guaranteed never to return more than 2^31/255?

Comment 8

[Timothy B. Terriberry (:derf)]

(In reply to Matthew Gregan [:kinetik] from comment #7)
> > +      double gain_dB = LEUint16(aPacket->packet + 16) / 256.0;
> 
> Since the original format is Q7.8, I assume the high bit is a sign bit (the
> OggOpus encapsulation mentions +/-128dB), so this should be parsed as a
> signed 16-bit value.

Good catch! It was like that in the original code, but I should definitely fix it.

> For consistency, either use PR_INT32_MAX here or make mGain_Q16 an int32_t
> (or int) and change the cast accordingly.

I should point out there's a bunch of other code here that has this problem (which is what I was copying from).

> Does this (and the same code below, and the same code earlier in where
> AudioDataValue is allocated) need an overflow check or is
> opus_decoder_get_nb_samples guaranteed never to return more than 2^31/255?

opus_decoder_get_nb_samples will never return more than 120 ms worth. Since we use a 48 kHz sample rate, that's limited to 5760 samples.

Comment 9

[Timothy B. Terriberry (:derf)]

Also, I forgot to post earlier:
This test file sounds extremely quiet/silent without header gain support, and normal with that support: http://people.xiph.org/~greg/opus_testvectors/correctness_gain_loud_speech.opus

Comment 10

[Timothy B. Terriberry (:derf)]

Created attachment 628227 [details] [diff] [review]
Support header gain in Opus files v2

Updated to address initial review comments.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 628227 [details] [diff] [review]
Support header gain in Opus files v2

Thanks!

(In reply to Timothy B. Terriberry (:derf) from comment #9)
> Also, I forgot to post earlier:
> This test file sounds extremely quiet/silent without header gain support,
> and normal with that support:
> http://people.xiph.org/~greg/opus_testvectors/correctness_gain_loud_speech.
> opus

The inverse of this would be useful for testing negative gain.

Comment 12

[Timothy B. Terriberry (:derf)]

Created attachment 628604 [details] [diff] [review]
Support header gain in Opus files v3

Rebased on top of the changes to bug 759612. Carrying forward r=kinetik.

Comment 13

[Timothy B. Terriberry (:derf)]

Created attachment 628798 [details] [diff] [review]
Support header gain in Opus files v4

Fix a dropped closing brace in the fixed-point code. I tested it before, I swear, but must have screwed it up rebasing. Always push to try, kids. Carrying forward r=kinetik.

Comment 14

[Timothy B. Terriberry (:derf)]

Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=7b0b7246cb3a

Comment 15

[Timothy B. Terriberry (:derf)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/359f1096bc7c

Comment 16

[Timothy B. Terriberry (:derf)]

(In reply to Matthew Gregan [:kinetik] from comment #11)
> The inverse of this would be useful for testing negative gain.

The always-fabulous Greg Maxwell has made one just for you: http://people.xiph.org/~greg/opus_testvectors/correctness_gain_silent_output.opus

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/359f1096bc7c