759612 - Opus granule position handling is incorrect

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

Currently, our Ogg Opus code does not handle end trimming correctly. Specifically, a granule position smaller than normal can be used to discard some of the last samples in the file, but we don't currently do this.

The spec (currently https://wiki.xiph.org/OggOpus) also requires us to reject streams that have certain invalid granule position values on the first page, to prevent the spread of tools which produce such broken files.

Comment 1

[Timothy B. Terriberry (:derf)]

Attachment: Update granule position accounting for Opus

This patch solves both problems, since they both required rewriting RecalculateOpusGranulepos().

Getting us to reject streams based on the granule position of the first data page (after the headers) was actually surprisingly difficult. There were two possible approaches I saw:
1) Make PageIn() fail during nsOggReader::FindStartTime(), and use this to make nsBuiltinDecoderStateMachine::ReadMetadata() fail.
2) Continue queuing pages after the headers during ReadHeaders(), and make PageIn() fail there.

This patch goes with approach #2. A bunch of decisions get made early on based on the available streams, and it is not at all clear how to go about updating them afterwards with approach #1. For example, ReadMetadata() passes back mInfo to the nsBuiltinDecoderReader, which has an mHasAudio flag that can be queried off the decode thread... if PageIn fails later and deactivates the audio stream, it wasn't at all clear to me how it could update this flag).

Approach #2 does require reworking a bit of the header parsing code, but the damage is contained to the Ogg-specific code. The main changes are that ReadHeaders() no longer verifies that a packet is a header before passing it on to DecodeHeader() (not a big deal, since the header-parsing code checks the format anyway), and that it does not automatically free the packet afterwards. Instead DecodeHeader() takes ownership and is responsible for freeing it, since in the Opus case, we want to stick the first data packet back on the queue to get decoded later.

Comment 2

[Timothy B. Terriberry (:derf)]

Various test files:
A valid one: http://people.xiph.org/~greg/opus_testvectors/correctness_trimming_nobeeps.opus
The file contents itself describe the correct behavior (no audible beeps).

Some invalid ones (all three of these should be rejected):
http://people.xiph.org/~tterribe/opus/test_files/failure-first_gp_less_than_preskip.opus
http://people.xiph.org/~tterribe/opus/test_files/failure-first_gp_less_than_sample_count.opus
http://people.xiph.org/~tterribe/opus/test_files/failure-end_gp_less_than_preskip.opus

With the above patch (applied on top of the ones in bug 758833, bug 759490, and bug 759399, in that order, behavior is correct for all four files. Tests pass locally.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 628189 [details] [diff] [review]
Update granule position accounting for Opus

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

I know it's not part of this patch, but I also noticed that mPreSkip in OpusState is declared as an int but read with LEUint16.  And mNominalRate is declared as an int but read with LEUint32.  It's easier to reason about the places where these values are used if the type matches the legal values that will be contained in the variable.

::: content/media/ogg/nsOggCodecState.cpp
@@ +280,5 @@
>    //    0x82 -> Setup header
>    // See http://www.theora.org/doc/Theora.pdf Chapter 6, "Bitstream Headers",
>    // for more details of the Ogg/Theora containment scheme.
>    bool isSetupHeader = aPacket->bytes > 0 && aPacket->packet[0] == 0x82;
> +  ReleasePacket(aPacket);

Please document that these functions now take ownership of the packet.

I think I'd rather see the packet stuffed into an nsAutoReleasePacket at the start of the functions receiving ownership of the packets so that later changes can't result in leaks.  You'll need to add a forget() to nsAutoReleasePacket for the case where you want to requeue the packet in the Opus DecodeHeader implementation.

nsAutoReleasePacket can probably be converted into an nsAutoRef so you get this for free (it's called disown() rather than forget() there, I think).

@@ +876,2 @@
>  
> +    } break;

break on a separate line please.

@@ +1006,5 @@
> +  int nframes;
> +  nframes = opus_packet_get_nb_frames(packet->packet, packet->bytes);
> +  if (nframes > 0) {
> +    int nsamples;
> +    nsamples = opus_packet_get_samples_per_frame(packet->packet, 48000);

Merge this line and the declaration.  Can this fail or return a negative value?  If so, error check, otherwise you could skip declaring nsamples altogether and merge the call with the return below.

@@ +1413,5 @@
> +      mLength = LEInt64(aPacket->packet + SKELETON_FILE_LENGTH_OFFSET);
> +
> +      LOG(PR_LOG_DEBUG, ("Skeleton segment length: %lld", mLength));
> +
> +      // Initialize the serianlno-to-index map.

Typo.

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

This can be |if (mOpusState && NS_FAILED(mOpusState->Reset(start)))|.

Comment 4

[Timothy B. Terriberry (:derf)]

Attachment: Update granule position accounting for Opus v2

(In reply to Matthew Gregan [:kinetik] from comment #3)
> declared as an int but read with LEUint32.  It's easier to reason about the
> places where these values are used if the type matches the legal values that
> will be contained in the variable.

Though the compiler may generate worse code (at least in the mPreSkip case.. mNominalRate is never actually used). But none of this is on a critical path, so I'm happy to make this change.

> Please document that these functions now take ownership of the packet.

Done.

> nsAutoReleasePacket can probably be converted into an nsAutoRef so you get
> this for free (it's called disown() rather than forget() there, I think).

I wound up not using nsAutoReleasePacket in the patch originally because it was declared locally in nsOggReader.cpp and not available in nsOggCodecState.cpp (and I was too lazy to move it). But I'll find my motivation if it means I can delete code. I went ahead and killed nsAutoReleasePacket entirely and replaced it with nsAutoRef, and used the latter in DecodeHeader().

> > +    } break;
> 
> break on a separate line please.

Done.

> Merge this line and the declaration.  Can this fail or return a negative
> value?  If so, error check, otherwise you could skip declaring nsamples
> altogether and merge the call with the return below.

No, it can't fail (it only looks at a few bits of the first byte, and all possibilities are legal values). Merged with the return as suggested.

> > +      // Initialize the serianlno-to-index map.
> 
> Typo.

I'm not even re-indenting this code now, but fixed anyway. Good catch.

> This can be |if (mOpusState && NS_FAILED(mOpusState->Reset(start)))|.

Done.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 628600 [details] [diff] [review]
Update granule position accounting for Opus v2

Thanks!

Comment 6

[Timothy B. Terriberry (:derf)]

Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=9b45a4bcf0ea

Comment 7

[Timothy B. Terriberry (:derf)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/010313752c64

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/010313752c64