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.

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 from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions build/source-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ var sourceFiles = [
"src/js/control-bar/volume-control.js",
"src/js/control-bar/mute-toggle.js",
"src/js/control-bar/volume-menu-button.js",
"src/js/control-bar/playback-rate-menu-button.js",
"src/js/poster.js",
"src/js/loading-spinner.js",
"src/js/big-play-button.js",
Expand Down
21 changes: 21 additions & 0 deletions src/css/video-js.less
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,21 @@ fonts to show/hide properly.
content: @pause-icon;
}

/* Playback toggle
--------------------------------------------------------------------------------
*/
.vjs-default-skin .vjs-playback-rate .vjs-playaback-rate-value {
font-size: 1.5em;
line-height: 2;
position: absolute;
top: 0;
left: 0;
width: 100%;
height: 100%;
text-align: center;
text-shadow: 1px 1px 1px rgba(0, 0, 0, 0.5);
}

/* Volume/Mute
-------------------------------------------------------------------------------- */
.vjs-default-skin .vjs-mute-control,
Expand Down Expand Up @@ -636,6 +651,12 @@ easily in the skin designer. http://designer.videojs.com/
.box-shadow(-0.2em -0.2em 0.3em rgba(255, 255, 255, 0.2));
}

.vjs-default-skin .vjs-playback-rate .vjs-menu .vjs-menu-content {
width: 4em;
left: -2em;
list-style: none;
}

.vjs-default-skin .vjs-menu-button:hover .vjs-menu {
display: block;
}
Expand Down
3 changes: 2 additions & 1 deletion src/js/control-bar/control-bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ vjs.ControlBar.prototype.options_ = {
'progressControl': {},
'fullscreenToggle': {},
'volumeControl': {},
'muteToggle': {}
'muteToggle': {},
'playbackRateMenuButton': {}
// 'volumeMenuButton': {}
}
};
Expand Down
102 changes: 102 additions & 0 deletions src/js/control-bar/playback-rate-menu-button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/**
* The component for controlling the playback rate
*
* @param {vjs.Player|Object} player
* @param {Object=} options
* @constructor
*/
vjs.PlaybackRateMenuButton = vjs.MenuButton.extend({
/** @constructor */
init: function(player, options){
vjs.MenuButton.call(this, player, options);

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) ||
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

this.player_.options_.playbackRates.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

this.addClass('vjs-hidden');
} else {
this.removeClass('vjs-hidden');
}
}));

player.on('ratechange', vjs.bind(this, this.rateChange));
}
});


vjs.PlaybackRateMenuButton.prototype.createEl = function(){
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>'
Copy link
Contributor

Choose a reason for hiding this comment

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

playaback -> playback

});
};

// Menu creation
vjs.PlaybackRateMenuButton.prototype.createMenu = function(){
var menu = new vjs.Menu(this.player_);
var rates = this.player_.options_.playbackRates;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

for (var i = rates.length - 1; i >= 0; i--) {
menu.addChild(
new vjs.PlaybackRateMenuItem(this.player_, {rate: rates[i] + 'x'})
);
};

return menu;
};

vjs.PlaybackRateMenuButton.prototype.updateARIAAttributes = function(){
// Current playback rate
this.el_.setAttribute('aria-valuenow',this.player_.playbackRate());
};

vjs.PlaybackRateMenuButton.prototype.onClick = function(){
// select next rate option
var current_rate = this.player_.playbackRate();
Copy link
Member

Choose a reason for hiding this comment

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

please use camel case here, currentRate

var rates = this.player_.options_.playbackRates;
Copy link
Member

Choose a reason for hiding this comment

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

this.player_.options(). playbackRates;

// wthis will select first if last currently selected
Copy link
Member

Choose a reason for hiding this comment

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

'wthis'

var new_rate = rates[0];
Copy link
Member

Choose a reason for hiding this comment

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

camelCase here please

for (var i = 0; i <rates.length ; i++) {
if (rates[i] > current_rate) {
new_rate = rates[i];
break;
}
};
this.player_.playbackRate( new_rate );
};

// Update button label when rate changed
vjs.PlaybackRateMenuButton.prototype.rateChange = function(){
this.el_.children[0].innerHTML = this.player_.playbackRate() + 'x';
};

/**
* The specific menu item type for selecting a playback rate
*
* @constructor
*/
vjs.PlaybackRateMenuItem = vjs.MenuItem.extend({
contentElType: 'button',
/** @constructor */
init: function(player, options){
var label = this.label = options['rate'];
var rate = this.rate = parseFloat(label, 10);

// Modify options for parent MenuItem class's init.
options['label'] = label;
options['selected'] = rate === 1;
vjs.MenuItem.call(this, player, options);

this.player_.on('ratechange', vjs.bind(this, this.update));
}
});

vjs.PlaybackRateMenuItem.prototype.onClick = function(){
vjs.MenuItem.prototype.onClick.call(this);
this.player_.playbackRate(this.rate);
};

