Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Basic UI for Live #1121

Closed
wants to merge 8 commits into from
Closed

Basic UI for Live #1121

wants to merge 8 commits into from

Conversation

tomjohnson916
Copy link
Contributor

  • Added LiveDisplay UI component
  • Included LiveDisplay component with default ControlBar.children
  • Added live stream detection on duration change
  • Added vjs-default-skin.vjs-live CSS classes to manage UI display

dmlap
dmlap reviewed Mar 31, 2014
View reviewed changes
src/js/player.js
if (duration <= 0 || duration === window.INFINITY) {
this.addClass('vjs-live');
} else {
this.removeClass('vjs-live');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably cause styles to get recalculated on the player every time there is a duration change. That might be expensive. Have you checked this out on mobile devices yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not yet, setting up HLS live testing right now. I could just as easily set this to check on loadstart if we feel this is too heavy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

durationchange should only happen once whenever the source changes (metadata is updated). Is that still a concern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in most segmented delivery formats (HLS/HDS/DASH) duration change happens regularly in live as the manifest updates on polling, so @dmlap 's concern is legitimate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok got it. Is the duration even used in that case then? Should we not even be triggering durationchange in a live scenario? I guess whatever iOS does we should be ready for either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duration is used in any live with DVR scenarios.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the live with DVR scenario, what does the duration look like? Would it be caught by the current conditional (duration < 0 || duration === window.INFINITY)?

At what point do we know for sure if the video is live or not? Is it after loadedmetadata? Could it change after loadedmetadata?

Can we outline the different live scenarios, how we know they're live, and at what point we have that information available?

@heff
Copy link
Member

heff commented Apr 1, 2014

Tests are failing because you need to export the component like these:
https://github.com/videojs/video.js/blob/master/src/js/exports.js#L77

@tomjohnson916
Copy link
Contributor Author

Updated, fixed tests. Thanks. I could not figure out what was making that fail.

@gkatsev
Copy link
Member

gkatsev commented Apr 1, 2014

Looks like there might still be problems with the minification of this. Seeing errors when trying to create a player with the minified code.

@gkatsev
Copy link
Member

gkatsev commented Apr 1, 2014

Looks like on iOS, we get the class removed right now. The class gets added but then ends up being removed, even though the duration of the video is set to Infinity.

@tomjohnson916
update to fix infinity capitalization. only do css logic on valid dur…
2dd8284 
…ation. set any duration of less than zero to window.Infinity
@gkatsev
Copy link
Member

gkatsev commented Apr 1, 2014

That last update fixed the iOS problem.

@heff
Copy link
Member

heff commented Apr 1, 2014

(this comment was on an outdated diff, so reposting it)

In the live with DVR scenario, what does the duration look like? Would it be caught by the current conditional (duration < 0 || duration === window.INFINITY)?

At what point do we know for sure if the video is live or not? Is it after loadedmetadata? Could it change after loadedmetadata? Can we outline the different live scenarios, how we know they're live, and at what point we have that information available?

@tomjohnson916
Copy link
Contributor Author

@heff - comments below;

In the live with DVR scenario, what does the duration look like?

  • It looks like VOD. There's a currentTime and a duration. The main difference is that both values update.

At what point do we know for sure if the video is live or not? Is it after loadedmetadata?

  • It varies by streaming type but would be after metadata. RTMP comes across in metadata with a duration of -1. HLS would be determined by the manifest(s) coming back without an ENDLIST tag.

Could it change after loadedmetadata?

  • RTMP no, HLS yes once the final manifest comes down with an ENDLIST tag.

Can we outline the different live scenarios, how we know they're live, and at what point we have that information available?

  • We certainly should have a quick meeting on this. The answers vary by streaming type.

@heff
Copy link
Member

heff commented Apr 1, 2014

Just loaded the UI and I'm seeing a few issues that need to be addressed.

  • The duration should probably be hidden (as discussed in chat)
  • LIVE isn't positioned inline with everything else. It's sitting awkwardly at the top of the bar.
  • The progress bar should probably be removed/hidden. It's confusing having it there and clickable as if it will do anything. It looks like maybe we're trying to but the CSS isn't working?

For now we're a assuming the live-only case, as opposed to the live-DVR case, which will behave differently.

screen shot 2014-04-01 at 1 35 03 pm

@heff
Copy link
Member

heff commented Apr 1, 2014

Sorry, none of that last comment is needed. I hadn't compiled the new CSS file in my example. The player is looking good.

screen shot 2014-04-01 at 2 18 39 pm

@heff
Copy link
Member

heff commented Apr 2, 2014

Tried manually merging and squashing commits, and it kept this from automatically closing, but it's pulled into master now.
6a097c0

@heff heff closed this Apr 2, 2014
@deedos
Copy link
Contributor

deedos commented Sep 24, 2014

the live dvr feature has been moved to another issue ? thanks

@heff
Copy link
Member

heff commented Sep 25, 2014

@seniorflexdeveloper did we make any progress on Live DVR with the DASH work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
confirmed enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomjohnson916 @heff @gkatsev @deedos @dmlap