From 97ec1cec30aa70bc35ed48e12a29d458c3da0f68 Mon Sep 17 00:00:00 2001 From: Nishant Karandikar Date: Fri, 3 Feb 2017 14:26:31 -0800 Subject: [PATCH 1/2] Remove unnecessary javascript Previously, the Youtube API had a bug where a speed change from another speed back to 1.0 would not register properly in Firefox. The API has since been fixed, so there is no need for a workaround --- .../xmodule/js/src/video/03_video_player.js | 66 +------------------ 1 file changed, 2 insertions(+), 64 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js index 8e85afe80c..c1bc5a2819 100644 --- a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js +++ b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js @@ -383,71 +383,9 @@ function(HTML5Video, Resizer) { function setPlaybackRate(newSpeed, useCueVideoById) { var duration = this.videoPlayer.duration(), - time = this.videoPlayer.currentTime, - methodName, youtubeId; + time = this.videoPlayer.currentTime; - // There is a bug which prevents YouTube API to correctly set the speed - // to 1.0 from another speed in Firefox when in HTML5 mode. There is a - // fix which basically reloads the video at speed 1.0 when this change - // is requested (instead of simply requesting a speed change to 1.0). - // This has to be done only when the video is being watched in Firefox. - // We need to figure out what browser is currently executing this code. - // - // TODO: Check the status of - // http://code.google.com/p/gdata-issues/issues/detail?id=4654 - // When the YouTube team fixes the API bug, we can remove this - // temporary bug fix. - - // If useCueVideoById is true it will reload video again. - // Used useCueVideoById to fix the issue video not playing if we change - // the speed before playing the video. - if ( - this.isHtml5Mode() && !(this.browserIsFirefox && - (useCueVideoById || newSpeed === '1.0') && this.isYoutubeType()) - ) { - this.videoPlayer.player.setPlaybackRate(newSpeed); - } else { - // We request the reloading of the video in the case when YouTube - // is in Flash player mode, or when we are in Firefox, and the new - // speed is 1.0. The second case is necessary to avoid the bug - // where in Firefox speed switching to 1.0 in HTML5 player mode is - // handled incorrectly by YouTube API. - methodName = 'cueVideoById'; - youtubeId = this.youtubeId(newSpeed); - - if (this.videoPlayer.isPlaying()) { - methodName = 'loadVideoById'; - } - - this.videoPlayer.player[methodName](youtubeId, time); - - // We need to call play() explicitly because after the call - // to functions cueVideoById() followed by seekTo() the video - // is in a PAUSED state. - // - // Why? This is how the YouTube API is implemented. - // sjson.search() only works if time is defined. - if (!_.isUndefined(time)) { - this.videoPlayer.updatePlayTime(time); - } - if (time > 0 && this.isFlashMode()) { - this.videoPlayer.seekTo(time); - this.trigger( - 'videoProgressSlider.updateStartEndTimeRegion', - { - duration: duration - } - ); - } - // In Html5 mode if video speed is changed before playing in firefox and - // changed speed is not '1.0' then manually trigger setPlaybackRate method. - // In browsers other than firefox like safari user can set speed to '1.0' - // if its not already set to '1.0' so in that case we don't have to - // call 'setPlaybackRate' - if (this.isHtml5Mode() && newSpeed != '1.0') { - this.videoPlayer.player.setPlaybackRate(newSpeed); - } - } + this.videoPlayer.player.setPlaybackRate(newSpeed); } function onSpeedChange(newSpeed) { From a480a502c17b40398759b8dad8522161799385ff Mon Sep 17 00:00:00 2001 From: Nishant Karandikar Date: Fri, 3 Feb 2017 14:50:48 -0800 Subject: [PATCH 2/2] Remove unnecessary param in function declaration & calls There was previously a bug in the Youtube API that caused videos to not play at a set speed in Safari. The bug has apparently since been fixed, so we no longer need code for that browser special case. --- .../js/spec/video/video_player_spec.js | 21 +++++-------------- .../xmodule/js/src/video/03_video_player.js | 16 ++------------ 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js index 7588db9ba4..b2faa75a89 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js @@ -436,7 +436,7 @@ function(VideoPlayer) { state.speed = '2.0'; state.videoPlayer.onPlay(); expect(state.videoPlayer.setPlaybackRate) - .toHaveBeenCalledWith('2.0', true); + .toHaveBeenCalledWith('2.0'); state.videoPlayer.onPlay(); expect(state.videoPlayer.setPlaybackRate.calls.count()) .toEqual(1); @@ -943,9 +943,8 @@ function(VideoPlayer) { state.isHtml5Mode.and.returnValue(false); state.videoPlayer.isPlaying.and.returnValue(true); VideoPlayer.prototype.setPlaybackRate.call(state, '0.75'); - expect(state.videoPlayer.updatePlayTime).toHaveBeenCalledWith(60); - expect(state.videoPlayer.player.loadVideoById) - .toHaveBeenCalledWith('videoId', 60); + expect(state.videoPlayer.player.setPlaybackRate) + .toHaveBeenCalledWith('0.75'); }); it('in Flash mode and video not started', function() { @@ -953,15 +952,7 @@ function(VideoPlayer) { state.isHtml5Mode.and.returnValue(false); state.videoPlayer.isPlaying.and.returnValue(false); VideoPlayer.prototype.setPlaybackRate.call(state, '0.75'); - expect(state.videoPlayer.updatePlayTime).toHaveBeenCalledWith(60); - expect(state.videoPlayer.seekTo).toHaveBeenCalledWith(60); - expect(state.trigger).toHaveBeenCalledWith( - 'videoProgressSlider.updateStartEndTimeRegion', - { - duration: 60 - }); - expect(state.videoPlayer.player.cueVideoById) - .toHaveBeenCalledWith('videoId', 60); + expect(state.videoPlayer.player.setPlaybackRate).toHaveBeenCalledWith('0.75'); }); it('in HTML5 mode', function() { @@ -975,9 +966,7 @@ function(VideoPlayer) { state.videoPlayer.isPlaying.and.returnValue(false); VideoPlayer.prototype.setPlaybackRate.call(state, '1.0'); - expect(state.videoPlayer.updatePlayTime).toHaveBeenCalledWith(60); - expect(state.videoPlayer.player.cueVideoById) - .toHaveBeenCalledWith('videoId', 60); + expect(state.videoPlayer.player.setPlaybackRate).toHaveBeenCalledWith('1.0'); }); }); }); diff --git a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js index c1bc5a2819..6eecd3df86 100644 --- a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js +++ b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js @@ -109,16 +109,7 @@ function(HTML5Video, Resizer) { // starts playing. Just after that configurations can be applied. state.videoPlayer.ready = _.once(function() { if (!state.isFlashMode() && state.speed != '1.0') { - // Work around a bug in the Youtube API that causes videos to - // play at normal speed rather than at the configured speed in - // Safari. Setting the playback rate to 1.0 *after* playing - // started and then to the actual value tricks the player into - // picking up the speed setting. - if (state.browserIsSafari && state.isYoutubeType()) { - state.videoPlayer.setPlaybackRate(1.0, false); - } - - state.videoPlayer.setPlaybackRate(state.speed, true); + state.videoPlayer.setPlaybackRate(state.speed); } }); @@ -381,10 +372,7 @@ function(HTML5Video, Resizer) { } } - function setPlaybackRate(newSpeed, useCueVideoById) { - var duration = this.videoPlayer.duration(), - time = this.videoPlayer.currentTime; - + function setPlaybackRate(newSpeed) { this.videoPlayer.player.setPlaybackRate(newSpeed); }