1188238 - Frames with a duration of 0 could be improperly inserted or cause later insertion to be incorrect

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Encountered a DASH video in bug 1186149 where the first three frames demuxed have a size of 0.

They got me thinking on how those could mess the current frame handling inside the track buffer.

If those frames are found right at the beginning and within a block of other frames, then they will be properly inserted.

However, when searching where to insert our next block of frame, if we have detected a discontinuity, we rely on finding the first frame intersecting our block of new frames.

If that frame happened to have a duration of 0, the time interval they cover is empty, and as such they will never intersect with anything.

What would occur here, is the next block of frame would be inserted *after* that particular frame (which also happen to be a key frame).
It would render all frames depending on that 0s frame undecodable.

Need to find a new algorithm or modify TimeIntervals so it can handle to interval of 0 duration.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P1. Don't use Interval::Intersect to find the first frame of an interval.

With H264, often the first frame of a media segment has no duration ; as such the time interval it represents is empty and will never intersect with anything.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P2. Don't assert when unable to find position in frames array.

Instead return an error which will terminate the video element.

Comment 3

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02841936fcd7

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f760a0088a81
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b76b57de21

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f760a0088a81
https://hg.mozilla.org/mozilla-central/rev/78b76b57de21

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8659757 [details] [diff] [review]
[MSE] P1. Don't use Interval::Intersect to find the first frame of an interval.

Approval is for the two patches

Approval Request Comment
[Feature/regressing bug #]:1171330
[User impact if declined]: Crashes ; stall or YouTube reverting to Flash
[Describe test coverage new/current, TreeHerder]: in central, manual test
[Risks and why]: Very low. We now return an error rather than crashing. We also now better calculate where to insert our frames
[String/UUID change made/needed]: None

Comment 8

[Jean-Yves Avenard [:jya]]

Not sure what's the policy is about the MSE blanker approval ; if it's something I could have simply pushed right away without explicit aurora+ .

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8659757 [details] [diff] [review]
[MSE] P1. Don't use Interval::Intersect to find the first frame of an interval.

Indeed, you have to use the regular uplift process.

Comment 10

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/e0c5c53a99a9
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a9cc4e966637