fix: Fix cases where the player reports that it is ready during source selection, but before tech selection. #4665

misteroneill · 2017-10-16T19:30:07Z

In working on some issues with asynchronous middleware and playlists, I realized that there is a period of time during the source and tech setting process where a new source and tech have not been selected, but the player still considers itself "ready".

The effect of this is to cause any callbacks to ready() during source selection to be called immediately instead of waiting for the source/tech to be selected:

player.src({src: 'foo.mp4', type: 'video/mp4'});
player.ready(() => {
  player.play();
});

I think most people would expect, in that case, foo.mp4 to begin playback. However, if there is asynchronous middleware happening, what you may get is some playback of the current source and then a source change to foo.mp4 (which will pause the player).

The essence of this change is to separate the player's readiness from the tech's readiness. The tech can be ready when the player is not because the player is still resolving middleware, etc.

This should be backward-compatible and make ready() more reliable and better match user expectations.

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

gkatsev · 2017-10-16T19:46:15Z

src/js/player.js

-      });
+    // Given a return value for a tech's `play()` method, silences "unhandled
+    // promise from play" errors.
+    const silencePromise = (value) => {


we probably want a promise helper file that contains isPromise and silencePromise, since we silence promises in a few places.

gkatsev · 2017-10-16T19:48:14Z

src/js/player.js

+    // Finally, if the player is ready, but we don't have a source, wait for
+    // one to be set.
+    } else {
+      this.ready(() => this.tech_.one('loadstart', () => silencePromise(this.play())));


This will fail because the function gets called with the context of the player and not the tech as relied on previously.

gkatsev · 2017-10-16T19:49:03Z

src/js/player.js

-        // we could not find an appropriate tech, but let's still notify the delegate that this is it
-        // this needs a better comment about why this is needed
+        // we could not find an appropriate tech, but let's still notify the
+        // delegate that this is it this needs a better comment about why this


can you revert this change or put a new line in? Otherwise the two statements run in together.

Ah, yes, it is hard to distinguish sentences without capitals and punctuation. ;)

misteroneill · 2017-10-17T18:01:55Z

test/unit/tracks/text-tracks.test.js

@@ -574,7 +574,7 @@ if (Html5.isSupported()) {
    player.addRemoteTextTrack(track, false);
    player.src({src: 'example.mp4', type: 'video/mp4'});

-    this.clock.tick(1);
+    this.clock.tick(2);


Calling src() now results in two async functions being queued up in sequence. First, middleware.setSource runs async. Then, its callback invokes this.triggerReady(), which queues up a flush of the ready queue.

That's why this changed from 1 to 2.

Im just wondering if this is going to break plugin tests, I don't really see an issue other that that.

misteroneill · 2017-10-17T18:02:51Z

test/unit/player.test.js

    loadTech_() {
      loadTechCalled++;
    }
  };

-  Player.prototype.src_.call(playerProxy);
+  Player.prototype.src_.call(playerProxy, {});


The changes to this test were necessary because src_() no longer calls ready(). Instead, it immediately loads the source into the tech because the only cases where this is possible is if the tech did not change; so, it should already be ready.

misteroneill · 2017-10-17T18:04:22Z

src/js/player.js

-    // middlewareSource is the source after it has been changed by middleware
+    // Set the player to a non-ready state while middleware asynchronously
+    // determines the final source.
+    this.isReady_ = false;


This is the key fix: the player should not be "ready" while middleware is resolving a source and deciding if it needs to load a new tech or set the resolved source on the existing tech.

brandonocasey · 2017-10-17T18:22:11Z

test/unit/tracks/text-tracks.test.js

