Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Correct opus preskip handling

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

Trunk
mozilla15
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

The current code trimming the initial samples out of the Opus decoder is, er, bad.

This bug is about a sane replacement.
Created attachment 627406 [details] [diff] [review]
proposed fix

Sanity check, please.
Attachment #627406 - Flags: feedback?(tterribe)
Comment on attachment 627406 [details] [diff] [review]
proposed fix

Review of attachment 627406 [details] [diff] [review]:
-----------------------------------------------------------------

This is moving in the right direction, but there's the issue of seeking...

::: content/media/ogg/nsOggCodecState.cpp
@@ +780,5 @@
>  
>    if (mActive && mDecoder) {
>      // Reset the decoder.
>      opus_decoder_ctl(mDecoder, OPUS_RESET_STATE);
> +    mSkipped = 0;

The issue here is that Reset() gets called after seeking, as well as when you start playing again from scratch. The amount to decode for convergence may be different after seeking than when starting the file (because when you're starting, it is from a known state). The Ogg Opus spec recommends skipping 80 ms (3840 samples at 48 kHz) after seeking, which could be either larger or smaller than the initial pre-skip.

I'd recommend keeping an mToSkip or similar variable, which counts _down_ the number of samples which need to be skipped. That could be set to the pre-skip on init, 3840 on a seek, or also to the pre-skip if subtracting 3840 samples from the seek target would put it in the pre-skip region.
Attachment #627406 - Flags: feedback?(tterribe) → feedback-
Created attachment 627749 [details] [diff] [review]
proposed fix 2

Thanks for the feedback, Tim. I didn't handle the seeking case because there's no easy for the codec state to know when it's reset to the beginning, or to hint the seek logic to allow for preroll. We'll have to poke this in directly from nsOggReader. What do you think, Chris?

Nevertheless, counting down instead of up makes the code more clear, so I've done that, and reset the mSkip count to 3840 frames in nsOpusState::Reset(). I only see the reset method called after seek, not after initial playback.

With the current patch, we trim an extra ~70 ms off the beginning on replay.
Attachment #627406 - Attachment is obsolete: true
Yeah, I thought about this some more, and I think the correct solution is probably to handle the 3840 sample offset in the seeking code itself. I.e., we should just seek back that much farther to begin with, and let the existing DecodeToTarget code handle skipping the output prior to where we want actual playback to begin. The reason for this is that we won't always be able to land exactly 3840 samples before where we want to start playing (because we can only seek at page/packet granularity).

That still leaves a wrinkle... what to do when seeking back to the beginning of the stream? I don't think you can do something here that does not rely on examining the granpos to either determine the correct amount to skip or to force seeking back to the first data page. Both rely on knowing the starting granule position (i.e., from immediately prior to the first sample in the file), to handle the case where it is not 0.
I agree it makes more sense to do the trimming in the seek code in nsOggReader, since it's the component that knows where it is in the stream.
Created attachment 627803 [details] [diff] [review]
proposed fix 3

Updated patch. This version pokes mPreSkip back into mSkip after seeking to the start, but otherwise returns audio immediately starting at the seek point.

That's still not correct, but at least is no regression over the previous code.
Attachment #627749 - Attachment is obsolete: true
Attachment #627803 - Flags: feedback?(tterribe)
Comment on attachment 627803 [details] [diff] [review]
proposed fix 3

Review of attachment 627803 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this looks like it's a solid improvement.

::: content/media/ogg/nsOggReader.cpp
@@ +424,5 @@
> +    PRInt32 skipFrames = NS_MIN(mOpusState->mSkip, frames);
> +    if (skipFrames == frames) {
> +      // discard the whole packet
> +      mOpusState->mSkip -= frames;
> +      LOG(PR_LOG_DEBUG, ("Opus decoder skipping packet %d frames", frames));

Perhaps a colon after packet?
Attachment #627803 - Flags: feedback?(tterribe) → feedback+
Created attachment 627810 [details] [diff] [review]
proposed fix 4

Thanks. I've changed to log message to

  "Opus decoder skipping 960 frames (whole packet)"

Which is grammatical while still lining up with the non-whole-packet case.
Assignee: nobody → giles
Attachment #627803 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #627810 - Flags: review?(cpearce)
I've filed bug 759399 for the seeking preroll issue.
(In reply to Ralph Giles (:rillian) from comment #9)
> I've filed bug 759399 for the seeking preroll issue.

Thanks. I have patches. I'll post as soon as I sort out some hardware issues on my end.
Blocks: 759490
Comment on attachment 627810 [details] [diff] [review]
proposed fix 4

cpearce is on vacation. doublec, do you mind reviewing instead?
Attachment #627810 - Flags: review?(cpearce) → review?(chris.double)
Blocks: 749994
Depends on: 674225

Comment 12

5 years ago
Comment on attachment 627810 [details] [diff] [review]
proposed fix 4

Review of attachment 627810 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/ogg/nsOggReader.cpp
@@ +118,5 @@
>    if (mVorbisState && NS_FAILED(mVorbisState->Reset())) {
>      res = NS_ERROR_FAILURE;
>    }
> +  if (mOpusState) {
> +    if NS_FAILED(mOpusState->Reset()) {

Please put brackets around the NS_FAILED call. ie: if (NS_FAILED(..)) { ... }

@@ +429,5 @@
> +                         " (whole packet)", frames));
> +      return NS_OK;
> +    }
> +    PRInt32 keepFrames = frames - skipFrames;
> +    int samples = keepFrames * channels;

Can this overflow? If so, please check for that case. Possibly the types are constrained already so it can't since I see the other Ogg code in the file doesn't check. It'd be great though if you could use PR_STATIC_ASSERT to force a compile time check if it's needed. See nsWaveDecoder::DecodeAudioData for an example. See bug 623998 where there are overflow issues due to some versions of gcc not seeing if sizeof (AudioDataValue) * samples overflows inside operator new.
Attachment #627810 - Flags: review?(chris.double) → review+
Created attachment 628582 [details] [diff] [review]
proposed fix 5

Thanks for the review.

(In reply to Chris Double (:doublec) from comment #12)

> > +  if (mOpusState) {
> > +    if NS_FAILED(mOpusState->Reset()) {
> 
> Please put brackets around the NS_FAILED call. ie: if (NS_FAILED(..)) { ... }

Oops. Fixed.

> > +    PRInt32 keepFrames = frames - skipFrames;
> > +    int samples = keepFrames * channels;
> 
> Can this overflow?

It cannot overflow. Statically skipFrames must be less than or equal to frames, and opus_decoder_get_nb_frames can return no more than 63*2880. Channels is read from an 8 bit field in the header, so the maximum total size of of the allocation.

However, that's just a static value analysis; I could add run-time asserts or checks, but can't prove it from the type sizes, and of course it could overflow if the stack is already corrupt.

Do you want me to use a checked int anyway?

 If so, please check for that case. Possibly the types are
> constrained already so it can't since I see the other Ogg code in the file
> doesn't check. It'd be great though if you could use PR_STATIC_ASSERT to
> force a compile time check if it's needed. See
> nsWaveDecoder::DecodeAudioData for an example. See bug 623998 where there
> are overflow issues due to some versions of gcc not seeing if sizeof
> (AudioDataValue) * samples overflows inside operator new.
Attachment #627810 - Attachment is obsolete: true
Heap or stack, I should say.

Comment 15

5 years ago
> Do you want me to use a checked int anyway?

No that's fine, thanks for the explanation.
Ok, thanks again. Read to land.
Keywords: checkin-needed

Updated

5 years ago
Attachment #628582 - Flags: review+

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac05e654dd9
Keywords: checkin-needed
Target Milestone: --- → mozilla14

Updated

5 years ago
Target Milestone: mozilla14 → mozilla15
https://hg.mozilla.org/mozilla-central/rev/1ac05e654dd9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.