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

feat: Better UI for live playback via an option #5511

Merged
merged 58 commits into from Dec 3, 2018

Conversation

Projects
None yet
7 participants
@BrandonOCasey
Copy link
Contributor

BrandonOCasey commented Oct 17, 2018

Changes

All of the following is behind a new option called liveui that defaults to false:

LiveDisplay

  • Shown when vjs-live is set on the player but hidden when vjs-live and vjs-liveui are both on the player

SeekToLive

  • A new component that will show when vjs-live and vjs-liveui classes are on the player
  • Used to show when the player is at the live edge, and to indicate that the player is live
  • Can be clicked to seek to the live edge when behind.

LiveTracker

  • Handles tracking of the "live edge" and fires events when the live edge and currentTime are not in sync.
  • Handles tracking of seekable end changes so that we can keep up to date with what the live current time should be

Seek/Progress Bar

  • Show during live playback
  • Make all time tooltips show negative numbers indicating the time from live.
  • Seek within the live window
  • Keep updating all the time when live streaming, since if you pause you will fall behind the live stream.
  • Hide and show various components via live css

window.currentLiveTimeRange = videojs.createTimeRanges(0, 0);
window.currentLiveTimeRange = videojs.createTimeRanges(0, segmentLength);

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Oct 17, 2018

Author Contributor

This fixes an issue where it would not show up as live for the first few seconds.


player.ready(function() {
player.currentTime(segmentLength);

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Oct 17, 2018

Author Contributor

This makes it so that there are a few seconds before the "live" point as in a normal live stream

@@ -13,10 +13,6 @@
cursor: default;
}

.vjs-live .vjs-progress-control {

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Oct 17, 2018

Author Contributor

we show this now

@@ -173,3 +169,7 @@
color: #fff;
@include background-color-with-alpha(#000, 0.8);
}

.vjs-live .vjs-load-progress {
display: none;

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Oct 17, 2018

Author Contributor

I am not sure if we want to hide this or fix it, basically this can make it so that the progress bar goes past the player.

@@ -24,7 +24,7 @@ class LiveDisplay extends Button {
constructor(player, options) {
super(player, options);

this.updateShowing();
player.ready(() => this.updateShowing());

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Oct 17, 2018

Author Contributor

We may not know if the player is live until it is ready.

@BrandonOCasey BrandonOCasey force-pushed the feat/live-progress-control branch from ec7998c to 64950bc Oct 17, 2018

@mister-ben

This comment has been minimized.

Copy link
Contributor

mister-ben commented Oct 18, 2018

The button width might have to be flexible to account for longer localised strings. Catalan 'EN DIRECTE' is the longest we currently have in the translations.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Oct 18, 2018

Is there a reason this isn't implemented as a middleware?
I think this should be enabled by default but with the ability to turn it off but there's still time to discuss.

@misteroneill

This comment has been minimized.

Copy link
Member

misteroneill commented Oct 19, 2018

@gkatsev We discussed that a bit in our grooming call, but @BrandonOCasey had some uncertainty. We can discuss further on Monday when you're back. 👍

@BrandonOCasey BrandonOCasey force-pushed the feat/live-progress-control branch 2 times, most recently from c8b5bc0 to e4e7241 Oct 19, 2018

@BrandonOCasey

This comment has been minimized.

Copy link
Contributor Author

BrandonOCasey commented Oct 22, 2018

Internally we originally though that this could be implemented as middleware, but I found several issues with that in the initial implementation. Here are the issues

  1. We would have to change currentTime internally to 0 at the live edge and negative seconds before that. This caused a bunch of issues across the board, since the UI did not know how to deal with currentTime not changing, or going negative. It would also be inconsistent with the video element API.
  2. In order to keep the progress bar within the seekable range of the video element we would have to change the duration from Infinity. Which would hide that it was a livestream. furthermore we would have to update the duration every time seekable changed which for short segment live streams could be every 2s.
  3. Even if we were to do the two things above, there would still be UI specific code for live streams, and it would probably be very similar to what we have now. For instance we would have to add support for negative time tool tips and the play progress would still have to update during pause when live streaming (so that the seek bar can decrease)

After talking about it we think that the issues do warrant this being a UI only change. As having live streams now have a finite duration and negative current times would be too far from what a video element does.

@BrandonOCasey BrandonOCasey force-pushed the feat/live-updates branch from 4da9701 to 6a7cc35 Oct 23, 2018

@BrandonOCasey BrandonOCasey force-pushed the feat/live-progress-control branch from ad6be26 to 3d9ee46 Oct 23, 2018

@BrandonOCasey BrandonOCasey force-pushed the feat/live-updates branch from 6a7cc35 to 3678963 Oct 25, 2018

@BrandonOCasey BrandonOCasey force-pushed the feat/live-progress-control branch from 3d9ee46 to a626394 Oct 25, 2018

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Nov 2, 2018

I haven't done in depth testing but a there's documentation missing.
Also, testing with a stream with a window of 5 segments of 10 seconds each, I'd have expected the progress bar to be let us seek back the full 20 seconds, at least after playing back a segment. Is this something we should talk with the playback team about?

@BrandonOCasey

This comment has been minimized.

Copy link
Contributor Author

BrandonOCasey commented Nov 2, 2018

Yeah I think we would need to talk to them about that. We can only seek back as far as seekable start, as far as I know.

@gkatsev

This comment has been minimized.

Copy link
Member

gkatsev commented Nov 2, 2018

Looks like the stream I was testing with had a target duration much larger than the segment size which cut into the seekable.

@BrandonOCasey BrandonOCasey force-pushed the feat/live-progress-control branch 2 times, most recently from 54fc3d3 to 1106fc9 Nov 6, 2018

@misteroneill
Copy link
Member

misteroneill left a comment

One thought. @gkatsev thoughts on this?

Show resolved Hide resolved src/js/live-tracker.js Outdated

@BrandonOCasey BrandonOCasey force-pushed the feat/live-progress-control branch from be2fceb to d75e491 Dec 3, 2018

@gkatsev gkatsev merged commit 2974ad3 into master Dec 3, 2018

4 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@gkatsev gkatsev deleted the feat/live-progress-control branch Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment