758833 - Correct opus preskip handling

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

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

This bug is about a sane replacement.

Comment 1

[Ralph Giles (:rillian)]

Attachment: proposed fix

Sanity check, please.

Comment 2

[Timothy B. Terriberry (:derf)]

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.

Comment 3

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 4

[Timothy B. Terriberry (:derf)]

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.

Comment 5

[Ralph Giles (:rillian)]

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.

Comment 6

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 7

[Timothy B. Terriberry (:derf)]

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?

Comment 8

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 9

[Ralph Giles (:rillian)]

I've filed bug 759399 for the seeking preroll issue.

Comment 10

[Timothy B. Terriberry (:derf)]

(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.

Comment 11

[Ralph Giles (:rillian)]

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

cpearce is on vacation. doublec, do you mind reviewing instead?

Comment 12

[cajbir (:cajbir)]

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.

Comment 13

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 14

[Ralph Giles (:rillian)]

Heap or stack, I should say.

Comment 15

[cajbir (:cajbir)]

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

No that's fine, thanks for the explanation.

Comment 16

[Ralph Giles (:rillian)]

Ok, thanks again. Read to land.

Comment 17

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac05e654dd9

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1ac05e654dd9