Closed
Bug 1062023
Opened 7 years ago
Closed 7 years ago
An append of SourceBuffer data that is into the future results in wrong buffered data
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
INVALID
People
(Reporter: cajbir, Assigned: cajbir)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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•7 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•7 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•7 years ago
|
Comment 2•7 years ago
|
||
Bug 1059058 has had a fix for this since Friday.
Assignee | ||
Comment 3•7 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.
Comment 4•7 years ago
|
||
Sorry, in bug 1061007.
Assignee | ||
Comment 5•7 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•7 years ago
|
||
Rebases on top of 1061007 and 1059058 changes.
Attachment #8483167 -
Attachment is obsolete: true
Comment 7•7 years ago
|
||
Oh, <= != <, which I fixed later but haven't updated the patch for. Too tired to read code properly. fabs() is simpler.
Updated•7 years ago
|
Attachment #8483208 -
Flags: review+
Assignee | ||
Comment 8•7 years ago
|
||
Needs bug 1059058 attachment 8481052 [details] [diff] [review] to be landed before landing.
Depends on: 1059058
Comment 9•7 years ago
|
||
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•7 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
Closed: 7 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•