@@ -574,7 +574,7 @@ if (Html5.isSupported()) {
    player.addRemoteTextTrack(track, false);
    player.src({src: 'example.mp4', type: 'video/mp4'});

-    this.clock.tick(1);
+    this.clock.tick(2);


Im just wondering if this is going to break plugin tests, I don't really see an issue other that that.

brandonocasey · 2017-10-17T18:23:03Z

src/js/player.js

   */
  play() {
-    if (this.changingSrc_) {


Having this function sometimes return a promise just sucks, not really something we can/should do about it in this PR though.

I dunno. I mean, it does model the native play method. The alternative would be to include a Promise polyfill.

I do think making a Promise is what we'd want to do eventually. Though, we'd probably ask users to provide a polyfill rather than include it in video.js. Then if Promises exists or was passed in we'll make one.
Either way, outside the scope of this PR :)

gkatsev

Would be great to make a test for the originally breaking condition.

gkatsev · 2017-10-18T17:56:39Z

@videojs/core-committers any thoughts or comments regarding this change?

gkatsev

Changes seem good to go. Should be tested first, though.

)

…ete: see videojs/video.js#4665 and videojs/generator-videojs-plugin@87f0d08

gkatsev · 2017-10-25T18:49:45Z

removing confirmed until this is tested so we don't merge it in early.

gkatsev · 2017-10-31T19:19:44Z

I just tested this with videojs-playlist and autoadvance is broken with this but works with the released version 6.3.3. We might want to re think about this taking the sourceset PR into account.

alex-barstow · 2017-11-01T15:30:13Z

@gkatsev I saw the same playlist behavior when re-testing to see if #4639 is still necessary. Autoadvance didnt work unless I wrapped silencePromise(this.techGet_('play')) in a timeout of some arbitrary amount (can't recall exact wait time), so perhaps the player is still reporting it is ready too early.

…e selection, but before tech selection.

misteroneill · 2017-11-02T19:34:37Z

@alex-barstow is correct. What's happening is that we're calling triggerReady when Video.js has updated its source, but the tech/video element still has the old source.

misteroneill · 2017-11-03T14:33:54Z

I'm not sure this is going to work, but I'm going to gather some thoughts here, partly to get my head on straight.

The current and previous assumption within Video.js has been that the player's ready state is identical to the tech's ready state.

This PR aims to adjust that assumption such that the tech could be ready, but the player may still be resolving a source asynchronously via middleware; so, the player should not be ready.

Why? Because, I think it's fair to assume as a user of Video.js that if I queue a ready callback on a player, I should be able to interact with it once that callback fires (e.g., calling play()). This is not currently the case.

However, the src_ function uses techCall_ to set the source, which relies on the tech/player being in a ready state. This leads to a chicken or egg situation that's not easy to solve. Changing techCall_ to use tech_.ready instead causes a raft of weird side effects, such as... video not playing. 😆

I'm going to spend some time today tinkering with this, but it may not be possible to work around without breaking something.

misteroneill · 2017-11-03T17:51:11Z

Alright, turns out most of the issues with changing techCall_ to wait for the tech, specifically, to be ready, were very minor.

gkatsev · 2017-11-06T15:53:37Z

src/js/player.js

+        // At this point, we've found a compatible tech and source and set the
+        // source both on the tech and in the internal cache. It's safe to tell
+        // any subscribers that the player is ready again.
+        this.triggerReady();


do we even need to do this? Should we be getting ready from the tech for the source change in the first place?

Well, the whole idea here is that tech ready != player ready - this is the latter. The tech is ready before the source change is complete. Maybe I'm not following the line of questions.

Well, when the player gets the tech's ready event, it already triggers ready itself:

video.js/src/js/player.js

Lines 907 to 908 in c7ad7b3

// player.triggerReady is always async, so don't need this to be async

this.tech_.ready(Fn.bind(this, this.handleTechReady_), true);

video.js/src/js/player.js

Lines 1055 to 1061 in c7ad7b3

/**

* Player waits for the tech to be ready

*

* @private

*/

handleTechReady_() {

this.triggerReady();

So, we would end up getting two ready events for the same source unless we change the way that handleTechReady functions.

gkatsev · 2017-11-06T15:53:46Z

src/js/player.js

@@ -2324,10 +2330,10 @@ class Player extends Component {
      this.middleware_ = mws;

      // Attempt to set the source on a tech.
-      const foundTech = this.src_(middlewareSource);
+      const didNotFindTech = this.src_(middlewareSource);


that is a better name

misteroneill · 2017-11-06T21:13:24Z

@dmlap brought up an interesting question related to this. If you are using on('ready') will you get more ready events than you expect?

I wondered more about this possibly being a subtle breaking change and whether the alternative approach should be to suggest waiting for canplay before calling play(), leaving the ready as a tech crutch...

player.src({src: 'foo.mp4', type: 'video/mp4'});
player.ready(() => {
  player.one('canplay', () => player.play());
});

gkatsev · 2017-11-07T19:17:43Z

@misteroneill sounds like @dmlap and I had similar questions though worded differently.

misteroneill · 2017-11-07T21:29:46Z

I wonder if a better fix is just to have play() wait for loadstart if it's changing sources?

misteroneill · 2017-11-13T17:14:59Z

I don't think this is the right path forward. I'm going back to the drawing board.

…instead of just ready. (#4743) The core goal here is to make sure the following works in light of some middleware process that makes setting the source more async than next tick: ```js player.src('...'); player.ready(() => player.play()); ``` In fact, given this change, we should even be able to do: ```js player.src('...'); player.play(); ``` Unlike #4665, which would have clarified/changed the meaning of "ready", it remains a reflection of the tech's state and we make better use of the ability to queue things on that state and on the middleware `setSource` process.

…#4665 (#136) This reverts commit 87f0d08.

…#4665 (#136) (#141) This reverts commit 87f0d08.

…136)

…#4665 (#136) (#141) This reverts commit 87f0d08119a825098e944e37a20d95596306eae2.

gkatsev reviewed Oct 16, 2017

View reviewed changes

misteroneill force-pushed the r-u-ready branch from f319644 to c3c35a7 Compare October 16, 2017 20:47

misteroneill commented Oct 17, 2017

View reviewed changes

brandonocasey reviewed Oct 17, 2017

View reviewed changes

gkatsev reviewed Oct 17, 2017

View reviewed changes

gkatsev approved these changes Oct 19, 2017

View reviewed changes

brandonocasey added the confirmed label Oct 20, 2017

misteroneill force-pushed the r-u-ready branch from 51b57d4 to 2203c40 Compare October 23, 2017 15:58

brandonocasey added a commit to videojs/generator-videojs-plugin that referenced this pull request Oct 23, 2017

fix: tick 2 for player ready, See videojs/video.js#4665

b505a8a

brandonocasey mentioned this pull request Oct 23, 2017

fix: tick 2 for player ready videojs/generator-videojs-plugin#136

Merged

misteroneill pushed a commit to videojs/generator-videojs-plugin that referenced this pull request Oct 24, 2017

fix: Tick 2ms in tests for player ready, see videojs/video.js#4665 (#136

87f0d08

)

amclin added a commit to amclin/videojs-scene7 that referenced this pull request Oct 25, 2017

force clock tick to ensure async load of tech in VideoJS 6.2 is compl…

2396254

…ete: see videojs/video.js#4665 and videojs/generator-videojs-plugin@87f0d08

gkatsev removed the confirmed label Oct 25, 2017

misteroneill added 4 commits November 2, 2017 09:16

fix: Fix cases where the player reports that it is ready during sourc…

364a2a8

…e selection, but before tech selection.

Remove unnecessary ready call and fix tests.

fa66837

Minor code cleanup (DRY) and more detailed commentary.

9a99f37

Add test

3997cec

misteroneill force-pushed the r-u-ready branch from 2203c40 to 3997cec Compare November 2, 2017 13:17

gkatsev mentioned this pull request Nov 2, 2017

refactor player.play() #4713

Closed

1 task

Clarify the tech/player readiness separation.

3fddd8a

misteroneill force-pushed the r-u-ready branch from b9f0acf to 3fddd8a Compare November 3, 2017 17:48

gkatsev reviewed Nov 6, 2017

View reviewed changes

misteroneill closed this Nov 13, 2017

misteroneill deleted the r-u-ready branch November 13, 2017 17:15

misteroneill mentioned this pull request Nov 13, 2017

fix: Wait for loadstart when changing sources, instead of just ready. #4743

Merged

2 tasks

misteroneill added a commit to videojs/generator-videojs-plugin that referenced this pull request Nov 27, 2017

revert: fix: Tick 2ms in tests for player ready, see videojs/video.js…

186d7c1

…#4665 (#136) This reverts commit 87f0d08.

misteroneill added a commit to videojs/generator-videojs-plugin that referenced this pull request Nov 27, 2017

revert: fix: Tick 2ms in tests for player ready, see videojs/video.js…

fd23387

…#4665 (#136) (#141) This reverts commit 87f0d08.

brandonocasey added a commit to videojs/tooling that referenced this pull request Jun 3, 2019

fix: Tick 2ms in tests for player ready, see videojs/video.js#4665 (#…

82a2a11

…136)

brandonocasey pushed a commit to videojs/tooling that referenced this pull request Jun 3, 2019

revert: fix: Tick 2ms in tests for player ready, see videojs/video.js…

f4b87d0

…#4665 (#136) (#141) This reverts commit 87f0d08119a825098e944e37a20d95596306eae2.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix cases where the player reports that it is ready during source selection, but before tech selection. #4665

fix: Fix cases where the player reports that it is ready during source selection, but before tech selection. #4665

misteroneill commented Oct 16, 2017 •

edited

Loading

gkatsev Oct 16, 2017

gkatsev Oct 16, 2017

misteroneill Oct 16, 2017

gkatsev Oct 16, 2017

misteroneill Oct 16, 2017

misteroneill Oct 17, 2017

brandonocasey Oct 17, 2017

misteroneill Oct 18, 2017

misteroneill Oct 17, 2017

misteroneill Oct 17, 2017

brandonocasey Oct 17, 2017

brandonocasey Oct 17, 2017

misteroneill Oct 18, 2017

gkatsev Oct 18, 2017

gkatsev left a comment

gkatsev commented Oct 18, 2017

gkatsev left a comment

gkatsev commented Oct 25, 2017

gkatsev commented Oct 31, 2017

alex-barstow commented Nov 1, 2017

misteroneill commented Nov 2, 2017

misteroneill commented Nov 3, 2017

misteroneill commented Nov 3, 2017

gkatsev Nov 6, 2017

misteroneill Nov 6, 2017

gkatsev Nov 6, 2017 •

edited

Loading

gkatsev Nov 6, 2017

misteroneill commented Nov 6, 2017

gkatsev commented Nov 7, 2017

misteroneill commented Nov 7, 2017

misteroneill commented Nov 13, 2017

	// player.triggerReady is always async, so don't need this to be async
	this.tech_.ready(Fn.bind(this, this.handleTechReady_), true);

	/**
	* Player waits for the tech to be ready
	*
	* @private
	*/
	handleTechReady_() {
	this.triggerReady();

fix: Fix cases where the player reports that it is ready during source selection, but before tech selection. #4665

fix: Fix cases where the player reports that it is ready during source selection, but before tech selection. #4665

Conversation

misteroneill commented Oct 16, 2017 • edited Loading

Requirements Checklist

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

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev commented Oct 18, 2017

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev commented Oct 25, 2017

gkatsev commented Oct 31, 2017

alex-barstow commented Nov 1, 2017

misteroneill commented Nov 2, 2017

misteroneill commented Nov 3, 2017

misteroneill commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill commented Nov 6, 2017

gkatsev commented Nov 7, 2017

misteroneill commented Nov 7, 2017

misteroneill commented Nov 13, 2017

misteroneill commented Oct 16, 2017 •

edited

Loading

gkatsev Nov 6, 2017 •

edited

Loading