Reading the code today I noticed we're scaling the lookahead reported by the libopus encoder as if it was reported in the original sample rate. The encoder reports the lookahead in units of 48 kHz samples, and we need to write that the the OpusHead 'id' header in those same units, so we should not be scaling by the sample rate ratio.

see https://mxr.mozilla.org/mozilla-central/source/content/media/encoder/OpusTrackEncoder.cpp#213

(In reply to Ralph Giles (:rillian) from comment #0) > libopus encoder as if it was reported in the original sample rate. The > encoder reports the lookahead in units of 48 kHz samples, and we need to > write that the the OpusHead 'id' header in those same units, so we should To be clear, the encoder reports the lookahead in the same units that were used to initialize the encoder instance, which is returned by GetOutputSamplingRate(), and might _not_ be 48 kHz. The value in the OpusHead 'id' header must use 48 kHz units. The code used to be correct before resampling support was added (when we required that mSamplingRate was a value supported by the libopus API).

Since we create the opus encoder with |GetOutputSampleRate()|, the sampling rate to write in Id header should be |GetOutputSampleRate()| as well.

Comment on attachment 8339758 [details] [diff] [review] Correct the pre-skip at its original samplerate Review of attachment 8339758 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Shelly. r=me if you correct the third argument. This code is way too clever, but looks like it should work based on derf's statement about what units mLookAhead returns. It also looks like OpusTrackEncoder will fail for 9600 Hz audio. That rate evenly divides 48000, so OpusTrackEncoder::Init won't enable resampling, but libopus doesn't support that rate, so opus_encoder_init will fail. Fixing that should be a separate patch though. ::: content/media/encoder/OpusTrackEncoder.cpp @@ +214,5 @@ > > // The ogg time stamping and pre-skip is always timed at 48000. > + SerializeOpusIdHeader(mChannels, mLookahead * (kOpusSamplingRate / > + GetOutputSampleRate()), GetOutputSampleRate(), > + &meta->mIdHeader); mSampleRate should still be passed for aInputSampleRate. The point of that field is to record the original sample rate in the file so a decoder can resample back to the original rate when that makes sense.

Thanks Ralph :). At first I though the input sampling rate should be the same as the one we pass for creating opus encoder, didn't read the doc clear enough. I'll file a bug for the issue of 96000 input sampling rate later.

Comment on attachment 8340867 [details] [diff] [review] [ver. 2] Correct the pre-skip at its original samplerate Hmmm, I guess it's okay to carry r+ with the fix lol.

Try result: https://tbpl.mozilla.org/?tree=Try&rev=37b13e5c4f16

Yes, carrying forward the r+ was what I intended. Greenish on try. Land at will! :)