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.

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

playbackRate support #1132

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@H1D
Contributor

H1D commented Apr 7, 2014

this fixes #840

But unfortunately I broke tests and don't know how to fix them =(
at file:///Users/user/proj/video.js/test/unit/player.js:41: The "playbackRate" method is not available on the playback technology's API

@jnwng

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
var rate = this.player_.tech.playbackRate() + 'x';
return vjs.Component.prototype.createEl.call(this, 'div', {
className: 'vjs-playback-rate vjs-menu-button vjs-control',
innerHTML:'<div class="vjs-playaback-rate-value">' + rate + '</div>'

This comment has been minimized.

@jnwng

jnwng Apr 8, 2014

Member

playaback -> playback

@jnwng

jnwng Apr 8, 2014

Member

playaback -> playback

@heff heff added the enhancement label Apr 11, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 14, 2014

Member

Great stuff, thanks! Digging into it now.

Member

heff commented Apr 14, 2014

Great stuff, thanks! Digging into it now.

@heff

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
player.on('loadstart', vjs.bind(this, function(){
// hide playback rate controls when they're no playback rate options to select
if (( player.tech.features && player.tech.features['playbackRate'] === false) ||
this.player_.options_.playbackRates.length === 0) {

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

Any var with an underscore at the end is a private var, so you should really never have two in the same reference. i.e. this should be this.player().options().playbackRates.length === 0.

@heff

heff Apr 14, 2014

Member

Any var with an underscore at the end is a private var, so you should really never have two in the same reference. i.e. this should be this.player().options().playbackRates.length === 0.

@heff

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
player.on('loadstart', vjs.bind(this, function(){
// hide playback rate controls when they're no playback rate options to select
if (( player.tech.features && player.tech.features['playbackRate'] === false) ||

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

The tech should really be a private thing, so I don't love the idea of accessing it directly from a UI component, but I know this is how the volume control currently does it too and I can't think of a better existing way to do this yet. I created issue #1147 to fix this later.

@heff

heff Apr 14, 2014

Member

The tech should really be a private thing, so I don't love the idea of accessing it directly from a UI component, but I know this is how the volume control currently does it too and I can't think of a better existing way to do this yet. I created issue #1147 to fix this later.

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

I would actually change this line to be

!player.tech.features || !player.tech.features['playbackRate'] || this.player_.options_.playbackRates.length === 0`

That way a tech isn't required to define any of that, and it assumes playback rate is not supported, which is the safer route.

@heff

heff Apr 14, 2014

Member

I would actually change this line to be

!player.tech.features || !player.tech.features['playbackRate'] || this.player_.options_.playbackRates.length === 0`

That way a tech isn't required to define any of that, and it assumes playback rate is not supported, which is the safer route.

@heff

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
// Menu creation
vjs.PlaybackRateMenuButton.prototype.createMenu = function(){
var menu = new vjs.Menu(this.player_);
var rates = this.player_.options_.playbackRates;

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

this should be this.player_.options().playbackRates;

@heff

heff Apr 14, 2014

Member

this should be this.player_.options().playbackRates;

This comment has been minimized.

@H1D

H1D Apr 21, 2014

Contributor

this.player().options().playbackRates; also works

@H1D

H1D Apr 21, 2014

Contributor

this.player().options().playbackRates; also works

@heff

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
vjs.PlaybackRateMenuButton.prototype.onClick = function(){
// select next rate option
var current_rate = this.player_.playbackRate();

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

please use camel case here, currentRate

@heff

heff Apr 14, 2014

Member

please use camel case here, currentRate

@heff

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
vjs.PlaybackRateMenuButton.prototype.onClick = function(){
// select next rate option
var current_rate = this.player_.playbackRate();
var rates = this.player_.options_.playbackRates;

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

this.player_.options(). playbackRates;

@heff

heff Apr 14, 2014

Member

this.player_.options(). playbackRates;

@heff

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
// select next rate option
var current_rate = this.player_.playbackRate();
var rates = this.player_.options_.playbackRates;
// wthis will select first if last currently selected

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

'wthis'

@heff

heff Apr 14, 2014

Member

'wthis'

@heff

View changes

Show outdated Hide outdated src/js/control-bar/playback-rate-menu-button.js
var current_rate = this.player_.playbackRate();
var rates = this.player_.options_.playbackRates;
// wthis will select first if last currently selected
var new_rate = rates[0];

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

camelCase here please

@heff

heff Apr 14, 2014

Member

camelCase here please

@heff

View changes

Show outdated Hide outdated src/js/player.js
@@ -1382,6 +1382,19 @@ vjs.Player.prototype.listenForUserActivity = function(){
clearInterval(activityCheck);
clearTimeout(inactivityTimeout);
});
vjs.Player.prototype.playbackRate = function(rate) {

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

This is actually being defined inside the listenForUserActivity function. It needs to be moved after the next ending curly brace.

@heff

heff Apr 14, 2014

Member

This is actually being defined inside the listenForUserActivity function. It needs to be moved after the next ending curly brace.

@heff

View changes

Show outdated Hide outdated src/js/player.js
return this;
}
return this.techGet('playbackRate') || 1.0;

This comment has been minimized.

@heff

heff Apr 14, 2014

Member

This is what's causing the tests to break. The tests use a mock tech that doesn't support a lot of things, and this call assumes that the features should exist. I think we should change this to protect against the case where where playbackRate is not defined.

  // this will get cleaner after #1147 is implemented
  if (this.tech && this.tech.features && this.tech.features['playbackRate']) {
    return this.techGet('playbackRate');
  }
  return 1.0;
@heff

heff Apr 14, 2014

Member

This is what's causing the tests to break. The tests use a mock tech that doesn't support a lot of things, and this call assumes that the features should exist. I think we should change this to protect against the case where where playbackRate is not defined.

  // this will get cleaner after #1147 is implemented
  if (this.tech && this.tech.features && this.tech.features['playbackRate']) {
    return this.techGet('playbackRate');
  }
  return 1.0;
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 14, 2014

Member

Great job figuring out all the pieces for getting this to work! If you want to make the suggested changes I should be able to pull it in.

@jnwng since you were looking into implementing this, does it do what you were hoping it would?

Member

heff commented Apr 14, 2014

Great job figuring out all the pieces for getting this to work! If you want to make the suggested changes I should be able to pull it in.

@jnwng since you were looking into implementing this, does it do what you were hoping it would?

@heff heff added the confirmed label Apr 14, 2014

@jnwng

This comment has been minimized.

Show comment
Hide comment
@jnwng

jnwng Apr 14, 2014

Member

yeah, i forked @H1D's work here and have the bare playback API working as expected. we've been using this internally already as the changes we added for playbackRate support were merged on our own fork a while ago, just took a while to come around to getting this upstream. we use an alternative front-end component for access the playbackRate, so i haven't had a chance to look at the one presented here.

i do have a few test additions that i've made on the fork, which i'll work with @H1D to get merged into his fork before bringing it all back here to master.

Member

jnwng commented Apr 14, 2014

yeah, i forked @H1D's work here and have the bare playback API working as expected. we've been using this internally already as the changes we added for playbackRate support were merged on our own fork a while ago, just took a while to come around to getting this upstream. we use an alternative front-end component for access the playbackRate, so i haven't had a chance to look at the one presented here.

i do have a few test additions that i've made on the fork, which i'll work with @H1D to get merged into his fork before bringing it all back here to master.

@jnwng

This comment has been minimized.

Show comment
Hide comment
@jnwng

jnwng Apr 14, 2014

Member

@H1D have an open pull with an additional playbackRate test to add to this

Member

jnwng commented Apr 14, 2014

@H1D have an open pull with an additional playbackRate test to add to this

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 15, 2014

Member

Awesome, thanks.

Member

heff commented Apr 15, 2014

Awesome, thanks.

Merge pull request #1 from jnwng/jw/add-playbackrate-test
Adding test for playbackRate API functionality
@H1D

This comment has been minimized.

Show comment
Hide comment
@H1D

H1D Apr 15, 2014

Contributor

cool!
@heff I'll try to follow suggestions you've made to finish this. Give me a day or two.

Contributor

H1D commented Apr 15, 2014

cool!
@heff I'll try to follow suggestions you've made to finish this. Give me a day or two.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 15, 2014

Member

Great, thanks!

Member

heff commented Apr 15, 2014

Great, thanks!

@H1D

This comment has been minimized.

Show comment
Hide comment
@H1D

H1D Apr 21, 2014

Contributor

@heff I did what you suggested. Tests still fails =(

Contributor

H1D commented Apr 21, 2014

@heff I did what you suggested. Tests still fails =(

@mmcc mmcc referenced this pull request May 2, 2014

Closed

change frame rate #1189

@H1D

This comment has been minimized.

Show comment
Hide comment
@H1D

H1D May 7, 2014

Contributor

damn. Tests are passing but UI is broken. Do not merge this

Contributor

H1D commented May 7, 2014

damn. Tests are passing but UI is broken. Do not merge this

};
vjs.PlaybackRateMenuButton.prototype.playbackRateSupported = function(){
return

This comment has been minimized.

@heff

heff May 7, 2014

Member

Got a little too fancy here. :) Should remove the line break after return.

@heff

heff May 7, 2014

Member

Got a little too fancy here. :) Should remove the line break after return.

className: 'vjs-playback-rate vjs-menu-button vjs-control'
});
this.contentEl_ = vjs.createEl('div', {

This comment has been minimized.

@heff

heff May 7, 2014

Member

I thought using the component's build in contentEl would make sense for this, but it looks like MenuButton already makes use of contentEl. You might be able to just change that to 'labelEl' or something like that.

@heff

heff May 7, 2014

Member

I thought using the component's build in contentEl would make sense for this, but it looks like MenuButton already makes use of contentEl. You might be able to just change that to 'labelEl' or something like that.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 7, 2014

Member

Sorry about that... I made some comments that I think should fix it.

Member

heff commented May 7, 2014

Sorry about that... I made some comments that I think should fix it.

@heff heff closed this in 8dfe0a4 May 13, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 13, 2014

Member

This is now fixed and merged into master. I changed it so the playback rates don't show by default because I'm not sure if playback rate is something that everyone wants in their player. This at least lets us see how popular it becomes before making it the default.

To add playback rates, supply the playbackRates option. e.g.

videojs(my_id, { playbackRates: [0.5, 1, 1.5, 2] })
Member

heff commented May 13, 2014

This is now fixed and merged into master. I changed it so the playback rates don't show by default because I'm not sure if playback rate is something that everyone wants in their player. This at least lets us see how popular it becomes before making it the default.

To add playback rates, supply the playbackRates option. e.g.

videojs(my_id, { playbackRates: [0.5, 1, 1.5, 2] })
@isdito

This comment has been minimized.

Show comment
Hide comment
@isdito

isdito May 13, 2014

Hello.
It is in version 4.5.1 ? videojs(my_id, { playbackRates: [0.5, 1, 1.5, 2] })
Thank you

isdito commented May 13, 2014

Hello.
It is in version 4.5.1 ? videojs(my_id, { playbackRates: [0.5, 1, 1.5, 2] })
Thank you

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 14, 2014

Member

No, it will be in 4.6.0, which will be released in the next few days.

Member

heff commented May 14, 2014

No, it will be in 4.6.0, which will be released in the next few days.

@isdito

This comment has been minimized.

Show comment
Hide comment
@isdito

isdito May 14, 2014

Thank you.

isdito commented May 14, 2014

Thank you.

@H1D

This comment has been minimized.

Show comment
Hide comment
@H1D

H1D May 14, 2014

Contributor

That's great! Thx for finishing it.

Contributor

H1D commented May 14, 2014

That's great! Thx for finishing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment