1246536 - If multiple opus packets are found in the last webm block, all will be set with DiscardPadding

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

Follow up from bug 1246358.

If the last webm block contains multiple opus packets, all packets are marked with the same start time and the same duration. This doesn't cause problem as such.

However, all of them will also have DiscardPadding set.

Meaning that we will only be able to decode the first packet, all remaining packets will cause a decoding error due to how DiscardPadding is set.

I believe DiscardPadding should only be set on the last opus packet of the last webm block as discard padding only applies to the last samples decoded.

Comment 1

[Jean-Yves Avenard [:jya]]

Thomas how should we handle discard padding for such case?

Can discard padding ever be used for something else than the last samples?

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Jean-Yves - should this be P1?

Comment 3

[Jean-Yves Avenard [:jya]]

Ive never seen those in the wild. It's a theorical issue found when looking at the code. 

YouTube doesn't use it.  So I think that P2 is appropriate.

Comment 4

[Thomas Daede [:TD-Linux]]

The webm parser, when finding all Opus packets in a block, should make sure to only call the decoder with the mExtraData field set for the last packet.

With Ogg Opus, the equivalent to DiscardPadding can only apply to a single packet. I think it's okay to enforce this restriction in Matroska too. Otherwise, you'd have to split it up and completely discard some packets.

Comment 5

[Jean-Yves Avenard [:jya]]

Sorry, not sure I follow. Are you saying we should ignore the discard packet if the webm block contains more than one packet, or just error?

Ensuring it only applies to the last packet shouldn't be difficult. However, if the last packet contains less frames than the discard frames count; as you said that becomes trickier.

Comment 6

[Thomas Daede [:TD-Linux]]

Just ensure it applies to only the last packet in a block. Error if it's greater than a packet size. It's possible to make it work up to the size of a block (by throwing away packets and adjusting discardpadding on the "new" last packet) but it's probably not worth doing - just throw an error like we already do.

Comment 7

[Timothy B. Terriberry (:derf)]

(In reply to Thomas Daede from comment #4)
> With Ogg Opus, the equivalent to DiscardPadding can only apply to a single
> packet. I think it's okay to enforce this restriction in Matroska too.
> Otherwise, you'd have to split it up and completely discard some packets.

Technically, Ogg Opus has the same issue, since the equivalent of DiscardPadding is derived from the timestamps, which are stored on pages, not packets. So it is technically possible to create a file that tries to trim more than one full packet. However, we state in the spec that you SHOULD NOT, in order to allow demuxers to do precisely the kind of packet-at-a-time processing that you're doing here. So I think Thomas's suggested approach is completely valid.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1246536: [webm] Only use discard padding information on last packet. r?kinetik

Review commit: https://reviewboard.mozilla.org/r/40027/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40027/

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8730567 [details]
MozReview Request: Bug 1246536: [webm] Only use discard padding information on last packet. r?kinetik

https://reviewboard.mozilla.org/r/40027/#review36535

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a65bdec004

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/66a65bdec004