Closed Bug 1246536 Opened 5 years ago Closed 5 years ago
If multiple opus packets are found in the last webm block, all will be set with Discard
MozReview Request: Bug 1246536: [webm] Only use discard padding information on last packet. r?kinetik
58 bytes, text/x-review-board-request
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.
Thomas how should we handle discard padding for such case? Can discard padding ever be used for something else than the last samples?
Jean-Yves - should this be P1?
Priority: -- → P2
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.
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.
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.
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.
(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.
Review commit: https://reviewboard.mozilla.org/r/40027/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40027/
Attachment #8730567 - Flags: review?(kinetik)
Attachment #8730567 - Flags: review?(kinetik) → review+
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
You need to log in before you can comment on or make changes to this bug.