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

Status: RESOLVED INVALID

Description

[cajbir (:cajbir)]

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.

Comment 1

[cajbir (:cajbir)]

Created attachment 8483167 [details] [diff] [review]
Spin a new decoder if the appended data is discontinuous

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.

Comment 2

[Matthew Gregan [:kinetik]]

Bug 1059058 has had a fix for this since Friday.

Comment 3

[cajbir (:cajbir)]

(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

[Matthew Gregan [:kinetik]]

Sorry, in bug 1061007.

Comment 5

[cajbir (:cajbir)]

(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.

Comment 6

[cajbir (:cajbir)]

Created attachment 8483208 [details] [diff] [review]
Spin a new decoder if the appended data is discontinuous

Rebases on top of 1061007 and 1059058 changes.

Comment 7

[Matthew Gregan [:kinetik]]

Oh, <= != <, which I fixed later but haven't updated the patch for.  Too tired to read code properly.  fabs() is simpler.

Comment 8

[cajbir (:cajbir)]

Needs bug 1059058 attachment 8481052 [details] [diff] [review] to be landed before landing.

Comment 9

[Matthew Gregan [:kinetik]]

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.

Comment 10

[cajbir (:cajbir)]

(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.