Queue playback events when the playback rate is zero #5024

squarebracket · 2018-03-15T22:53:57Z

Description

SourceHandlers that use MSE have a problem: if they push a segment into a SourceBuffer and then seek close to the end, playback will stall and/or there will be a massive downswitch in quality. The general approach to fixing this that was discussed on slack was by setting the playback rate of the player to zero, buffering all that was required, and then restoring the previous playback rate. In my implementation, I've done this in the source handler (see: videojs/videojs-contrib-hls#1374).

From the video.js perspective, it should ensure that the UI reflects the buffering status and that the player API behaves like you'd expect -- that is to say, that it will fire seeking immediately after a call to currentTime, and it will fire seeked, canplay, canplaythrough, and playing when everything is buffered.

Specific Changes proposed

When the playback rate of the tech is zero, queue up callbacks for the seeked, canplay, canplaythrough, and playing events
When the playback rate is changed to something non-zero and its previous playback rate was zero, fire the queued callbacks

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

asd29696 · 2018-03-15T23:36:32Z

2018-03-16 6:54 GMT+08:00 Chuck Wilson <[email protected]>:

…

Description SourceHandlers that use MSE have a problem: if they push a segment into a SourceBuffer and then seek close to the end, playback will stall and/or there will be a massive downswitch in quality. The general approach to fixing this that was discussed on slack was by setting the playback rate of the player to zero, buffering all that was required, and then restoring the previous playback rate. In my implementation, I've done this in the source handler. From the video.js perspective, it should ensure that the UI reflects the buffering status and that the player API behaves like you'd expect -- that is to say, that it will fire seeking immediately after a call to currentTime, and it will fire seeked, canplay, canplaythrough, and playing when everything is buffered. Specific Changes proposed - When the playback rate of the tech is zero, queue up callbacks for the seeked, canplay, canplaythrough, and playing events - When the playback rate is changed to something non-zero and its previous playback rate was zero, fire the queued callbacks Requirements Checklist - Feature implemented / Bug fixed - If necessary, more likely in a feature request than a bug fix - Change has been verified in an actual browser (Chome, Firefox, IE) - Unit Tests updated or fixed - Docs/guides updated - Example created (starter template on JSBin <http://jsbin.com/axedog/edit?html,output>) - Reviewed by Two Core Contributors Missing I still need to edit/add tests and docs, but I'd like to get some opinions on this approach first. ------------------------------ You can view, comment on, or merge this pull request online at: #5024 Commit Summary - Queue playback events when the playback rate is zero File Changes - *M* src/js/player.js <https://github.com/videojs/video.js/pull/5024/files#diff-0> (37) Patch Links: - https://github.com/videojs/video.js/pull/5024.patch - https://github.com/videojs/video.js/pull/5024.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5024>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeI8rvH8ryBWZC1274OAo_ExrtfdbsmJks5tevEggaJpZM4StAPQ> .

gkatsev

Thanks for starting this @squarebracket. Made some comments.

gkatsev · 2018-03-16T14:33:07Z

src/js/player.js

@@ -985,6 +1002,7 @@ class Player extends Component {
    this.on(this.tech_, 'loadedmetadata', this.updateStyleEl_);
    this.on(this.tech_, 'posterchange', this.handleTechPosterChange_);
    this.on(this.tech_, 'textdata', this.handleTechTextData_);
+    this.on(this.tech_, 'ratechange', this.handleTechRateChange_);


if we want to run special handling, it needs to be removed from https://github.com/videojs/video.js/blob/master/src/js/player.js#L196 and the documentation needs to be moved to the handleTechRateChange_.

Moved the documentation.

gkatsev · 2018-03-16T14:34:51Z

src/js/player.js

+    Object.keys(TECH_EVENTS_QUEUE).forEach((event) => {
+      this.on(this.tech_, event, () => {
+        if (this.tech_.playbackRate() === 0) {
+          this.queuedCallbacks_.push(this[`handleTech${TECH_EVENTS_QUEUE[event]}_`].bind(this));


the event object should be passed through as well.

gkatsev · 2018-03-16T14:35:01Z

src/js/player.js

+          this.queuedCallbacks_.push(this[`handleTech${TECH_EVENTS_QUEUE[event]}_`].bind(this));
+          return;
+        }
+        this[`handleTech${TECH_EVENTS_QUEUE[event]}_`]();


event object should be passed through as well.

gkatsev · 2018-03-16T14:36:35Z

src/js/player.js

+
+    Object.keys(TECH_EVENTS_QUEUE).forEach((event) => {
+      this.on(this.tech_, event, () => {
+        if (this.tech_.playbackRate() === 0) {


I think this would be a breaking change. Potentially, doing it when isSeeking() is true and playbackRate is 0, might be OK.

Could you elaborate what would/could be breaking? A tech or source handler would have to both set the playback rate to zero and rely on these events being dispatched by the player (the tech itself will still dispatch them) right after the seek. Perhaps there's some use case I'm not thinking of.

I don't think that would work. Correct me if I'm wrong but I think seeking() is only true between the seeking and seeked events, and this needs to queue everything after and including the seeked event.

Because people could still expect these events even when playbackRate is 0. Granted, this is veery unlikely but I would still worry about making a change like that. Would be much nicer to section it off only for seeking().

Maybe we need to allow techs/source handlers dictate when seeking is happening? Looks like seeking() method just asks the video element directly:

video.js/src/js/tech/html5.js

Lines 1552 to 1564 in d7f45ba

/**

* Get the value of `seeking` from the media element. `seeking` indicates whether the

* media is currently seeking to a new position or not.

*

* @method Html5#seeking

* @return {boolean}

* - The value of `seeking` from the media element.

* - True indicates that the media is currently seeking to a new position.

* - Flase indicates that the media is not seeking to a new position at this time.

*

* @see [Spec]{@link https://www.w3.org/TR/html5/embedded-content-0.html#dom-media-seeking}

*/

'seeking',

video.js/src/js/tech/html5.js

Lines 1700 to 1704 in d7f45ba

].forEach(function(prop) {

Html5.prototype[prop] = function() {

return this.el_[prop];

};

});

but we could modify it so that sourcehandlers can handle it like duration and seekable?

video.js/src/js/tech/tech.js

Lines 1166 to 1173 in d7f45ba

/**

* When using a source handler, prefer its implementation of

* any function normally provided by the tech.

*/

const deferrable = [

'seekable',

'duration'

];

that way, we could gate it on player.seekable() and so any potential breakage would be minimal.

Looks like seeking() method just asks the video element directly

Yeah, that's why I said it wouldn't work. I like the idea of deferring seeking() to the source handlers. In fact, that should probably be done anyway to make the API correct -- i.e. you'd expect seeking() to be true up to the point a seeked is fired but that's not the case with the current code. I'll make that change and gate the event queueing to it.

Any other APIs you can think of that might need to be deferred to spoof things properly?

Not sure, but we've been deferring them as needed, we just haven't really needed any others until now, apparently.
Do you think that deferring to SH and then only queueing the events if seeking() works?

Yeah, that should work.

If you want to go with override-only behaviour, there's potentially another possibility. We could rejigger player.js so that most (all?) of the event handlers are dispatched on the player event rather than the tech event. And then have something like tech.delegateEvent(event). An undelegated event would then just be retriggered on the player, whereas a delegated event would be triggered on a source handler -- and expected to eventually be fired on the player manually be the source handler. I did a quick test and it worked as expected, but you obviously have a better idea about the bigger implications of such a change.

I think that deferring seeking() makes sense in this case.

I'm not sure I follow this 100%. If I understand correctly, the player would not listen for tech events but rather the tech would trigger the events directly on the player when necessary? If that is the case, I see two issues.

the tech and SH aren't supposed to have a reference to the player, so, they can't trigger an event on it.

triggering the event on the player directly would mean that the player doesn't necessarily have a chance to do something related to the event before others would receive the event, plus, if the player would to re-trigger the event, it would end up in an event loop.

This also does lend itself a bit to what middleware strives to be and we've been thinking/wanting to have events be able to be intercepted by mw as well, so, maybe we can do that eventually. I think that deferring seeking() is the quick solution and a decent solution to this.

Yeah, I was talking about removing this.trigger to avoid the event loop, but you got the gist of what I was going for.

Cool. Yeah, I think that's a bit more complicated and would require a lot of though to get it right.

gkatsev · 2018-03-19T20:41:10Z

Also, this probably needs to be changed to be a PR against 6.x, master will soon be a 7.x-only. Or you can leave it against master and we can backport it later on.

squarebracket · 2018-03-20T15:24:13Z

I'll have to open the PR against any VJS branch that contrib-hls/VHS supports, since the PR there will require the change in video.js.

gkatsev · 2018-03-29T20:56:41Z

test/unit/player.test.js

@@ -1364,6 +1364,49 @@ QUnit.test('Remove waiting class on timeupdate after tech waiting', function(ass
  player.dispose();
 });

+QUnit.test('Queues playing events when playback rate is zero', function(assert) {


should we have a test to verify that events aren't queued if the playbackRate was changed and we're not seeking?

Might as well. Added tests.

gkatsev · 2018-03-29T21:11:07Z

src/js/player.js

+        if (this.tech_.playbackRate() === 0 && this.tech_.seeking()) {
+          this.queuedCallbacks_.push({
+            callback: this[`handleTech${TECH_EVENTS_QUEUE[event]}_`].bind(this),
+            event


the event that we want to cache is actually event object that's passed to the handler (the argument that would be passed into this arrow function), not the event name

Woops! That one slipped right by me, thanks for catching.

gkatsev · 2018-03-29T21:43:36Z

Just tested it, works great. Brings us one step closer to natively seeking via playbackrate 0, too

misteroneill · 2018-04-02T19:51:42Z

src/js/player.js

+      this.queuedCallbacks_.forEach((queued) => queued.callback(queued.event));
+      this.queuedCallbacks_ = [];
+    }
+    this.previousPlaybackRate = this.tech_.playbackRate();


Should this be pseudo-private (i.e. previousPlaybackRate_)?

totally missed that, yes, it should. Could even potentially be stored on the cache_ object as well/instead.

Changed to this.cache_.lastPlaybackRate

squarebracket · 2018-04-03T17:54:18Z

src/js/player.js

@@ -389,6 +387,9 @@ class Player extends Component {

    this.el_ = this.createEl();

+    // Set default value for lastPlaybackRate
+    this.cache_.lastPlaybackRate = this.defaultPlaybackRate();


Is it appropriate to use defaultPlaybackRate here? i.e. will everything be set up properly at this point? Since the element exists, I made the assumption that it would.

some techs, namely flash, don't support playbackrate. However, if it isn't supported or the tech isn't ready, it'll just return the default value of 1.0, so, this is probably fine.

video.js/src/js/player.js

Lines 3090 to 3099 in 4e79a04

defaultPlaybackRate(rate) {

if (rate !== undefined) {

return this.techCall_('setDefaultPlaybackRate', rate);

}

if (this.tech_ && this.tech_.featuresPlaybackRate) {

return this.techGet_('defaultPlaybackRate');

}

return 1.0;

}

squarebracket · 2018-04-03T17:56:20Z

LMK if there's somewhere in the docs these changes should be reflected.

gkatsev · 2018-04-04T19:45:02Z

It doesn't seem like we have any docs for source handlers, so, not really any place to put it currently.

gkatsev

👍

gkatsev · 2018-04-04T20:11:39Z

@forbesjo brought up that when contrib-hls is updated with this, what happens if a user is using a version of Video.js without this change? Would this make the contrib-hls update a breaking change or can we make the contrib-hls backwards compatible?

squarebracket · 2018-04-04T22:10:42Z

I figured it would require a bump in the dependency version. So we'd wait until this change is merged in and released, and then the changes for contrib-hls would be merged in with a change to its package.json.

Is that a problem? I imagine that if you're updating contrib-hls, it's probably not that big of a deal to update video.js as well.

Otherwise we'll have to do some feature detection. A tech.supportsProperSeek or some such thing.

gkatsev · 2018-04-04T22:15:14Z

it means that the minimum supported version of video.js for contrib-hls is increased, which is probably a breaking change. It's probably fine to do, but we'd want to make this choice explicitly.

squarebracket · 2018-04-05T02:37:39Z

Care to elaborate what you mean by making the choice explicitly?

gkatsev · 2018-04-05T04:54:53Z

As in, we should decide what to do in this case, from making a breaking change on contrib-hls, or just upping the minimum requires video.js version in a minor, to making contrib-hls work with or without this change (with degraded behavior without this change). What we shouldn't do is just merge and release and let the cards fall where they may. Though, choosing that could be an option too.

forbesjo · 2018-04-05T15:31:56Z

Upon further discussion we'll make it very clear in the contrib-hls README and package.json which version of video.js this change is compatible with.

… seeking (#5024) SourceHandlers that use MSE have a problem: if they push a segment into a SourceBuffer and then seek close to the end, playback will stall and/or there will be a massive downswitch in quality. The general approach to fixing this that was discussed on slack was by setting the playback rate of the player to zero, buffering all that was required, and then restoring the previous playback rate. In my implementation, I've done this in the source handler (see: videojs/videojs-contrib-hls#1374). From the video.js perspective, it should ensure that the UI reflects the buffering status and that the player API behaves like you'd expect -- that is to say, that it will fire seeking immediately after a call to currentTime, and it will fire seeked, canplay, canplaythrough, and playing when everything is buffered.

… seeking (#5061) 6.x version of #5024 SourceHandlers that use MSE have a problem: if they push a segment into a SourceBuffer and then seek close to the end, playback will stall and/or there will be a massive downswitch in quality. The general approach to fixing this that was discussed on slack was by setting the playback rate of the player to zero, buffering all that was required, and then restoring the previous playback rate. In my implementation, I've done this in the source handler (see: videojs/videojs-contrib-hls#1374). From the video.js perspective, it should ensure that the UI reflects the buffering status and that the player API behaves like you'd expect -- that is to say, that it will fire seeking immediately after a call to currentTime, and it will fire seeked, canplay, canplaythrough, and playing when everything is buffered.

gkatsev · 2018-04-17T19:30:22Z

Thanks @squarebracket!

Queue playback events when the playback rate is zero

bb4ea18

squarebracket mentioned this pull request Mar 15, 2018

Improved seek behaviour videojs/videojs-contrib-hls#1374

Closed

6 tasks

gkatsev reviewed Mar 16, 2018

View reviewed changes

fixes

2f2c2b1

adding tests

c94e136

squarebracket changed the title ~~WIP: Queue playback events when the playback rate is zero~~ Queue playback events when the playback rate is zero Mar 26, 2018

gkatsev reviewed Mar 29, 2018

View reviewed changes

squarebracket added 2 commits March 29, 2018 17:12

include test ensuring that events are dispatched when not seeking

e439723

dispatch the actual event object

0d234d6

gkatsev approved these changes Mar 29, 2018

View reviewed changes

squarebracket mentioned this pull request Mar 30, 2018

Feature/queue playback events for zero playback rate (vjs6) #5061

Merged

tiny doc update

6d5eef4

squarebracket mentioned this pull request Mar 30, 2018

queue playback events when playback rate is zero and seeking is true (vjs5) #5062

Closed

misteroneill reviewed Apr 2, 2018

View reviewed changes

cache lastPlaybackRate

5dab218

squarebracket commented Apr 3, 2018

View reviewed changes

gkatsev approved these changes Apr 4, 2018

View reviewed changes

brandonocasey approved these changes Apr 11, 2018

View reviewed changes

gkatsev merged this pull request into videojs:master Apr 17, 2018

squarebracket mentioned this pull request Apr 17, 2018

fix test for ie8 #5104

Closed

7 tasks

gkatsev mentioned this pull request Apr 23, 2018

Changelog for 7.0.0? #5119

Closed

squarebracket mentioned this pull request Jan 11, 2019

Improved seek behaviour videojs/http-streaming#370

Closed

6 tasks

	/**
	* Get the value of `seeking` from the media element. `seeking` indicates whether the
	* media is currently seeking to a new position or not.
	*
	* @method Html5#seeking
	* @return {boolean}
	* - The value of `seeking` from the media element.
	* - True indicates that the media is currently seeking to a new position.
	* - Flase indicates that the media is not seeking to a new position at this time.
	*
	* @see [Spec]{@link https://www.w3.org/TR/html5/embedded-content-0.html#dom-media-seeking}
	*/
	'seeking',

	].forEach(function(prop) {
	Html5.prototype[prop] = function() {
	return this.el_[prop];
	};
	});

	/**
	* When using a source handler, prefer its implementation of
	* any function normally provided by the tech.
	*/
	const deferrable = [
	'seekable',
	'duration'
	];

	defaultPlaybackRate(rate) {
	if (rate !== undefined) {
	return this.techCall_('setDefaultPlaybackRate', rate);
	}

	if (this.tech_ && this.tech_.featuresPlaybackRate) {
	return this.techGet_('defaultPlaybackRate');
	}
	return 1.0;
	}

Queue playback events when the playback rate is zero #5024

Queue playback events when the playback rate is zero #5024

Conversation

squarebracket commented Mar 15, 2018 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

asd29696 commented Mar 15, 2018 via email

gkatsev left a 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

gkatsev Mar 16, 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

gkatsev commented Mar 19, 2018

squarebracket commented Mar 20, 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

gkatsev commented Mar 29, 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

squarebracket commented Apr 3, 2018

gkatsev commented Apr 4, 2018

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev commented Apr 4, 2018

squarebracket commented Apr 4, 2018

gkatsev commented Apr 4, 2018

squarebracket commented Apr 5, 2018

gkatsev commented Apr 5, 2018

forbesjo commented Apr 5, 2018

gkatsev commented Apr 17, 2018

squarebracket commented Mar 15, 2018 •

edited

Loading

gkatsev Mar 16, 2018 •

edited

Loading