Remove buffered data on fast quality switches #113

alex-barstow · 2018-06-22T17:51:29Z

Description

This adds logic to remove all buffered data when a new playlist is enabled via Representation.enabled, such as with manual quality selection. Currently, the rendition switch following the enabling of a new playlist is slow due to the new segments being appended to the end of the buffer. This change comes following discussion with @gesinger, @forbesjo, and @mjneil. There was also some talk about modifying this conditional to check fetchAtBuffer_ to reduce delay before fetching new segments at the playhead. Let me know if you would like me to add that to this PR.

Specific Changes proposed

Rename the existing fastQualityChange_ function to smoothQualityChange_
Create new function called fastQualityChange_ which calls resetEverything() instead of just resetLoader()
Modify existing tests for the old fastQualityChange_ method to reflect the new function name
Add new test for the new fastQualityChange_ method

gkatsev · 2018-06-22T18:09:01Z

This would also fix videojs/video.js#5044, I believe.

mjneil · 2018-06-22T18:16:45Z

Did we want to put this behind a feature flag (either enabled or disabled by default) so that users can keep the smooth manual switch if they want?

forbesjo · 2018-07-05T17:37:00Z

In testing this PR it looks like there's a bit of a stutter when manually switching renditions. I'm expecting to see a loading spinner while rebuffering and smooth playback. Perhaps the stuttering happens when near the end of a segment?

Here's a gif of the behavior (BC skin)

alex-barstow · 2018-07-06T19:41:10Z

@forbesjo Hm I'll into that. Interestingly, Firefox handles the switch smoothly

alex-barstow · 2018-07-18T19:29:16Z

Pushed my changes. I believe the solution for the stutter is to seek in place after clearing the buffer. Chrome seems to cache a few frames from the previous rendition, and seeking in place gives the browser the "kick" it needs to clear them.

gesinger · 2018-07-23T21:41:52Z

src/master-playlist-controller.js