vjs.PlaybackRateMenuItem.prototype.update = function(){
this.selected(this.player_.playbackRate() == this.rate);
};
3 changes: 3 additions & 0 deletions src/js/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ vjs.options = {
// defaultVolume: 0.85,
'defaultVolume': 0.00, // The freakin seaguls are driving me crazy!

// default playback rates
'playbackRates': [0.5, 1, 1.5, 2],

// Included control sets
'children': {
'mediaLoader': {},
Expand Down
3 changes: 3 additions & 0 deletions src/js/exports.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ goog.exportSymbol('videojs.PosterImage', vjs.PosterImage);
goog.exportSymbol('videojs.Menu', vjs.Menu);
goog.exportSymbol('videojs.MenuItem', vjs.MenuItem);
goog.exportSymbol('videojs.MenuButton', vjs.MenuButton);
goog.exportSymbol('videojs.PlaybackRateMenuButton', vjs.PlaybackRateMenuButton);
goog.exportProperty(vjs.MenuButton.prototype, 'createItems', vjs.MenuButton.prototype.createItems);
goog.exportProperty(vjs.TextTrackButton.prototype, 'createItems', vjs.TextTrackButton.prototype.createItems);
goog.exportProperty(vjs.ChaptersButton.prototype, 'createItems', vjs.ChaptersButton.prototype.createItems);
Expand Down Expand Up @@ -135,6 +136,8 @@ goog.exportProperty(vjs.Html5.prototype, 'setAutoplay', vjs.Html5.prototype.setA
goog.exportProperty(vjs.Html5.prototype, 'setLoop', vjs.Html5.prototype.setLoop);
goog.exportProperty(vjs.Html5.prototype, 'enterFullScreen', vjs.Html5.prototype.enterFullScreen);
goog.exportProperty(vjs.Html5.prototype, 'exitFullScreen', vjs.Html5.prototype.exitFullScreen);
goog.exportProperty(vjs.Html5.prototype, 'playbackRate', vjs.Html5.prototype.playbackRate);
goog.exportProperty(vjs.Html5.prototype, 'setPlaybackRate', vjs.Html5.prototype.setPlaybackRate);

goog.exportSymbol('videojs.Flash', vjs.Flash);
goog.exportProperty(vjs.Flash, 'isSupported', vjs.Flash.isSupported);
Expand Down
1 change: 0 additions & 1 deletion src/js/media/flash.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,6 @@ vjs.Flash.prototype.enterFullScreen = function(){
return false;
};


// Create setters and getters for attributes
var api = vjs.Flash.prototype,
readWrite = 'rtmpConnection,rtmpStream,preload,defaultPlaybackRate,playbackRate,autoplay,loop,mediaGroup,controller,controls,volume,muted,defaultMuted'.split(','),
Expand Down
3 changes: 3 additions & 0 deletions src/js/media/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,9 @@ vjs.Html5.prototype.seeking = function(){ return this.el_.seeking; };
vjs.Html5.prototype.ended = function(){ return this.el_.ended; };
vjs.Html5.prototype.defaultMuted = function(){ return this.el_.defaultMuted; };

vjs.Html5.prototype.playbackRate = function(){ return this.el_.playbackRate; };
vjs.Html5.prototype.setPlaybackRate = function(val){ this.el_.playbackRate = val; };

/* HTML5 Support Testing ---------------------------------------------------- */

vjs.Html5.isSupported = function(){
Expand Down
14 changes: 13 additions & 1 deletion src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -1382,6 +1382,19 @@ vjs.Player.prototype.listenForUserActivity = function(){
clearInterval(activityCheck);
clearTimeout(inactivityTimeout);
});

vjs.Player.prototype.playbackRate = function(rate) {
Copy link
Member

Choose a reason for hiding this comment

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

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

if (rate !== undefined) {

this.techCall('setPlaybackRate', rate);

this.trigger('ratechange');

return this;
}

return this.techGet('playbackRate') || 1.0;
Copy link
Member

Choose a reason for hiding this comment

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

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;

};
};

// Methods to add support for
Expand All @@ -1396,7 +1409,6 @@ vjs.Player.prototype.listenForUserActivity = function(){
// videoWidth: function(){ return this.techCall('videoWidth'); },
// videoHeight: function(){ return this.techCall('videoHeight'); },
// defaultPlaybackRate: function(){ return this.techCall('defaultPlaybackRate'); },
// playbackRate: function(){ return this.techCall('playbackRate'); },
// mediaGroup: function(){ return this.techCall('mediaGroup'); },
// controller: function(){ return this.techCall('controller'); },
// defaultMuted: function(){ return this.techCall('defaultMuted'); }
Expand Down
1 change: 1 addition & 0 deletions test/unit/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ test('should export useful components to the public', function () {
ok(videojs.Menu, 'Menu should be public');
ok(videojs.MenuItem, 'MenuItem should be public');
ok(videojs.MenuButton, 'MenuButton should be public');
ok(videojs.PlaybackRateMenuButton, 'PlaybackRateMenuButton should be public');

ok(videojs.util, 'util namespace should be public');
ok(videojs.util.mergeOptions, 'mergeOptions should be public');
Expand Down
24 changes: 24 additions & 0 deletions test/unit/controls.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,27 @@ test('calculateDistance should use changedTouches, if available', function() {

equal(slider.calculateDistance(event), 0.5, 'we should have touched exactly in the center, so, the ratio should be half');
});

test('should hide playback rate control if it\'s not supported', function(){
expect(1);

var noop, player, playbackRate, muteToggle;
noop = function(){};
player = {
id: noop,
on: noop,
ready: noop,
tech: {
features: {
'playbackRate': false
}
},
volume: function(){},
muted: function(){},
reportUserActivity: function(){}
};

playbackRate = new vjs.PlaybackRateMenuButton(player);

ok(playbackRate.el().className.indexOf('vjs-hidden') >= 0, 'playbackRate is not hidden');
});