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

Basic UI for Live #1121

Closed
wants to merge 8 commits into from
Closed

Basic UI for Live #1121

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bc47c5e
Basic UI for Live
tomjohnson916 Mar 31, 2014
2fb10cb
fix tests, remove update display list in LiveDisplay
tomjohnson916 Mar 31, 2014
554c003
update exports, fix tests
tomjohnson916 Apr 1, 2014
f940bef
update to less than zero on player durationChange
tomjohnson916 Apr 1, 2014
2dd8284
update to fix infinity capitalization. only do css logic on valid dur…
tomjohnson916 Apr 1, 2014
6a093d6
remove window scope of infinity
tomjohnson916 Apr 1, 2014
10f21cc
whitespace fix
tomjohnson916 Apr 1, 2014
2c5d4d5
update control text for liveDisplay component
tomjohnson916 Apr 1, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build/source-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ var sourceFiles = [
"src/js/menu.js",
"src/js/player.js",
"src/js/control-bar/control-bar.js",
"src/js/control-bar/live-display.js",
"src/js/control-bar/play-toggle.js",
"src/js/control-bar/time-display.js",
"src/js/control-bar/fullscreen-toggle.js",
Expand Down
21 changes: 21 additions & 0 deletions src/css/video-js.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,27 @@ fonts to show/hide properly.
padding-top: 0.1em /* Minor adjustment */;
}

/* Live Mode
--------------------------------------------------------------------------------
*/
.vjs-default-skin.vjs-live .vjs-time-controls,
.vjs-default-skin.vjs-live .vjs-time-divider,
.vjs-default-skin.vjs-live .vjs-progress-control {
display: none;
}
.vjs-default-skin.vjs-live .vjs-live-display {
display: block;
}

/* Live Display
--------------------------------------------------------------------------------
*/
.vjs-default-skin .vjs-live-display {
display: none;
font-size: 1em;
line-height: 3em;
}

/* Time Display
--------------------------------------------------------------------------------
*/
Expand Down
1 change: 1 addition & 0 deletions src/js/control-bar/control-bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ vjs.ControlBar.prototype.options_ = {
'timeDivider': {},
'durationDisplay': {},
'remainingTimeDisplay': {},
'liveDisplay': {},
'progressControl': {},
'fullscreenToggle': {},
'volumeControl': {},
Expand Down
28 changes: 28 additions & 0 deletions src/js/control-bar/live-display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Displays the live indicator
* TODO - Future make it click to snap to live
* @param {vjs.Player|Object} player
* @param {Object=} options
* @constructor
*/
vjs.LiveDisplay = vjs.Component.extend({
init: function(player, options){
vjs.Component.call(this, player, options);
}
});

vjs.LiveDisplay.prototype.createEl = function(){
var el = vjs.Component.prototype.createEl.call(this, 'div', {
className: 'vjs-live-controls vjs-control'
});

this.contentEl_ = vjs.createEl('div', {
className: 'vjs-live-display',
innerHTML: '<span class="vjs-control-text">LIVE</span>LIVE',
Copy link
Member

Choose a reason for hiding this comment

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

Is the second LIVE outside of the vjs-control-text needed? Would be better to take it out.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind. Didn't realize other controls were also doing it and that it was mostly for screen readers.

Copy link
Member

Choose a reason for hiding this comment

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

Actually yeah, in this case the screen reader would read both, "LIVELIVE". If you look at how the durationDisplay works, the control text is used to describe what the value is.

'<span class="vjs-control-text">Duration Time </span>' + '0:00'

So in this case we might have something like

'<span class="vjs-control-text">Stream Type </span>' + 'LIVE'

Not exactly sure on the right term to use there.

'aria-live': 'off'
});

el.appendChild(this.contentEl_);

return el;
};
7 changes: 7 additions & 0 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,13 @@ vjs.Player.prototype.onDurationChange = function(){
if (duration) {
this.duration(duration);
}

// Determine if the stream is live and propagate styles down to UI.
if (duration <= 0 || duration === window.INFINITY) {
Copy link
Member

Choose a reason for hiding this comment

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

Are there specific cases where if duration == 0, it means live? In other cases it just means we don't have the duration yet, so I think that could cause cases where it says it's live when it isn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. That should probably be duration < 0, not <=. On some platforms RTMP and HDS live can report -1 which is why I put that in there.

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?

}
};

/**
Expand Down