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)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: rillian, Assigned: derf)
References
Details
(Whiteboard: [mentor=rillian] [lang=c++])
Attachments
(1 file, 3 obsolete files)
6.48 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Also, we'll want test files for this. I'm not sure how to verify output levels. Maybe we can use MozAudioAvailable?
Reporter | ||
Updated•9 years ago
|
Whiteboard: [mentor=rillian] [lang=c++]
Comment 2•9 years ago
|
||
Want to link to a source file this work would take place inside?
Reporter | ||
Comment 3•9 years ago
|
||
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•9 years ago
|
||
No, I'm asking for pointers that someone interested in doing this work could use to begin doing just that.
Reporter | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
Assignee: nobody → tterribe
Status: NEW → ASSIGNED
Depends on: 759612
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #628191 -
Flags: review?(kinetik)
Comment 7•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
Updated to address initial review comments.
Attachment #628191 -
Attachment is obsolete: true
Attachment #628191 -
Flags: review?(kinetik)
Attachment #628227 -
Flags: review?(kinetik)
Comment 11•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=7b0b7246cb3a
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/359f1096bc7c
Target Milestone: --- → mozilla15
Assignee | ||
Comment 16•9 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
Comment 17•9 years ago
|
||
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.
Description
•