Fix #240

[davemevans]

This PR is a workaround issue #240. For more detail, see @LloydW93's comments on #633.

It's not perfect and in the long run we would expect there to be a better solution when a general clean up of fragment loading occurs, but at least seeking actually works when this patch is applied which we think is important for v1.5.

[bwidtmann]

Can you please explain this 3 seconds. Is it related to the chunk duration? Or is it for edge cases?

[bwidtmann]

Hi @bbcrddave and @LloydW93

your provided alternate fix looks cleaner than ours (code wise), but when I was pulling your branch, I could still reproduce the original problem. I will describe the test case in detail:

1. use a stream with high resolution in order to reach the internal buffer limit easily (1080px or 4k)
2. start the stream and ensure highest quality level is appended to source buffer (10Mbps or higher internet connection)
3. seek 10-20 times until browser starts to prune the first appended chunks on its own silently (check video elements buffer in browser console)
4. seek back to the start (at 0 seconds; only seek back when you really notice a buffer pruning in browser console(!))
5. playback freezes or continues at wrong position

That is the reason why we decided to go for the "prune the buffer on our own" solution as we only experienced bullet proofed stability if we never let the browser run into the buffer limit situation.

[LloydW93]

Hey @bwidtmann, thanks for the feedback on this!

The 3 seconds thing is actually a bad hack that I completely forgot about - it's there to handle the situation where a fragment has been downloaded, but is in dash.js' internal queue to be appended to the SourceBuffer. I'm going to take another look at this and replacing it with querying the virtual buffer instead of the media element directly, if possible.

With the reproduction case, that's really interesting. I don't believe this is the case if you seek a short while into the content after that space has been pruned, so there is possibly a bug specifically around currentTime==0 case.

I understand and agree with the general rationale of pruning the buffer on our own so that we have full control over these cases - I was just hoping (perhaps naively) that we should be able to expect browsers to perform in a vaguely predictable manner. Looking closely at both this PR and #633, there is nothing to suggest these are mutually exclusive changes - and indeed I think there are situations each cover which the other doesn't.

[bwidtmann]

merging both PRs is a quite good solution I never thought about 👍

regarding the reproducing: I just tried to seek back to another already appended time other than 0

$('video').get(0).currentTime = 12

but playback froze as well :-(

what I experienced that the function isDiscarded never returns true when I put a console.log there

[davemevans]

@AkamaiDASH Please hold off merging this for now as we still need to make a couple more changes to address @bwidtmann's comments. We should have it sorted early next week.

[dsparacio]

👍 - If it is much later then Monday then maybe needs to be voted on to get into 1.5 depending on the RC at the time...... Sure it will be fine. We just have this hard date this time with IBC...

[LloydW93]

I've done some more playing with this lately, with some other changes I've made to make it more robust (they haven't yet been added to the branch this PR references). However, leaving the browser to handle buffer pruning once you look through and handle the other issues around it, in Chrome you start to encounter http://crbug.com/421694 - it will decide to evict buffer at the current playhead position so that it can append the next segment.
I think it may be best to hold off on this in terms of 1.5 until I have some more time to play with it (and hopefully Chrome merges the fixes mentioned in that bug). #633 works rather well as a solution in most cases; I disagree with having to manually prune the buffer ahead of what any browser would reasonably do, but it currently looks like in most cases that's the only option.

[dsparacio]

Pulling out of 1.5 release. Revisit in next release. Merged #747

[davemevans]

I'm going to close this PR for now. Will open a new one once we have decided on the best solution.