Skip to content

Commit

Permalink
Fix native control seeking on Edge
Browse files Browse the repository at this point in the history
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
  • Loading branch information
joeyparrish committed Aug 10, 2017
1 parent 37ebcff commit 4b6e1fd
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 16 deletions.
7 changes: 6 additions & 1 deletion lib/media/playhead.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
95 changes: 80 additions & 15 deletions test/media/playhead_unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -562,7 +562,7 @@ describe('Playhead', function() {
expectEvent: false,
expectedEndTime: 21
});
});
}); // with small gaps

describe('with large gaps', function() {
playingTest('will fire an event', {
Expand Down Expand Up @@ -611,7 +611,7 @@ describe('Playhead', function() {
expectEvent: true,
expectedEndTime: 10
});
});
}); // with large gaps

/**
* @param {string} name
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -701,7 +701,7 @@ describe('Playhead', function() {
expectedEndTime: 30,
expectEvent: false
});
});
}); // with small gaps

describe('with large gaps', function() {
seekTest('will raise event', {
Expand Down Expand Up @@ -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}],
Expand Down Expand Up @@ -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}],
Expand Down Expand Up @@ -850,8 +850,8 @@ describe('Playhead', function() {
preventDefault: true,
expectEvent: true
});
});
});
}); // with large gaps
}); // with unbuffered seeks

/**
* @param {string} name
Expand Down Expand Up @@ -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
});

0 comments on commit 4b6e1fd

Please sign in to comment.