feat: Use ll-hls query directives and support skipping segments #1079

brandonocasey · 2021-02-22T21:18:09Z

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.

codecov · 2021-04-08T17:39:57Z

Codecov Report

Merging #1079 (c555607) into main (1c7a63b) will increase coverage by 0.09%.
The diff coverage is 91.81%.

❗ Current head c555607 differs from pull request most recent head e8cd59d. Consider uploading reports for the commit e8cd59d to get more accurate results

@@            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

Impacted Files	Coverage Δ
src/playlist-loader.js	`93.05% <90.90%> (+1.42%)`	⬆️
src/playlist.js	`94.28% <91.66%> (-0.22%)`	⬇️
src/manifest.js	`97.97% <100.00%> (+0.13%)`	⬆️
src/segment-loader.js	`95.82% <100.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c7a63b...e8cd59d. Read the comment docs.

brandonocasey · 2021-04-28T20:01:35Z

src/playlist-loader.js

@@ -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_({


move parseManifest to this.parseManifest as we weren't previously passing all the options in both calls.

brandonocasey · 2021-04-28T20:02:46Z

src/playlist-loader.js

@@ -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) => {


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.

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.

brandonocasey · 2021-04-28T20:03:17Z

src/playlist-loader.js

    return null;
  }

-  const mergedPlaylist = mergeOptions(playlist, media);
+  newMedia.segments = getAllSegments(newMedia);


preserve preloadSegment from newMedia during the merge that will happen below.

brandonocasey · 2021-04-28T20:03:54Z

src/playlist-loader.js


  // if the update could overlap existing segment information, merge the two segment lists
-  if (playlist.segments) {
+  if (oldMedia.segments) {
+    if (newMedia.skip) {


if we have skipped segments, add them back in so that we can merge information from the old media segments into them.

brandonocasey · 2021-04-28T20:04:28Z

src/playlist-loader.js

+
+      let uri = resolveUrl(this.master.uri, media.uri);
+
+      if (this.experimentalLLHLS) {


This is all new, basically use query directives and server side blocking if available.

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.

sure I will make this an && !endList

brandonocasey · 2021-04-28T20:10:13Z

src/playlist-loader.js

+
+  let currentMap;
+
+  for (let newIndex = 0; newIndex < newSegments.length; newIndex++) {


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.

brandonocasey · 2021-04-28T20:11:42Z

src/playlist-loader.js

+      result.push(updateSegment(oldSegment, newSegment));
+    } else {
+      // carry over map to new segment if it is missing
+      if (currentMap && !newSegment.map) {


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.

src/manifest.js

src/playlist.js

src/playlist-loader.js

gkatsev · 2021-05-24T21:02:23Z

src/playlist-loader.js

@@ -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) => {


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.

gkatsev · 2021-05-24T21:09:05Z

src/playlist-loader.js

+
+      let uri = resolveUrl(this.master.uri, media.uri);
+
+      if (this.experimentalLLHLS) {


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.

gkatsev · 2021-05-24T21:14:52Z

Also, was trying out https://conventionalcomments.org/. LMK what you think.

src/playlist-loader.js

gesinger · 2021-05-25T18:56:37Z

src/manifest.js

+  const parts = getLastParts(manifest);
+
+  if (parts.length && !manifest.partTargetDuration) {
+    const partTargetDuration = parts.reduce((acc, p) => Math.max(acc, p.duration), 0);


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.

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?

I think an error would be good in that case. Maybe we can reference the spec too.

We do log onwarn below. Is that sufficient?

I don't think a warning log is sufficient, as this goes against a requirement of the spec.

src/playlist-loader.js

test/playlist-loader.test.js

Co-authored-by: Garrett Singer <gesinger@gmail.com>

src/manifest.js

Co-authored-by: Garrett Singer <gesinger@gmail.com>

src/playlist-loader.js

Co-authored-by: Garrett Singer <gesinger@gmail.com>

brandonocasey mentioned this pull request Feb 22, 2021

feat: use EXT-X-PART for LL-HLS #1055

Merged

brandonocasey force-pushed the feat/llhls-3 branch from 3f21703 to e587c92 Compare March 3, 2021 15:22

brandonocasey force-pushed the feat/llhls-2 branch 2 times, most recently from eeba482 to 1161c6f Compare March 3, 2021 20:30

brandonocasey force-pushed the feat/llhls-2 branch 2 times, most recently from 3309677 to 37ae651 Compare March 12, 2021 18:50

brandonocasey added the blocked label Mar 30, 2021

Base automatically changed from feat/llhls-2 to main April 5, 2021 16:17

feat: Use ll-hls query directives and support skipping segments

3d39612

brandonocasey force-pushed the feat/llhls-3 branch from e587c92 to 3d39612 Compare April 6, 2021 17:18

brandonocasey and others added 4 commits April 7, 2021 17:11

Merge branch 'main' into feat/llhls-3

5e46a1e

Merge branch 'main' into feat/llhls-3

df957d4

unified manifest parse function

ff40332

tests for llhls and query directives in playlist-loader

53ec980

brandonocasey added needs: review and removed blocked labels Apr 8, 2021

Merge branch 'main' into feat/llhls-3

addf841

brandonocasey and others added 6 commits April 26, 2021 10:47

Merge branch 'main' into feat/llhls-3

285a4ae

remove unused option

a46f0a7

add duration to preload segment

9d1fc06

always add a part target duration when we have parts

00a8e25

query directive fixes

1ff48b0

small logging fix, bring map to skipped segments

3ff694a

brandonocasey force-pushed the feat/llhls-3 branch from 046fa94 to 3ff694a Compare April 28, 2021 19:59

brandonocasey commented Apr 28, 2021

View reviewed changes

brandonocasey added 2 commits April 29, 2021 11:18

cover all scenarios

2854967

fix logging and merging issues

a842b84

gkatsev reviewed May 24, 2021

View reviewed changes

brandonocasey and others added 3 commits May 24, 2021 17:21

Merge branch 'main' into feat/llhls-3

7898b4f

check for endlist before trying query directives

3ff79bc

code review

8ca1629

gkatsev reviewed May 25, 2021

View reviewed changes

src/playlist-loader.js Show resolved Hide resolved

use new media

941881f

gkatsev previously approved these changes May 25, 2021

View reviewed changes

gesinger reviewed May 25, 2021

View reviewed changes

Update src/playlist-loader.js

9288f11

Co-authored-by: Garrett Singer <gesinger@gmail.com>

brandonocasey dismissed gkatsev’s stale review via 9288f11 May 25, 2021 19:41

brandonocasey and others added 7 commits May 25, 2021 15:42

Update src/playlist-loader.js

adb47d2

Co-authored-by: Garrett Singer <gesinger@gmail.com>

Update src/playlist-loader.js

9744999

Co-authored-by: Garrett Singer <gesinger@gmail.com>

Update test/playlist-loader.test.js

1d2b8a2

Co-authored-by: Garrett Singer <gesinger@gmail.com>

remove uneeded comment and duplicate test

ee09077

separate code

a1d3176

add before/afterEach

c555607

log error on missing PART-TARGET

44b8830

gesinger reviewed May 26, 2021

View reviewed changes

src/manifest.js Outdated Show resolved Hide resolved

brandonocasey and others added 2 commits May 26, 2021 15:49

add spec, capitalize llhls

2aa89ab

Co-authored-by: Garrett Singer <gesinger@gmail.com>

add comment for _HLS_msn= logic

bba29c4

gesinger reviewed May 26, 2021

View reviewed changes

src/playlist-loader.js Outdated Show resolved Hide resolved

Update src/playlist-loader.js

e8cd59d

Co-authored-by: Garrett Singer <gesinger@gmail.com>

gesinger approved these changes May 26, 2021

View reviewed changes

brandonocasey merged commit 458be2c into main May 26, 2021

brandonocasey deleted the feat/llhls-3 branch May 26, 2021 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use ll-hls query directives and support skipping segments #1079

feat: Use ll-hls query directives and support skipping segments #1079

brandonocasey commented Feb 22, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

brandonocasey Apr 28, 2021

brandonocasey Apr 28, 2021

gkatsev May 24, 2021

brandonocasey Apr 28, 2021

brandonocasey Apr 28, 2021

brandonocasey Apr 28, 2021

gkatsev May 24, 2021

brandonocasey May 24, 2021

brandonocasey Apr 28, 2021

brandonocasey Apr 28, 2021

gkatsev May 24, 2021

gkatsev May 24, 2021

gkatsev commented May 24, 2021

gesinger May 25, 2021

brandonocasey May 25, 2021

gesinger May 26, 2021

gkatsev May 26, 2021

gesinger May 26, 2021


		let uri = resolveUrl(this.master.uri, media.uri);

		if (this.experimentalLLHLS) {


		let currentMap;

		for (let newIndex = 0; newIndex < newSegments.length; newIndex++) {

feat: Use ll-hls query directives and support skipping segments #1079

feat: Use ll-hls query directives and support skipping segments #1079

Conversation

brandonocasey commented Feb 22, 2021 • edited Loading

Description

codecov bot commented Apr 8, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Feb 22, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading