When evicting data, the next appendBuffer could render samples undecodable

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
This is an actual bug in the W3C MSE spec, or more accurately, a problem that is not handled at all.

When appending a media segment, the coded frame processing algorithm (http://w3c.github.io/media-source/index.html#sourcebuffer-coded-frame-processing)
describes the step to detect a discontinuity, or how to append data to the current "coded frame group" http://w3c.github.io/media-source/index.html#coded-frame-group.

When appending a partial media segment, it is expected that the next appendBuffer will complete the media segment, and continue the current coded frame group.

When data is removed in the coded frame algorithm, it is possible that frames from the current coded frame group will be removed. If this happens, and the next appendBuffer data doesn't start with a keyframe, we will start inserting frames that can't be decoded.

I will open a W3C bug and in the mean time, implement what I believe should be done under this scenario.
(Assignee)

Comment 1

4 years ago
Created attachment 8622278 [details] [diff] [review]
P1. Update insertion index on the fly.

Prevent reparsing the entire stream in the next call to appendBuffer.
Attachment #8622278 - Flags: review?(cpearce)
(Assignee)

Comment 2

4 years ago
Created attachment 8622279 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

W3C bug pending.
Attachment #8622279 - Flags: review?(cpearce)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 3

4 years ago
Created attachment 8622864 [details] [diff] [review]
P1. Update insertion index on the fly.

Prevent reparsing the entire stream in the next call to appendBuffer.
Attachment #8622864 - Flags: review?(cpearce)
(Assignee)

Comment 4

4 years ago
Created attachment 8622865 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

W3C bug pending.
Attachment #8622865 - Flags: review?(cpearce)
(Assignee)

Updated

4 years ago
Attachment #8622278 - Attachment is obsolete: true
Attachment #8622278 - Flags: review?(cpearce)
(Assignee)

Updated

4 years ago
Attachment #8622279 - Attachment is obsolete: true
Attachment #8622279 - Flags: review?(cpearce)
Comment on attachment 8622864 [details] [diff] [review]
P1. Update insertion index on the fly.

Review of attachment 8622864 [details] [diff] [review]:
-----------------------------------------------------------------

Passing review to Alfredo. Thanks for taking on this responsibility Alfredo!
Attachment #8622864 - Flags: review?(cpearce) → review?(ayang)
Comment on attachment 8622865 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

Review of attachment 8622865 [details] [diff] [review]:
-----------------------------------------------------------------

Passing review to Alfredo. Thanks for taking on this responsibility Alfredo!
Attachment #8622865 - Flags: review?(cpearce) → review?(ayang)
Attachment #8622864 - Flags: review?(ayang) → review?(gsquelart)
Comment on attachment 8622865 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

Thank you for help, Gerald.
Attachment #8622865 - Flags: review?(ayang) → review?(gsquelart)
Attachment #8622864 - Flags: review?(gsquelart) → review+
Comment on attachment 8622865 [details] [diff] [review]
P2. Properly handle removal of data within the current coded frame group.

Review of attachment 8622865 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/TrackBuffersManager.h
@@ +229,5 @@
> +    void ResetAppendState()
> +    {
> +      mLastDecodeTimestamp.reset();
> +      mLastFrameDuration.reset();
> +      mHighestEndTimestamp.reset();

That's a lot of code in a header, and for a private method! Please consider moving it to the cpp.
Attachment #8622865 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/c2e4889a65a1
https://hg.mozilla.org/mozilla-central/rev/b3f78edbfbe3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.