fix: fix replay functionality #204
Conversation
src/middleware-set-current-time.js
Outdated
| play() { | ||
| if (player.vhs && | ||
| player.currentSource().src === player.vhs.source_.src) { | ||
| player.vhs.setCurrentTime(player.currentTime()); |
There was a problem hiding this comment.
I know this is meant to be a short-term fix, but we should add some comments about what we're doing and why it works for the replay case.
test/playback.test.js
Outdated
|
|
||
| player.one('ended', function() { | ||
| player.one('timeupdate', function() { | ||
| assert.ok(true, 'played'); |
There was a problem hiding this comment.
It might be worth checking the currentTime here.
|
Instead of calling VHS's play before the tech's play, I am syncing VHS with the player after play. This is because I had an issue with live streams where the VHS play assumes that the player is in a playing state. |
src/middleware-set-current-time.js
Outdated
|
|
||
| // Sync VHS after play requests. | ||
| // This specifically handles replay where the video element will | ||
| // seek to 0, skipping the setCurrentTime middleware, then play |
There was a problem hiding this comment.
technically it plays first, but triggers the play event last
src/middleware-set-current-time.js
Outdated
| play() { | ||
| if (player.vhs && | ||
| player.currentSource().src === player.vhs.source_.src) { | ||
| player.vhs.setCurrentTime(player.currentTime()); |
There was a problem hiding this comment.
I think I would feel better if we specifically handled the case where player.currentTime() === 0 instead of always doing this
test/playback.test.js
Outdated
| assert.expect(2); | ||
| let player = this.player; | ||
|
|
||
| player.autoplay(true); |
There was a problem hiding this comment.
do you have to autoplay? Seems like this would be annoying to manage with autoplay restrictions
There was a problem hiding this comment.
This is to simplify the tests and not have to wait for loadstart before playing. Autoplay restrictions aren't a problem because the player is muted
There was a problem hiding this comment.
I could remove the autoplay setting from all the tests and just create the player with autoplay on
|
Tested in Chrome, Firefox, and IE11 (on a Windows VM). |
|
The middleware file probably should be renamed now 🙂 |
Description
Fixes #201
Specific Changes proposed
Replay is broken because VHS is not loading the buffer when the video element seeks on replay. This previously worked because VHS would react to video element seek events. The seek caused by replay does not call
player.currentTime()so the middleware does not kick in. This adds an additional middleware to call VHS's during play calls.Previously the play method of VHS would not go down the ended -> seek to 0 path because the tech would have already seeked to 0 on it's own, then emitted a play event.
Requirements Checklist