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.
Also, we'll want test files for this. I'm not sure how to verify output levels. Maybe we can use MozAudioAvailable?
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.
Created attachment 628191 [details] [diff] [review] Support header gain in Opus files
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
Created attachment 628227 [details] [diff] [review] Support header gain in Opus files v2 Updated to address initial review comments.
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.
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.
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.
Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=7b0b7246cb3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/359f1096bc7c
(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
or
