The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rillian, Assigned: derf)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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++]

Comment 2

5 years ago
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.

Comment 4

5 years ago
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)

Updated

5 years ago
Assignee: nobody → tterribe
Status: NEW → ASSIGNED
Depends on: 759612
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 6

5 years ago
Created attachment 628191 [details] [diff] [review]
Support header gain in Opus files
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?
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

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

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+
(Assignee)

Comment 12

5 years ago
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.
Attachment #628227 - Attachment is obsolete: true
Attachment #628604 - Flags: review+
(Assignee)

Comment 13

5 years ago
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.
Attachment #628604 - Attachment is obsolete: true
Attachment #628798 - Flags: review+
(Assignee)

Comment 14

5 years ago
Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=7b0b7246cb3a
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/359f1096bc7c
Target Milestone: --- → mozilla15
(Assignee)

Comment 16

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.