Add safe margin parameter to avoid rebuffering interruptions when clearing the buffer #1154

wasp898 · 2017-11-24T11:08:56Z

The goal of this PR is to introduce an optional parameter that allows the buffer to be cleared but not entirely.

The idea is to keep a safe margin that avoids rebuffering events but allows for fast switches. The value of this parameter should be computed and provided by a custom ABR manager. Nothing changes if the value is not provided of course (e.g. SimpleAbrManager).

Since the buffer could be busy, another parameter had to be introduced to save the value of the safe margin when the call to clear the buffer is deferred to a later update.

Something similar was present in Shaka v1, but it was removed with the redesign.

googlebot · 2017-11-24T11:08:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

wasp898 · 2017-11-24T11:11:03Z

I signed it!

googlebot · 2017-11-24T11:11:06Z

CLAs look good, thanks!

joeyparrish · 2017-12-01T18:02:09Z

The trouble with this is that clearing media ahead of the playhead causes stuttering issues on some browsers. That is why we don't stopped doing that in v2.

Have you tested this on multiple browsers? Which browsers does your application intend to support?

wasp898 · 2017-12-05T16:43:25Z

Thanks a lot for your reply.

These few days I've spent some time testing it on every browser for desktop. Issues occur when you clear too close to the playhead. As long as the margin is computed appropriately, I don't see any artifect or stuttering. Playback goes smoothly.

Here's what I tried to do in detail. After computing (approximately) the next segment boundary, I tried clearing from that point on when switching variant. This caused big problems already (because of a lack of accuracy probably), so I tried adding 0.2 seconds to such margin, and it was enough to solve the issue on most browsers except mainly for Chrome. Finally, I tried to increase the margin by adding the duration of a segment, and I stopped getting stuttering altogether. As long as the network state is good though. I mean, the only stutters that I noticed have happened because of buffer depletion, but that's a different issue.

Bottom line is: I think two segments worth of buffer is a safe minimum to consider when computing the margin; and then the final value should be computed according to the bandwidth estimate I guess, to avoid depletion.

I have found the commit where the support was dropped: 385a501

p.s. I've tested with Chrome, Firefox (52 and 57), Safari, Opera, Edge, IE 11. I also tried different segment sizes: 1, 4 and 6 seconds. Tested under good network conditions, mainly. Some tests under bandwidth cap as well.

joeyparrish · 2017-12-05T18:35:52Z

Thank you for doing such detailed testing. At this point, our plates are pretty full for the rest of the year to finish v2.3.0. We will look into this PR in January when we start work on v2.4.0. Please accept our apologies for the delay, and thank you for your contribution!

wasp898 · 2017-12-05T19:08:57Z

Thank you!

TheModMaker

Are you still interested in this PR? If so please rebase on the latest master and fix any style issues with our new style requirements (run build/all.py).

TheModMaker · 2018-08-01T21:49:32Z

lib/media/streaming_engine.js