+   * @private
+   */
+  fastQualityChange_() {
+    let media = this.selectPlaylist();


minor: this can be const

gesinger · 2018-07-23T21:42:32Z

src/master-playlist-controller.js

+  fastQualityChange_() {
+    let media = this.selectPlaylist();
+
+    if (media !== this.masterPlaylistLoader_.media()) {


To save some indentation, we can change the condition to just return if (media === this.masterPlaylistLoader_.media())

gesinger · 2018-07-23T21:44:09Z

src/master-playlist-controller.js

+
+      // Wait for the buffer to finish clearing, then seek in place to give the
+      // browser a kick to remove any cached frames from the previous rendition
+      sourceBuffer.one('updateend', function() {


Might be better to use an arrow function here (can also remove the one time hls const above and can just use this.hls_.ignoreNextSeekingEvent_ = true below, and not have to bind setCurrentTime)

Depending on line length, also may not need to create the const for sourceBuffer

Same goes for the currentTime-- I think it can just be this.tech_.setCurrentTime(this.tech_.currentTime());

gesinger · 2018-07-23T21:57:37Z

src/master-playlist-controller.js

+
+      // Wait for the buffer to finish clearing, then seek in place to give the
+      // browser a kick to remove any cached frames from the previous rendition
+      sourceBuffer.one('updateend', function() {


Is it possible that this updateend will occur for a different action (e.g., an append)? Is it OK for it to run then?

@gesinger I suppose if we are in the middle of an append operation when the quality switch is initiated it might be possible? When we get the updateend we could check the segment loader state to make sure it isn't 'APPENDING'. Thoughts?

We might run into a timing issue if we are listening on the updateend but segment loader is also listening on updateend, as segment-loader can transition to another state before this function runs (it may not be the case right now, but to determine that MPC would need to have internal logic of the segment-loader state).

Optimally, we could avoid using the wrapped source buffer directly. It might make sense for segment-loader's resetEverything to take a callback for when it's finished. Then resetEverything can pass the callback along to source updater's remove function, and the remove function can execute it on updateend instead of a noop (as an optional parameter).

@gesinger Makes sense. Went ahead and made those changes so let me know how they look.

gesinger · 2018-07-23T22:04:12Z

test/master-playlist-controller.test.js

+
+  let segmentLoader = this.masterPlaylistController.mainSegmentLoader_;
+
+  segmentLoader.resyncLoader = function() {


minor: but for this (and below) we can save some lines using arrows, e.g., () => resyncs++

gesinger · 2018-07-23T22:05:04Z

test/master-playlist-controller.test.js

+
+  assert.deepEqual(removeFuncArgs, {start: 0, end: 60}, 'remove() called with correct arguments');
+
+  // verify stats


This kinda hung on in too many tests, but I think it's OK to remove here.

gesinger · 2018-07-23T22:05:36Z

test/master-playlist-controller.test.js

+  });
+
+  this.masterPlaylistController.selectPlaylist = () => {
+    return this.masterPlaylistController.master().playlists[0];


We may also want a test for not resetting if the media remains the same.

gesinger · 2018-08-07T20:26:45Z

src/master-playlist-controller.js

+    this.mainSegmentLoader_.resetEverything(() => {
+      // we want SegmentLoader.load() to be called only once the new playlist
+      // has finished loading. This prevents it from being called too soon.
+      this.hls_.ignoreNextSeekingEvent_ = true;


With the merge of #161 ignoreNextSeekingEvent_ was removed, and a new method of seeking was introduced. It's possible seekTo may be used directly and ignoreNextSeekingEvent_ removed.

@gesinger Hm just calling seekTo with the flag removed is problematic since the logic in MasterPlaylistController.setCurrentTime() results in SegmentLoader.load() getting called before the new playlist has finished loading, so we append another segment from the previous rendition before appending the new quality segments. Perhaps adding an another return in MPC.setCurrentTime() if the PlaylistLoader state is 'SWITCHING_MEDIA' would be a possible solution to this? Off the top of my head I'm not sure what other consequences that could have though.

If we can avoid looking at the PlaylistLoader state, that would be best. One thing that could be done here is use the normal this.tech_.setCurrentTime call directly, if we want to avoid the MasterPlaylistController.setCurrentTime() call, since seekTo is meant to add exactly that to the setCurrentTime calls. If we do use this.tech_.setCurrentTime directly though, it would be good to add a comment about why we can't go through the normal seekTo method (in this case to avoid resetting the loader, since it's a seek meant to kick things instead of actually performing a seek with buffer clearing).

gesinger · 2018-08-07T20:27:49Z

src/segment-loader.js

@@ -582,10 +584,12 @@ export default class SegmentLoader extends videojs.EventTarget {
   * Remove any data in the source buffer between start and end times
   * @param {Number} start - the start time of the region to remove from the buffer
   * @param {Number} end - the end time of the region to remove from the buffer
+   * @param {Function} done - an optional callback to be executed when the remove


For JSDoc optionals we can use [done]

gesinger · 2018-08-07T20:28:11Z

src/source-updater.js

@@ -131,14 +131,16 @@ export default class SourceUpdater {
   *
   * @param {Number} start where to start the removal
   * @param {Number} end where to end the removal
+   * @param {Function} done optional callback to be executed when the remove


For JSDoc optionals we can use [done], or even [done=noop] here

gesinger · 2018-08-07T20:29:41Z

test/master-playlist-controller.test.js

+
+  this.masterPlaylistController.fastQualityChange_();
+
+  assert.equal(segmentLoader.ended_, false, 'segment loader ended property is false');


Was there a specific reason for the ended check?

I included it since ended_ gets set to false as a side effect of calling resetEverything(). If you think it's superfluous I can remove it.

Probably don't need it here. While it's a decent test, since these tests are meant for MasterPlaylistController we can probably leave checking that state change within the SegmentLoader tests.

alex-barstow · 2018-08-09T17:47:11Z

Had to rebase to get the netlify deploy to succeed. @gesinger Ready for another look.

welcome · 2018-08-13T16:22:16Z

Congrats on merging your first pull request! 🎉🎉🎉

forbesjo self-assigned this Jul 5, 2018

gesinger reviewed Jul 23, 2018

View reviewed changes

forbesjo assigned gesinger Aug 7, 2018

gesinger reviewed Aug 7, 2018

View reviewed changes

alex-barstow added 8 commits August 9, 2018 13:31

add separate smooth and fast quality change functions WIP

43cc552

update test

74b6c12

fix stutter after fastQualityChange

4d496e3

some code review changes

b14d56f

code review changes

a3b766b

remove extra line

1aac103

remove use of ignoreNextSeekingEvent flag

afd8840

remove .ended_ check from test, add comment in fastQualityChange

59c7c3c

alex-barstow force-pushed the fast-quality-switch-nuclear-option branch from a23cf57 to 59c7c3c Compare August 9, 2018 17:38

gesinger approved these changes Aug 9, 2018

View reviewed changes

forbesjo approved these changes Aug 13, 2018

View reviewed changes

forbesjo merged commit bc94fbb into videojs:master Aug 13, 2018

alex-barstow deleted the fast-quality-switch-nuclear-option branch August 13, 2018 17:37

gkatsev mentioned this pull request Aug 13, 2018

Update @videojs/http-streaming to the latest version 🚀 videojs/video.js#5377

Merged

gesinger mentioned this pull request Aug 24, 2018

Revert "fix: Use middleware and a wrapped function for seeking instea… #203

Closed

alex-barstow mentioned this pull request Aug 28, 2018

fix: stutter after fast quality change in IE/Edge #213

Merged

squarebracket mentioned this pull request Sep 19, 2018

add back manual smooth quality changes via smoothQualityChange flag #235

Merged

6 tasks

squarebracket mentioned this pull request Oct 11, 2018

Fast quality change does not eject audio buffer when using multi-lingual stream #255

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove buffered data on fast quality switches #113

Remove buffered data on fast quality switches #113

alex-barstow commented Jun 22, 2018 •

edited

Loading

gkatsev commented Jun 22, 2018

mjneil commented Jun 22, 2018

forbesjo commented Jul 5, 2018

alex-barstow commented Jul 6, 2018

alex-barstow commented Jul 18, 2018 •

edited

Loading

gesinger Jul 23, 2018

gesinger Jul 23, 2018

gesinger Jul 23, 2018

gesinger Jul 23, 2018

alex-barstow Jul 31, 2018

gesinger Jul 23, 2018

alex-barstow Jul 31, 2018

gesinger Aug 2, 2018

alex-barstow Aug 6, 2018

gesinger Jul 23, 2018

gesinger Jul 23, 2018

gesinger Jul 23, 2018

gesinger Aug 7, 2018

alex-barstow Aug 7, 2018 •

edited

Loading

gesinger Aug 8, 2018

gesinger Aug 7, 2018

gesinger Aug 7, 2018

gesinger Aug 7, 2018

alex-barstow Aug 7, 2018

gesinger Aug 8, 2018

alex-barstow commented Aug 9, 2018

welcome bot commented Aug 13, 2018


		let segmentLoader = this.masterPlaylistController.mainSegmentLoader_;

		segmentLoader.resyncLoader = function() {


		assert.deepEqual(removeFuncArgs, {start: 0, end: 60}, 'remove() called with correct arguments');

		// verify stats


		this.masterPlaylistController.fastQualityChange_();

		assert.equal(segmentLoader.ended_, false, 'segment loader ended property is false');

Remove buffered data on fast quality switches #113

Remove buffered data on fast quality switches #113

Conversation

alex-barstow commented Jun 22, 2018 • edited Loading

Description

Specific Changes proposed

gkatsev commented Jun 22, 2018

mjneil commented Jun 22, 2018

forbesjo commented Jul 5, 2018

alex-barstow commented Jul 6, 2018

alex-barstow commented Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-barstow Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-barstow commented Aug 9, 2018

welcome bot commented Aug 13, 2018

alex-barstow commented Jun 22, 2018 •

edited

Loading

alex-barstow commented Jul 18, 2018 •

edited

Loading

alex-barstow Aug 7, 2018 •

edited

Loading