If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 48

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Unassigned)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Thomas how should we handle discard padding for such case?

Can discard padding ever be used for something else than the last samples?
Flags: needinfo?(tdaede)
Jean-Yves - should this be P1?
Flags: needinfo?(jyavenard)
Priority: -- → P2
(Reporter)

Comment 3

2 years ago
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.
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(tdaede)
(Reporter)

Comment 5

2 years ago
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.
(Reporter)

Comment 8

2 years ago
Created attachment 8730567 [details]
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/
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

Comment 10

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/66a65bdec004
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.