An append of SourceBuffer data that is into the future results in wrong buffered data

RESOLVED INVALID

Status

()

defect
RESOLVED INVALID
5 years ago
5 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
When a SourceBuffer has data appended that is beyond the last parsed timestamp (ie. a discontiguous region in the future) then the buffered range for that source buffer is reported as containing from the earliest point in the buffer to the end of that discontiguous range. This causes issues during the decoder selection algorithm.

The data is also appended to the MediaSourceResource meaning that resource things it has the data for that range, rather than it actually be two seperate blocks of data.

This occurs when seeking into the future and has an effect when then seeking into the past and letting playback continue into that source buffer and having the normal playback cross the data range.
(Assignee)

Updated

5 years ago
Summary: And append of SourceBuffer data that is into the future results in wrong buffered data → An append of SourceBuffer data that is into the future results in wrong buffered data
(Assignee)

Comment 1

5 years ago
This change spins a new decoder if the data appended is discontinuous. Previously it was doing this if the data overlapped with an existing region. This does it if is discontinuous in any way.
Assignee: nobody → cajbir.bugzilla
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: MSE, 1056440
Bug 1059058 has had a fix for this since Friday.
(Assignee)

Comment 3

5 years ago
(In reply to Matthew Gregan [:kinetik] from comment #2)
> Bug 1059058 has had a fix for this since Friday.

Which part of bug 1059058 specifically? Rebasing on top of fixes in that I also need:

+ if (mParser->IsMediaSegmentPresent(aData, aLength) && fabs(mTrackBuffer->LastParsedTimestamp() - start) > 0.1)

ie. I need to handle where 'start' is greater than  'mTrackBuffer->LastParsedTimestamp()' and spin off the new decoder.
(Assignee)

Comment 5

5 years ago
(In reply to Matthew Gregan [:kinetik] from comment #4)
> Sorry, in bug 1061007.

Same question for that bug then. Which part specifically fixes this issue? With those patches landed I get the same issue with discontiguous data in the future being added. With the fix it works.
(Assignee)

Comment 6

5 years ago
Rebases on top of 1061007 and 1059058 changes.
Attachment #8483167 - Attachment is obsolete: true
Oh, <= != <, which I fixed later but haven't updated the patch for.  Too tired to read code properly.  fabs() is simpler.
Attachment #8483208 - Flags: review+
(Assignee)

Comment 8

5 years ago
Needs bug 1059058 attachment 8481052 [details] [diff] [review] to be landed before landing.
Depends on: 1059058
Comment on attachment 8483208 [details] [diff] [review]
Spin a new decoder if the appended data is discontinuous

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

Sorry, too hasty with the r+, thinking about this more it seems like we need a new decoder any time the new range overlaps the old range at all (not just with the 0.1 heuristic hack).  I'm curious why the current code is failing and I think making the 0.1 heuristic apply to more cases is not the right approach since there may be a more subtle bug here.  Maybe there's a case with a very small overlap that is causing a new decoder that doesn't with fabs()?  Need to dig more...

last = 0
range [    0,  2] append -> no action
range [ 1.95,  4] append -> overlap, new decoder
range [  3.8,  6] append -> overlap, new decoder
range [ 6.05,  8] append -> no action
range [  8.1, 10] append -> gap, new decoder
range [10.05, 12] append -> no action

Does this make sense?

Perhaps we need more data than just the parsed timestamps to make an accurate decision here.
Attachment #8483208 - Flags: review+
(Assignee)

Comment 10

5 years ago
(In reply to Matthew Gregan [:kinetik] from comment #9)
> Sorry, too hasty with the r+, thinking about this more it seems like we need
> a new decoder any time the new range overlaps the old range at all (not just
> with the 0.1 heuristic hack).  I'm curious why the current code is failing

On offline discussion with Matthew it looks like a combination of the attachment in bug 1061007 being different to the landed patch due to rebasing combined with merging of the attachment in bug 1059058 touching the same area of code reversed the fix that was in the landed patch. The landed patch seems to do the equivalent of what's here so marking as INVALID.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
(Assignee)

Updated

5 years ago
Blocks: 1041374
(Assignee)

Updated

5 years ago
Blocks: 1040552
You need to log in before you can comment on or make changes to this bug.