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

RESOLVED FIXED in Firefox 42

Status

()

P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.
Assignee: nobody → jyavenard
Priority: -- → P2
(Assignee)

Comment 1

3 years ago
Created attachment 8659757 [details] [diff] [review]
[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.
Attachment #8659757 - Flags: review?(gsquelart)
(Assignee)

Comment 2

3 years ago
Created attachment 8659758 [details] [diff] [review]
[MSE] P2. Don't assert when unable to find position in frames array.

Instead return an error which will terminate the video element.
Attachment #8659758 - Flags: review?(gsquelart)
Attachment #8659757 - Flags: review?(gsquelart) → review+
Attachment #8659758 - Flags: review?(gsquelart) → review+
(Assignee)

Updated

3 years ago
Blocks: 1197083
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1199676
https://hg.mozilla.org/mozilla-central/rev/f760a0088a81
https://hg.mozilla.org/mozilla-central/rev/78b76b57de21
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 7

3 years ago
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
Attachment #8659757 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 8

3 years ago
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 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.
Attachment #8659757 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8659758 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

3 years ago
status-firefox42: affected → fixed
You need to log in before you can comment on or make changes to this bug.