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: use serverControl and preloadSegment llhls features behind a flag #1078

Merged
merged 30 commits into from
Apr 5, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
123aba7
feat: Add ll-hls ext-x-part support behind a flag
brandonocasey Jan 26, 2021
32de23e
code review
brandonocasey Mar 1, 2021
3f8ad12
change option from llhls -> experimentalLLHLS
brandonocasey Mar 2, 2021
9c9493a
update to camelcase
brandonocasey Mar 2, 2021
73278fa
cr
brandonocasey Mar 3, 2021
e7598c6
separate updateSegment function
brandonocasey Mar 3, 2021
d3236ca
m3u8-parser update
brandonocasey Mar 4, 2021
c2ebf7b
test fixes with m3u8-parser update
brandonocasey Mar 4, 2021
a8957ff
ie 11 fix
brandonocasey Mar 5, 2021
2ce6e59
feat(llhls): serverControl, preloadSegment, and partTargetDuration
brandonocasey Feb 10, 2021
0fa6dee
getHoldBack -> liveEdgeDelay
brandonocasey Mar 17, 2021
b4e219c
use part length
brandonocasey Mar 17, 2021
a58bcef
getAllSegments helper
brandonocasey Mar 17, 2021
83bf171
partSegments -> partsAndSegments
brandonocasey Mar 17, 2021
43720f0
msn -> mediaSequenceNumber
brandonocasey Mar 17, 2021
6ce8c1e
cr add comments
brandonocasey Mar 18, 2021
c3bef56
test text update
brandonocasey Mar 18, 2021
5d78a1f
better test
brandonocasey Mar 18, 2021
467832f
Merge remote-tracking branch 'origin/main' into feat/llhls-2
brandonocasey Mar 22, 2021
d35fa4d
code review
brandonocasey Mar 22, 2021
4374c0e
remove last three durations logic
brandonocasey Mar 25, 2021
e80c0fb
Merge branch 'main' into feat/llhls-2
brandonocasey Mar 29, 2021
b77c94c
Update src/playlist.js
brandonocasey Mar 30, 2021
dffa291
Merge remote-tracking branch 'origin/main' into feat/llhls-2
brandonocasey Mar 30, 2021
3aa8582
code review
brandonocasey Mar 30, 2021
1aa7785
null check
brandonocasey Mar 30, 2021
b381d96
Merge branch 'main' into feat/llhls-2
brandonocasey Mar 30, 2021
088ee6e
set to largest duration, add test
brandonocasey Apr 5, 2021
d7c30d2
Update test/manifest.test.js
brandonocasey Apr 5, 2021
01a6763
Merge remote-tracking branch 'origin/main' into feat/llhls-2
brandonocasey Apr 5, 2021
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
36 changes: 22 additions & 14 deletions src/playlist.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,24 @@ const getPartsAndSegments = (playlist) => (playlist.segments || []).reduce((acc,
return acc;
}, []);

const sumLastThreeDurations = function(partsAndSegments) {
// get the last three part/segment durations
let lastThreeDurations = 0;

if (partsAndSegments.length >= 3) {
for (let i = partsAndSegments.length - 1; i > partsAndSegments.length - 4; i--) {
// segment missing a duration, we cannot calculate
if (!partsAndSegments[i].duration) {
lastThreeDurations = 0;
break;
}
lastThreeDurations += partsAndSegments[i].duration;
}
}

return lastThreeDurations;
};

/**
* Get the number of seconds to delay from the end of a
* live playlist.
Expand All @@ -45,20 +63,10 @@ export const liveEdgeDelay = (master, media) => {
const partsAndSegments = getPartsAndSegments(media);
const hasParts = partsAndSegments.length &&
typeof partsAndSegments[partsAndSegments.length - 1].partIndex === 'number';

// get the last three part/segment durations
let lastThreeDurations = 0;

if (partsAndSegments.length >= 3) {
for (let i = partsAndSegments.length - 1; i > partsAndSegments.length - 4; i--) {
// segment missing a duration, we cannot calculate
if (!partsAndSegments[i].duration) {
lastThreeDurations = 0;
break;
}
lastThreeDurations += partsAndSegments[i].duration;
}
}
// by default we use the last three durations of segments if
// part target duration or target duration isn't found.
// see: https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-08#section-4.4.3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be missing something, but I don't think that section of the spec, if referring to HOLD-BACK, matches the comment here, since that is saying to use part target duration or target duration when the parameter is not available, and doesn't refer to the use of segment durations. Do we just have this in case the required target durations are missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we use three segment durations in case target durations are not available for whatever reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it's worth it to add the logic for this here if those are required attributes. It adds logic to the code that we shouldn't really encounter, and if we were to, then it's something that should be resolved by the creator of the stream, since target duration is always required (https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-08#section-4.4.3.1) and part target duration is required when parts are used (https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-08#section-4.4.3.7).

I think if we're trying to account for missing target durations we should do that at a higher level, when we first parse the manifest. It would help to simplify the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I will remove the last three durations logic.

Copy link
Member

Choose a reason for hiding this comment

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

What about just default to 10s target duration in case somehow it isn't set or whatever? While I agree that bad streams are bad, we should still try to play as much as possible and having a default, if bad value, is worth it. Plus, it should make things simpler than trying to figure out segment duration and using that for target duation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having a sort of default may be appropriate (along with a warning). We should probably add it at the point where we first parse the stream (i.e., in the loaders).

brandonocasey marked this conversation as resolved.
Show resolved Hide resolved
const lastThreeDurations = sumLastThreeDurations(partsAndSegments);

// dash suggestedPresentationDelay trumps everything
if (master && master.suggestedPresentationDelay) {
Expand Down
7 changes: 6 additions & 1 deletion src/segment-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -1426,8 +1426,13 @@ export default class SegmentLoader extends videojs.EventTarget {

this.logger_(`checkBuffer_ returning ${segmentInfo.uri}`, {
segmentInfo,
playlist,
currentMediaIndex,
currentPartIndex
currentPartIndex,
nextPartIndex,
nextMediaIndex,
startOfSegment,
isSyncRequest
});

return segmentInfo;
Expand Down
4 changes: 2 additions & 2 deletions test/playlist.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -750,15 +750,15 @@ QUnit.test('liveEdgeDelay works as expected', function(assert) {
assert.equal(
Playlist.liveEdgeDelay(master, media),
0,
'no segment durations live delay can be calculated'
'no segment durations, live delay can\'t be calculated'
);

media.segments.length = 0;

assert.equal(
Playlist.liveEdgeDelay(master, media),
0,
'no segments live delay can be calculated'
'no segments, live delay can\'t be calculated'
);
});

Expand Down