@@ -991,7 +1002,8 @@ shaka.media.StreamingEngine.prototype.onUpdate_ = function(mediaState) {
  if (mediaState.waitingToClearBuffer) {
    // Note: clearBuffer_() will schedule the next update.
    shaka.log.debug(logPrefix, 'skipping update and clearing the buffer');
-    this.clearBuffer_(mediaState, mediaState.waitingToFlushBuffer);
+    this.clearBuffer_(mediaState, mediaState.waitingToFlushBuffer,
+        mediaState.deferredClearBufferOffset);


The style is to keep all the arguments aligned. So please do either:

this.clearBuffer_(mediaState, mediaState.waitingToFlushBuffer, mediaState.deferredClearBufferOffset);

Or

this.clearBuffer_( mediaState, mediaState.waitingToFlushBuffer, mediaState.deferredClearBufferOffset);

wasp898 · 2018-08-02T08:22:27Z

Yes, absolutely, still interested! I'll get to work asap, thanks.

…ufferings. A new optional parameter called safeMargin is introduced to avoid clearing the buffer entirely. The value of such offset should be computed in a custom ABR manager and provided to the player and the streaming engine. Since the buffer could be busy when clearBuffer is called, another parameter had to be introduced to save the value of the safe margin when the call to clear the buffer is deferred to a later update. When a safe margin value is provided, the clearBuffer method uses ‘remove’ as method to clear the buffer, and not the method ‘clear’.

…umber.

wasp898 · 2018-08-06T14:34:58Z

Okay, I rebased and fixed the style related issues as requested. No conflicts, and build went fine.

TheModMaker · 2018-08-06T15:28:01Z

lib/media/streaming_engine.js

  p.then(function() {
-    if (!this.destroyed_ && flush) {
+    if (!this.destroyed_ && flush && safeMargin == 0) {


Since this is dependent on safeMargin == 0, you could put this as part of line 2191.

What did you have in mind here? Not sure how to move this without duplicating things after.

p.then(function() { if (!this.destroyed_ && flush && safeMargin == 0) { return this.playerInterface_.mediaSourceEngine.flush(mediaState.type); } }.bind(this)).then(function() { if (this.destroyed_) return; shaka.log.debug(logPrefix, 'cleared buffer'); mediaState.lastStream = null; mediaState.lastSegmentReference = null; mediaState.clearingBuffer = false; mediaState.endOfStream = false; this.scheduleUpdate_(mediaState, 0); }.bind(this));

if (safeMargin) { p = ... } else { p = this.playerInterface_.mediaSourceEngine.clear(mediaState.type).then(() => { if (!this.destroyed_ && flush) return this.playerInterface_.mediaSourceEngine.flush(mediaState.type); }); } p.then(() => { if (this.destroyed_) return; shaka.log.debug(logPrefix, 'cleared buffer'); ... });

Oh okay, got it, thanks!

TheModMaker · 2018-08-06T15:30:17Z

lib/media/streaming_engine.js

@@ -237,6 +237,7 @@ shaka.media.StreamingEngine.PlayerInterface;
 *   updateTimer: ?number,
 *   waitingToClearBuffer: boolean,
 *   waitingToFlushBuffer: boolean,
+ *   deferredClearBufferOffset: number,


I think a better name would be clearBufferSafeMargin, or something including safeMargin, to be consistent.

TheModMaker · 2018-08-06T15:31:11Z

lib/player.js

@@ -133,6 +133,9 @@ shaka.Player = function(video, dependencyInjector) {
  /** @private {boolean} */
  this.deferredVariantClearBuffer_ = false;

+  /** @private {number} */
+  this.deferredVariantClearBufferOffset_ = 0;


"Deferred" is fine here, but again, I think it should include safeMargin instead of offset.

TheModMaker · 2018-08-06T15:32:30Z

lib/player.js

@@ -1640,9 +1643,11 @@ shaka.Player.prototype.usingEmbeddedTextTrack = function() {
 *
 * @param {shaka.extern.Track} track
 * @param {boolean=} clearBuffer
+ * @param {number=} safeMargin


This deserves a comment for what this will do. You should also include a part about browser compatibility for having this value be too small.

TheModMaker · 2018-08-06T15:37:26Z

lib/player.js

 * @private
 */
-shaka.Player.prototype.switch_ = function(variant, clearBuffer = false) {
+shaka.Player.prototype.switch_ = function(
+    variant, clearBuffer = false, safeMargin = 0) {


This is the callback from AbrManager. If you want to change this, you'll need to change the definitions in externs/shaka/abr_manager.js and in SimpleAbrManager to match the new function signature.

In SimpleAbrManager I see that the call to the switch function only passes the variant, without the optional parameters. I kept it this way. Is that okay?
I pushed the other modifications, including the updated abr manager extern.

That's fine.

TheModMaker · 2018-08-06T15:39:50Z

CONTRIBUTORS

@@ -33,6 +33,7 @@ Donato Borrello <[email protected]>
 Duc Pham <[email protected]>
 Esteban Dosztal <[email protected]>
 François Beaufort <[email protected]>
+Giuseppe Samela <[email protected]>


You should also add an entry to AUTHORS.

TheModMaker

Looks good to me. Let me run it by the build bot.

TheModMaker · 2018-08-09T16:17:35Z

lib/player.js

 * @private
 */
-shaka.Player.prototype.switch_ = function(variant, clearBuffer = false) {
+shaka.Player.prototype.switch_ = function(
+    variant, clearBuffer = false, safeMargin = 0) {


That's fine.

shaka-bot · 2018-08-09T16:32:49Z

All tests passed!

joeyparrish self-assigned this Dec 1, 2017

joeyparrish added the type: enhancement New feature or request label Dec 1, 2017

joeyparrish added this to the Backlog milestone Dec 1, 2017

joeyparrish modified the milestones: Backlog, v2.4.0 Dec 5, 2017

joeyparrish modified the milestones: v2.4, v2.5 Mar 4, 2018

joeyparrish removed their assignment Mar 4, 2018

TheModMaker reviewed Aug 1, 2018

View reviewed changes

TheModMaker added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Aug 1, 2018

wasp898 added 4 commits August 6, 2018 13:31

Fixed calls to switchInternal_ to reflect that the safe margin is a n…

c265c00

…umber.

Fixed errors that made the build fail.

4f6316f

Fix unit test files + update contributors list

50260b6

wasp898 force-pushed the master branch from 97f1028 to 50260b6 Compare August 6, 2018 11:38

wasp898 added 3 commits August 6, 2018 15:29

Refactor optional parameters and comply with style requirements

8bfd113

Fix same margin type issue

0270ac9

Fix styling issue in player.js and streaming_engine_unit

cc7b0a5

TheModMaker reviewed Aug 6, 2018

View reviewed changes

Rename deferred safe margin variable

e4b1880

TheModMaker approved these changes Aug 9, 2018

View reviewed changes

TheModMaker removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Aug 9, 2018

TheModMaker merged commit fed7166 into shaka-project:master Aug 9, 2018

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add safe margin parameter to avoid rebuffering interruptions when clearing the buffer #1154

Add safe margin parameter to avoid rebuffering interruptions when clearing the buffer #1154

wasp898 commented Nov 24, 2017

googlebot commented Nov 24, 2017

wasp898 commented Nov 24, 2017

googlebot commented Nov 24, 2017

joeyparrish commented Dec 1, 2017

wasp898 commented Dec 5, 2017

joeyparrish commented Dec 5, 2017

wasp898 commented Dec 5, 2017

TheModMaker left a comment

TheModMaker Aug 1, 2018

wasp898 commented Aug 2, 2018

wasp898 commented Aug 6, 2018

TheModMaker Aug 6, 2018

wasp898 Aug 7, 2018

TheModMaker Aug 7, 2018

wasp898 Aug 7, 2018

TheModMaker Aug 6, 2018

TheModMaker Aug 6, 2018

TheModMaker Aug 6, 2018

TheModMaker Aug 6, 2018

wasp898 Aug 8, 2018

TheModMaker Aug 9, 2018

TheModMaker Aug 6, 2018

TheModMaker left a comment

TheModMaker Aug 9, 2018

shaka-bot commented Aug 9, 2018

Add safe margin parameter to avoid rebuffering interruptions when clearing the buffer #1154

Add safe margin parameter to avoid rebuffering interruptions when clearing the buffer #1154

Conversation

wasp898 commented Nov 24, 2017

googlebot commented Nov 24, 2017

wasp898 commented Nov 24, 2017

googlebot commented Nov 24, 2017

joeyparrish commented Dec 1, 2017

wasp898 commented Dec 5, 2017

joeyparrish commented Dec 5, 2017

wasp898 commented Dec 5, 2017

TheModMaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasp898 commented Aug 2, 2018

wasp898 commented Aug 6, 2018

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

TheModMaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaka-bot commented Aug 9, 2018