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.

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: suggestedPresentationDelay #698

Merged
merged 18 commits into from
Feb 4, 2020
Merged

feat: suggestedPresentationDelay #698

merged 18 commits into from
Feb 4, 2020

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Dec 11, 2019

Use the suggestedPresentation delay when available instead of calculating it like it is done in HLS.

Depends on videojs/mpd-parser#82

src/playlist.js Outdated Show resolved Hide resolved
src/playlist.js Outdated Show resolved Hide resolved
src/playlist.js Outdated Show resolved Hide resolved
@@ -925,6 +925,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
}

let media = this.masterPlaylistLoader_.media();
const suggestedPresentationDelay = this.masterPlaylistLoader_.master.suggestedPresentationDelay
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but might be better to move this closer to where it's used.

src/playlist.js Outdated
if (!playlist.segments.length) {
return 0;
}

let i = playlist.segments.length - 1;
let distanceFromEnd = playlist.segments[i].duration || playlist.targetDuration;
const safeDistance = distanceFromEnd + playlist.targetDuration * 2;
const safeDistance = liveEdgePadding || (distanceFromEnd + playlist.targetDuration * 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone wants to explicitly put 0 in the MPD for suggestedPresentationDelay? (IOP recommends it be at least minBufferTime, but nothing in spec prevents it from being set to 0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, that should be valid. I guess that's why having some tests added would be good.

src/playlist.js Outdated Show resolved Hide resolved
@@ -1973,6 +1974,7 @@ function(assert) {
Playlist.seekable = () => {
return videojs.createTimeRanges(mainTimeRanges);
};
this.masterPlaylistController.masterPlaylistLoader_.master = media;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but may be worth adding a comment saying it's just a placeholder, or putting in a different value, since the master should always reference either a master playlist object or a skeleton of one created to wrap around a media playlist.

test/playlist.test.js Outdated Show resolved Hide resolved
test/playlist.test.js Outdated Show resolved Hide resolved
test/playlist.test.js Outdated Show resolved Hide resolved
test/playlist.test.js Outdated Show resolved Hide resolved
test/playlist.test.js Show resolved Hide resolved
let lastSegmentDuration = playlist.segments[i - 1].duration || playlist.targetDuration;
const safeDistance = typeof liveEdgePadding === 'number' ? liveEdgePadding : lastSegmentDuration + playlist.targetDuration * 2;

if (safeDistance === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to account for any distance less than last segment duration?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I think we should only special-case 0 exactly. Any other value should be ran through the loop below to find the appropriate return value.

@gkatsev gkatsev merged commit c14fb43 into 1.x Feb 4, 2020
@gkatsev gkatsev deleted the delay branch February 4, 2020 21:40
gkatsev added a commit that referenced this pull request Feb 4, 2020
gkatsev added a commit that referenced this pull request Feb 4, 2020
gkatsev added a commit that referenced this pull request Feb 4, 2020
gkatsev added a commit that referenced this pull request Feb 4, 2020
gkatsev added a commit that referenced this pull request Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants