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.

Sign up for GitHub

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

Queue playback events when the playback rate is zero #5024

Merged
merged 7 commits into from
Apr 17, 2018
Merged

Queue playback events when the playback rate is zero #5024

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb4ea18
Queue playback events when the playback rate is zero
squarebracket Mar 15, 2018
2f2c2b1
fixes
squarebracket Mar 20, 2018
c94e136
adding tests
squarebracket Mar 26, 2018
e439723
include test ensuring that events are dispatched when not seeking
squarebracket Mar 29, 2018
0d234d6
dispatch the actual event object
squarebracket Mar 29, 2018
6d5eef4
tiny doc update
squarebracket Mar 30, 2018
5dab218
cache lastPlaybackRate
squarebracket Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,13 @@ const TECH_EVENTS_RETRIGGER = [
'texttrackchange'
];

const TECH_EVENTS_QUEUE = {
canplay: 'CanPlay',
canplaythrough: 'CanPlayThrough',
playing: 'Playing',
seeked: 'Seeked'
};

/**
* An instance of the `Player` class is created when any of the Video.js setup methods
* are used to initialize a video.
Expand Down Expand Up @@ -320,6 +327,9 @@ class Player extends Component {
// Tracks when a tech changes the poster
this.isPosterFromTech_ = false;

// TODO: Docs
this.queuedCallbacks_ = [];

// Turn off API access because we're loading a new tech that might load asynchronously
this.isReady_ = false;

Expand Down Expand Up @@ -967,15 +977,22 @@ class Player extends Component {
TECH_EVENTS_RETRIGGER.forEach((event) => {
this.on(this.tech_, event, this[`handleTech${toTitleCase(event)}_`]);
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gkatsev gkatsev Mar 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. the tech and SH aren't supposed to have a reference to the player, so, they can't trigger an event on it.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.queuedCallbacks_.push(this[`handleTech${TECH_EVENTS_QUEUE[event]}_`].bind(this));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the event object should be passed through as well.

return;
}
this[`handleTech${TECH_EVENTS_QUEUE[event]}_`]();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event object should be passed through as well.

});
});

this.on(this.tech_, 'loadstart', this.handleTechLoadStart_);
this.on(this.tech_, 'sourceset', this.handleTechSourceset_);
this.on(this.tech_, 'waiting', this.handleTechWaiting_);
this.on(this.tech_, 'canplay', this.handleTechCanPlay_);
this.on(this.tech_, 'canplaythrough', this.handleTechCanPlayThrough_);
this.on(this.tech_, 'playing', this.handleTechPlaying_);
this.on(this.tech_, 'ended', this.handleTechEnded_);
this.on(this.tech_, 'seeking', this.handleTechSeeking_);
this.on(this.tech_, 'seeked', this.handleTechSeeked_);
this.on(this.tech_, 'play', this.handleTechPlay_);
this.on(this.tech_, 'firstplay', this.handleTechFirstPlay_);
this.on(this.tech_, 'pause', this.handleTechPause_);
Expand All @@ -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_);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the documentation.


this.usingNativeControls(this.techGet_('controls'));

Expand Down Expand Up @@ -1276,6 +1294,17 @@ class Player extends Component {
this.trigger('play');
}

/**
* TODO: docs
*/
handleTechRateChange_() {
if (this.tech_.playbackRate() > 0 && this.previousPlaybackRate === 0) {
this.queuedCallbacks_.forEach((callback) => callback());
this.queuedCallbacks_ = [];
}
this.previousPlaybackRate = this.tech_.playbackRate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to this.cache_.lastPlaybackRate

}

/**
* Retrigger the `waiting` event that was triggered by the {@link Tech}.
*
Expand Down