1069660 - Strength checks for WebM DiscardPadding

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

For Opus (and Vorbis) in WebM we added the DiscardPadding element to allow sample-accurate trimming at the end of tracks. And we implement this part, up to the size of the current packet.

 - DisardPadding can 64 bits. We should reject files where it's larger than the block duration.

 - Matroska says DiscardPadding can be negative to support trimming at the start of the block. I don't think we should allow this for Opus. Trimming at the start of the track is already handled by CodecDelay, and it's not clear how the two interact. It doesn't make sense to cut parts of decoded data out of the middle of the stream.

- The Ogg Opus draft says, "The number of discarded samples SHOULD be no larger than the number decoded from the last packet." For compatibility we should reject WebM files where DiscardPadding is larger. Also saves fixing our implementation for when there are multiple packets in the block.

- DiscardPadding can be present on any block. We should reject tracks where it occurs anywhere but the final block.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: wip, p1: Factor out Vorbis/Opus decoding to helpers.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: wip, p2: Enforce Opus DiscardPadding restrictions.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: p1: Factor Vorbis and Opus decoding out

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: p2: WebMReader cleanups

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: p3: Enforce DiscardPadding validity

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: p4: Add mochitests for WebM/Opus files with invalid DiscardPadding

Comment 7

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=83eba4e333a7

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8496696 [details] [diff] [review]
p1: Factor Vorbis and Opus decoding out

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

r=me with nits addressed.

::: content/media/webm/WebMReader.cpp
@@ +540,5 @@
>    // the previous audio chunk, we need to increment the packet count so that
>    // the vorbis decode doesn't use data from before the gap to help decode
>    // from after the gap.
> +  CheckedInt64 tstamp_frames = UsecsToFrames(tstamp_usecs, mInfo.mAudio.mRate);
> +  CheckedInt64 decoded_frames = UsecsToFrames(mAudioStartUsec, mInfo.mAudio.mRate);

Please wrap to 80 columns. That's why 'rate' was here as a local in the first place, I think.

@@ +657,5 @@
> +}
> +
> +#ifdef MOZ_OPUS
> +bool WebMReader::DecodeOpus(unsigned char* aData, size_t aLength,
> +                            int64_t aOffset, uint64_t aTstampUsecs, nestegg_packet* aPacket)

80 columns here too.

::: content/media/webm/WebMReader.h
@@ +180,5 @@
> +                    int64_t aOffset, uint64_t aTstampUsecs, int32_t* aTotalFrames);
> +#ifdef MOZ_OPUS
> +  bool DecodeOpus(unsigned char* aData, size_t aLength,
> +                  int64_t aOffset, uint64_t aTstampUsecs, nestegg_packet* aPacket);
> +#endif

It's odd that vorbis needs total_frames and opus needs aPacket.

We should fix that in a separate bug, making DiscardPadding work for Vorbis as well.

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8496697 [details] [diff] [review]
p2: WebMReader cleanups

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

I don't mind de-duping variables, but they don't help us stay within the 80 column limit. :(

::: content/media/webm/WebMReader.cpp
@@ +500,5 @@
> +                                                 mOpusParser->mChannels,
> +                                                 mOpusParser->mStreams,
> +                                                 mOpusParser->mCoupledStreams,
> +                                                 mOpusParser->mMappingTable,
> +                                                 &r);

I give up.

@@ +701,5 @@
>                           " (whole packet)", frames));
>        return true;
>      }
>      int32_t keepFrames = frames - skipFrames;
> +    PodMove(buffer.get(), buffer.get() + skipFrames * channels, keepFrames * channels);

This keeps the full buffer around. I guess that's ok because it only happens on the first frames?

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8496699 [details] [diff] [review]
p4: Add mochitests for WebM/Opus files with invalid DiscardPadding

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

Thanks for making test files!

::: content/media/test/test_invalid_reject_play.html
@@ +20,5 @@
> +  // Seeing a decoder error is a success.
> +  v.addEventListener("error", function(e) {
> +    is(v.error.code, v.error.MEDIA_ERR_DECODE,
> +      "decoder should reject " + test.name);
> +    v.removeEventListener('error', arguments.callee, false);

arguments.callee is poor style. Name the handler function instead:

v.addEventListener('error', function onerror(e) {
  ...
  v.removeEventListener(e.type, onerror, false);
  ...
}

@@ +23,5 @@
> +      "decoder should reject " + test.name);
> +    v.removeEventListener('error', arguments.callee, false);
> +    manager.finished(token);
> +  });
> +

Please also add a listener for 'ended' or something similar which fails the test and calls manager.finished() so we don't have to wait for timeout in the failure case.

Comment 11

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0fe62cca9c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9a5527bda7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ccd29f9bba
https://hg.mozilla.org/integration/mozilla-inbound/rev/60ca1b747822
https://hg.mozilla.org/integration/mozilla-inbound/rev/1edf6a3b6ca0

Comment 12

[Matthew Gregan [:kinetik]]

(In reply to Ralph Giles (:rillian) from comment #9)
> I don't mind de-duping variables, but they don't help us stay within the 80
> column limit. :(

I've addressed the long lines in a separate commit, which also fixes a bunch of other cases in the same two files.

> ::: content/media/webm/WebMReader.cpp
> @@ +500,5 @@

> I give up.

That one is still under 80 columns, the indentation was just off originally.

> @@ +701,5 @@
> This keeps the full buffer around. I guess that's ok because it only happens
> on the first frames?

Yeah, I figured that was better than multiple allocations.  The change for discard padding does the same thing.  It's a bit lazy to leave the initial size allocated, but the allocation only lives until we play that buffer.  It wouldn't be too hard to change the code to realloc, though.

Comment 13

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #12)
> Yeah, I figured that was better than multiple allocations.  The change for
> discard padding does the same thing.  It's a bit lazy to leave the initial
> size allocated, but the allocation only lives until we play that buffer.  It
> wouldn't be too hard to change the code to realloc, though.

I forgot, I did have a better reason than just laziness: we can't use realloc easily, because the AudioData owns this memory via an nsAutoArrayPtr, so we'd require another allocation via new[] to pass the final pointer on (or changes to AudioData). :-(

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0fe62cca9c0
https://hg.mozilla.org/mozilla-central/rev/2b9a5527bda7
https://hg.mozilla.org/mozilla-central/rev/e9ccd29f9bba
https://hg.mozilla.org/mozilla-central/rev/60ca1b747822
https://hg.mozilla.org/mozilla-central/rev/1edf6a3b6ca0