From 4b6e1fda6587104b5603a33e0e1d2dcf18a2f10e Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Thu, 10 Aug 2017 10:03:51 -0700 Subject: [PATCH] Fix native control seeking on Edge When the user seeks using Edge's native controls, Edge sets playbackRate to 0 first. This confused Playhead and resulted in a playbackRate stuck at 0 after the seek. To fix it, we filter out rate changes to 0 in the code that remembers the previous setting during a buffering event. Closes #951 Change-Id: Ia7a9a2a6d65dcf2d74ea6fb4d92594070a1ebe6a --- lib/media/playhead.js | 7 ++- test/media/playhead_unit.js | 95 +++++++++++++++++++++++++++++++------ 2 files changed, 86 insertions(+), 16 deletions(-) diff --git a/lib/media/playhead.js b/lib/media/playhead.js index 939bb03ad4..d3fc97723d 100644 --- a/lib/media/playhead.js +++ b/lib/media/playhead.js @@ -286,7 +286,12 @@ shaka.media.Playhead.prototype.onRateChange_ = function() { // the playback rate is negative. Pause will still work. var expectedRate = this.buffering_ || this.playbackRate_ < 0 ? 0 : this.playbackRate_; - if (this.video_.playbackRate != expectedRate) { + + // Native controls in Edge trigger a change to playbackRate and set it to 0 + // when seeking. If we don't exclude 0 from this check, we will force the + // rate to stay at 0 after a seek with Edge native controls. + // https://github.com/google/shaka-player/issues/951 + if (this.video_.playbackRate && this.video_.playbackRate != expectedRate) { shaka.log.debug('Video playback rate changed to', this.video_.playbackRate); this.setPlaybackRate(this.video_.playbackRate); } diff --git a/test/media/playhead_unit.js b/test/media/playhead_unit.js index 6be6f8d0bf..45d53fbb3b 100644 --- a/test/media/playhead_unit.js +++ b/test/media/playhead_unit.js @@ -216,7 +216,7 @@ describe('Playhead', function() { video.currentTime = 6; expect(playhead.getTime()).toBe(6); }); - }); + }); // getTime it('clamps playhead after seeking for live', function() { video.readyState = HTMLMediaElement.HAVE_METADATA; @@ -499,7 +499,7 @@ describe('Playhead', function() { expect(playhead.getTime()).toBe(10); expect(onSeek).toHaveBeenCalled(); }); - }); + }); // clamps playhead after resuming describe('gap jumping', function() { beforeAll(function() { @@ -562,7 +562,7 @@ describe('Playhead', function() { expectEvent: false, expectedEndTime: 21 }); - }); + }); // with small gaps describe('with large gaps', function() { playingTest('will fire an event', { @@ -611,7 +611,7 @@ describe('Playhead', function() { expectEvent: true, expectedEndTime: 10 }); - }); + }); // with large gaps /** * @param {string} name @@ -656,7 +656,7 @@ describe('Playhead', function() { expect(video.currentTime).toBe(data.expectedEndTime); }); } - }); + }); // when playing describe('with buffered seeks', function() { describe('with small gaps', function() { @@ -701,7 +701,7 @@ describe('Playhead', function() { expectedEndTime: 30, expectEvent: false }); - }); + }); // with small gaps describe('with large gaps', function() { seekTest('will raise event', { @@ -730,11 +730,11 @@ describe('Playhead', function() { expectedEndTime: 12, expectEvent: true }); - }); - }); + }); // with large gaps + }); // with buffered seeks - describe('unbuffered seek', function() { - describe('w/ small gaps', function() { + describe('with unbuffered seeks', function() { + describe('with small gaps', function() { seekTest('won\'t jump when seeking into buffered range', { // [0-10], [20-30], [31-40] buffered: [{start: 0, end: 10}], @@ -805,9 +805,9 @@ describe('Playhead', function() { expectedEndTime: 2, expectEvent: false }); - }); + }); // with small gaps - describe('w/ large gaps', function() { + describe('with large gaps', function() { seekTest('will jump large gap at beginning', { buffered: [{start: 20, end: 30}], newBuffered: [{start: 20, end: 30}], @@ -850,8 +850,8 @@ describe('Playhead', function() { preventDefault: true, expectEvent: true }); - }); - }); + }); // with large gaps + }); // with unbuffered seeks /** * @param {string} name @@ -928,5 +928,70 @@ describe('Playhead', function() { // The video doesn't have any video data. return HTMLMediaElement.HAVE_METADATA; } - }); + }); // gap jumping + + describe('rate changes', function() { + beforeEach(function() { + playhead = new shaka.media.Playhead( + video, + manifest, + config, + 0 /* startTime */, + Util.spyFunc(onSeek), + Util.spyFunc(onEvent)); + }); + + it('notices video rate changes', function() { + expect(playhead.getPlaybackRate()).toBe(1); + + video.playbackRate = 2; + video.on['ratechange'](); + expect(playhead.getPlaybackRate()).toBe(2); + }); + + it('controls video rate with setPlaybackRate', function() { + expect(playhead.getPlaybackRate()).toBe(1); + playhead.setPlaybackRate(2); + expect(playhead.getPlaybackRate()).toBe(2); + expect(video.playbackRate).toBe(2); + }); + + it('sets video rate to 0 when buffering', function() { + expect(video.playbackRate).toBe(1); + playhead.setBuffering(true); + expect(video.playbackRate).toBe(0); + }); + + it('remembers previous rate while buffering', function() { + playhead.setPlaybackRate(5); + expect(video.playbackRate).toBe(5); + expect(playhead.getPlaybackRate()).toBe(5); + playhead.setBuffering(true); + expect(video.playbackRate).toBe(0); + expect(playhead.getPlaybackRate()).toBe(5); + playhead.setBuffering(false); + expect(video.playbackRate).toBe(5); + expect(playhead.getPlaybackRate()).toBe(5); + }); + + it('ignores a rate change to 0', function() { + // Regression test for https://github.com/google/shaka-player/issues/951 + expect(video.playbackRate).toBe(1); + expect(playhead.getPlaybackRate()).toBe(1); + + // With native controls on Edge, a rate change to 0 occurs when the user + // seeks. This seems to happen before setBuffering(true). + video.playbackRate = 0; + video.on['ratechange'](); + expect(playhead.getPlaybackRate()).toBe(1); + + playhead.setBuffering(true); + expect(video.playbackRate).toBe(0); + expect(playhead.getPlaybackRate()).toBe(1); + + playhead.setBuffering(false); + expect(video.playbackRate).toBe(1); + expect(playhead.getPlaybackRate()).toBe(1); + }); + }); // rate changes });