Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix seeking issue #2973

Conversation

bbert
Copy link
Contributor

@bbert bbert commented Apr 11, 2019

In BufferController, when seeking, a segment may being appended into the buffer.
Therefore if we do prune the buffer before the current segment appending process is completed, then the range to be removed is erroneous (does not take into account current segment being appended).
This can have side effects and leads to playback freeze.
On some devices, appending process can take a relatively long time and can more easily highlight this issue.

This PR attempts to fix this issue by waiting for segment appending process to be completed before pruning the buffer at seeking.

@epiclabsDASH epiclabsDASH added this to the v3.0 milestone Apr 16, 2019
@epiclabsDASH
Copy link
Contributor

Totally makes sense, @bbert. I think we should do the same in the part of the code where we are managing quota exceed situations, isn't it?

Alternatively, to cover both cases and possible future ones, we could move the checking business logic within pruneAllSafely method. What do you think?

@bbert
Copy link
Contributor Author

bbert commented Apr 25, 2019

@epiclabsDASH Yes, this can be achieved this way indeed, but this requires having sourceBufferSink::waitForUpdateEnd() method to be public.
I have modified the PR this way, let me know if it suits

@epiclabsDASH
Copy link
Contributor

I was talking about moving this logic within pruneAllSafely()

if (buffer.getBuffer().updating) {
   seeking = true;
} else {
   pruneAllSafely();
}

Anyway, your approach looks ok to me. Requires BufferController to know it should wait for pruning all buffer but implementation overall implementation looks cleaner.

Just one last thing. We need to add the waitForUpdateEnd method also to the PreBufferSink class.

@bbert
Copy link
Contributor Author

bbert commented Apr 25, 2019

OK, I've missed the PreBufferSink.
I have updated the PR but I did not manage to test the PreBufferSink. I tried the getting-started/pre-load-video sample, but it does not fetch any segment, it stops after manifest is loaded. Is this the expected behavior? I have never tried preload functionnality before.

@epiclabsDASH
Copy link
Contributor

Looks ok to me!

Regarding prefetch, there is a pending task to improve how it works (from the UI/interface point of view) so you don't need to click two times to attach button of the demo to start playback. Anyway, not related with your change at all.

Thanks!

@epiclabsDASH epiclabsDASH merged commit f292782 into Dash-Industry-Forum:development Apr 26, 2019
@nicosang nicosang deleted the seeking-while-appending branch April 26, 2019 13:10
jeffcunat pushed a commit to Orange-OpenSource/dash.js that referenced this pull request May 13, 2019
BufferController: when seeking, prune buffer only once current segment appending is achieved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants