Last Comment Bug 751219 - Opus codec in <audio> and <video> should support gain header
: Opus codec in <audio> and <video> should support gain header
Status: RESOLVED FIXED
[mentor=rillian] [lang=c++]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Timothy B. Terriberry (:derf)
:
Mentors:
Depends on: 674225 759612
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-02 09:34 PDT by Ralph Giles (:rillian) needinfo me
Modified: 2012-06-01 08:40 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Support header gain in Opus files (5.71 KB, patch)
2012-05-29 19:38 PDT, Timothy B. Terriberry (:derf)
no flags Details | Diff | Review
Support header gain in Opus files v2 (6.42 KB, patch)
2012-05-29 22:23 PDT, Timothy B. Terriberry (:derf)
kinetik: review+
Details | Diff | Review
Support header gain in Opus files v3 (6.48 KB, patch)
2012-05-30 22:37 PDT, Timothy B. Terriberry (:derf)
tterribe: review+
Details | Diff | Review
Support header gain in Opus files v4 (6.48 KB, patch)
2012-05-31 10:34 PDT, Timothy B. Terriberry (:derf)
tterribe: review+
Details | Diff | Review

Description Ralph Giles (:rillian) needinfo me 2012-05-02 09:34:13 PDT
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 2012-05-02 09:39:01 PDT
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] 2012-05-03 08:27:25 PDT
Want to link to a source file this work would take place inside?
Comment 3 Ralph Giles (:rillian) needinfo me 2012-05-03 09:23:47 PDT
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] 2012-05-03 09:51:47 PDT
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 2012-05-03 11:30:55 PDT
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) 2012-05-29 19:38:47 PDT
Created attachment 628191 [details] [diff] [review]
Support header gain in Opus files
Comment 7 Matthew Gregan [:kinetik] 2012-05-29 22:01:25 PDT
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) 2012-05-29 22:08:02 PDT
(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) 2012-05-29 22:15:14 PDT
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) 2012-05-29 22:23:54 PDT
Created attachment 628227 [details] [diff] [review]
Support header gain in Opus files v2

Updated to address initial review comments.
Comment 11 Matthew Gregan [:kinetik] 2012-05-29 22:27:39 PDT
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) 2012-05-30 22:37:30 PDT
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) 2012-05-31 10:34:51 PDT
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) 2012-05-31 10:35:46 PDT
Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=7b0b7246cb3a
Comment 15 Timothy B. Terriberry (:derf) 2012-05-31 11:15:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/359f1096bc7c
Comment 16 Timothy B. Terriberry (:derf) 2012-05-31 21:46:48 PDT
(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] 2012-06-01 08:40:00 PDT
https://hg.mozilla.org/mozilla-central/rev/359f1096bc7c

Note You need to log in before you can comment on or make changes to this bug.