Closed Bug 941283 Opened 11 years ago Closed 11 years ago

OpusTrackEncoder writing pre-skip at the original samplerate

Categories

(Core :: Audio/Video: Recording, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: rillian, Assigned: shelly)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ft:media-recording][qa-])

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [ft:media-recording]
Assignee: nobody → rlin
(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.
Attachment #8338246 - Flags: review?(tterribe)
Attachment #8338246 - Flags: review?(tterribe) → review?(giles)
Attachment #8338246 - Attachment is obsolete: true
Attachment #8338246 - Flags: review?(giles)
Attachment #8339758 - Flags: review?(giles)
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.
Attachment #8339758 - Flags: review?(giles) → review+
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.
Attachment #8339758 - Attachment is obsolete: true
Attachment #8340867 - Flags: review?(giles)
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.
Attachment #8340867 - Flags: review?(giles) → review+
Assignee: rlin → slin
Yes, carrying forward the r+ was what I intended.

Greenish on try. Land at will! :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/539fb8b1b9e3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [ft:media-recording] → [ft:media-recording][qa-[
Whiteboard: [ft:media-recording][qa-[ → [ft:media-recording][qa-]
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: