Closed Bug 751219 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: rillian, Assigned: derf)

References

Details

(Whiteboard: [mentor=rillian] [lang=c++])

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 674225
Also, we'll want test files for this. I'm not sure how to verify output levels. Maybe we can use MozAudioAvailable?
Whiteboard: [mentor=rillian] [lang=c++]
Want to link to a source file this work would take place inside?
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.
No, I'm asking for pointers that someone interested in doing this work could use to begin doing just that.
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.
Assignee: nobody → tterribe
Status: NEW → ASSIGNED
Depends on: 759612
OS: Linux → All
Hardware: x86_64 → All
Attachment #628191 - Flags: review?(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?
(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.
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
Updated to address initial review comments.
Attachment #628191 - Attachment is obsolete: true
Attachment #628191 - Flags: review?(kinetik)
Attachment #628227 - Flags: review?(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.
Attachment #628227 - Flags: review?(kinetik) → review+
Rebased on top of the changes to bug 759612. Carrying forward r=kinetik.
Attachment #628227 - Attachment is obsolete: true
Attachment #628604 - Flags: review+
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.
Attachment #628604 - Attachment is obsolete: true
Attachment #628798 - Flags: review+
(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
https://hg.mozilla.org/mozilla-central/rev/359f1096bc7c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.