-
Notifications
You must be signed in to change notification settings - Fork 426
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 ll-hls query directives and support skipping segments #1079
Conversation
3f21703
to
e587c92
Compare
eeba482
to
1161c6f
Compare
3309677
to
37ae651
Compare
e587c92
to
3d39612
Compare
Codecov Report
@@ Coverage Diff @@
## main #1079 +/- ##
==========================================
+ Coverage 86.20% 86.30% +0.09%
==========================================
Files 39 39
Lines 9289 9362 +73
Branches 2127 2155 +28
==========================================
+ Hits 8008 8080 +72
- Misses 1281 1282 +1
Continue to review full report at Codecov.
|
046fa94
to
3ff694a
Compare
@@ -632,11 +715,9 @@ export default class PlaylistLoader extends EventTarget { | |||
|
|||
this.src = resolveManifestRedirect(this.handleManifestRedirects, this.src, req); | |||
|
|||
const manifest = parseManifest({ | |||
const manifest = this.parseManifest_({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move parseManifest to this.parseManifest
as we weren't previously passing all the options in both calls.
@@ -146,28 +177,36 @@ export const isPlaylistUnchanged = (a, b) => a === b || | |||
* master playlist with the updated media playlist merged in, or | |||
* null if the merge produced no change. | |||
*/ | |||
export const updateMaster = (master, media, unchangedCheck = isPlaylistUnchanged) => { | |||
export const updateMaster = (master, newMedia, unchangedCheck = isPlaylistUnchanged) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I continuously got confused by media
and playlist
here and realized that they are just terrible names. Most of the changes in this function, other than those noted, are for this rename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought (non-blocking): yeah, I'm sure have a lot of places where the terminology is confusing. We probably want to write up some guidelines around naming, particularly around words like media
, playlist
, etc.
return null; | ||
} | ||
|
||
const mergedPlaylist = mergeOptions(playlist, media); | ||
newMedia.segments = getAllSegments(newMedia); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preserve preloadSegment
from newMedia
during the merge that will happen below.
|
||
// if the update could overlap existing segment information, merge the two segment lists | ||
if (playlist.segments) { | ||
if (oldMedia.segments) { | ||
if (newMedia.skip) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have skipped segments, add them back in so that we can merge information from the old media segments into them.
src/playlist-loader.js
Outdated
|
||
let uri = resolveUrl(this.master.uri, media.uri); | ||
|
||
if (this.experimentalLLHLS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all new, basically use query directives and server side blocking if available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: do we want to skip these two server-control checks if we have an end list tag? The spec says that the server MUST ignore these, so, we may as well not do the calculations in those cases, if we don't already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure I will make this an && !endList
|
||
let currentMap; | ||
|
||
for (let newIndex = 0; newIndex < newSegments.length; newIndex++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was completely changed. Previously we used to create a new array from newSegments
and then loop over old segments, merging in old timing information and properties.
Now create an empty array for the result. Loop over every new segment and merge the accompanying old segment if we have one. Otherwise we just push the new segment into the list.
result.push(updateSegment(oldSegment, newSegment)); | ||
} else { | ||
// carry over map to new segment if it is missing | ||
if (currentMap && !newSegment.map) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big push for the refactor of this function was:
using HLS_skip=YES
also skips init segment maps that have already been seen, so we need to make sure that the map that was in use for the last old segment is merged into new segments.
@@ -146,28 +177,36 @@ export const isPlaylistUnchanged = (a, b) => a === b || | |||
* master playlist with the updated media playlist merged in, or | |||
* null if the merge produced no change. | |||
*/ | |||
export const updateMaster = (master, media, unchangedCheck = isPlaylistUnchanged) => { | |||
export const updateMaster = (master, newMedia, unchangedCheck = isPlaylistUnchanged) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought (non-blocking): yeah, I'm sure have a lot of places where the terminology is confusing. We probably want to write up some guidelines around naming, particularly around words like media
, playlist
, etc.
src/playlist-loader.js
Outdated
|
||
let uri = resolveUrl(this.master.uri, media.uri); | ||
|
||
if (this.experimentalLLHLS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: do we want to skip these two server-control checks if we have an end list tag? The spec says that the server MUST ignore these, so, we may as well not do the calculations in those cases, if we don't already.
Also, was trying out https://conventionalcomments.org/. LMK what you think. |
const parts = getLastParts(manifest); | ||
|
||
if (parts.length && !manifest.partTargetDuration) { | ||
const partTargetDuration = parts.reduce((acc, p) => Math.max(acc, p.duration), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the spec says:
4.4.3.7. EXT-X-PART-INF
The EXT-X-PART-INF tag provides information about the Partial
Segments in the Playlist. It is REQUIRED if a Playlist contains one
or more EXT-X-PART tags.
https://datatracker.ietf.org/doc/html/draft-pantos-hls-rfc8216bis-09#section-4.4.3.7
I think we may be better off throwing an error on the manifest if it's not available and parts are present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an ideal world I think we would, but we already guess at target duration above this so I think it makes sense to guess at partTargetDuration as well. Might be ok to log an error here though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an error would be good in that case. Maybe we can reference the spec too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do log onwarn
below. Is that sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a warning log is sufficient, as this goes against a requirement of the spec.
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Co-authored-by: Garrett Singer <[email protected]>
Description
Implements "query directives" when supported by the server so that we can get delta playlist updates to offset the cost of requesting a manifests so frequently.