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

brandonocasey · 2021-01-27T21:26:48Z

Description

Implements support for EXT-X-PART and EXT-X-PRELOAD-HINT
Requires videojs/m3u8-parser#137

Future pull requests

~~Use #EXT-X-PART-INF, query parameters for delta updates, #EXT-X-SERVER-CONTROL, and #EXT-X-SKIP~~ feat: Use ll-hls query directives and support skipping segments #1079
~~Use #EXT-X-PRELOAD-HINT aka preloadSegment on the manifest~~ feat: use serverControl and preloadSegment llhls features behind a flag #1078

Investigation

How would we use #EXT-X-RENDITION-REPORT when switching renditions? Is it only relevant in the context of delta updates?

brandonocasey · 2021-02-22T20:37:51Z

src/media-segment-request.js

@@ -971,7 +971,7 @@ export const mediaSegmentRequest = ({
  }

  const segmentRequestOptions = videojs.mergeOptions(xhrOptions, {
-    uri: segment.resolvedUri,
+    uri: segment.part && segment.part.resolvedUri || segment.resolvedUri,


we don't actually use uri anywhere 🤷 but for completeness sake I changed it.

brandonocasey · 2021-02-22T20:39:39Z

src/segment-loader.js

+        // In that case we need to reset partIndex and resync
+        if (this.partIndex && (!segment.parts || !segment.parts.length || !segment.parts[this.partIndex])) {
+          this.logger_(`part fell off on part ${this.partIndex}`);
+          this.resetLoader();


Not sure of a better way to do this. If we appended part 4 of 8 but parts fell off the manifest, they will no longer be request-able, so we kind of have to reset partIndex and the buffer. Perhaps the solution is to never use the first sequence of parts in a manifest?

We may be OK since:

EXT-X-PART tags SHOULD be removed from the Playlist after they are
greater than three Target Durations from the end of the Playlist.
https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-08#section-6.2.2

I imagine if we are close to the live edge we shouldn't fall that far back, and if we do, it may be worth doing something else (like seeking to live, or just reverting to standard playback which should happen with the reset).

brandonocasey · 2021-02-22T20:41:12Z

test/test-helpers.js

@@ -470,15 +475,23 @@ export const playlistWithDuration = function(time, conf) {
      segment.discontinuity = true;
    }

+    // add parts for the the last 3 segments in llhls playlists
+    if (conf && conf.llhls && (count - i) <= 3) {


add parts if llhls is set.

brandonocasey · 2021-02-22T20:41:37Z

test/test-helpers.js

  const remainder = time % targetDuration;
+  const count = Math.floor(time / targetDuration) + (remainder ? 1 : 0);


Don't append remainder after the loop, just do it in the for loop.

src/manifest.js

src/playlist-loader.js

gesinger · 2021-03-01T20:16:48Z

src/segment-loader.js

+        // In that case we need to reset partIndex and resync
+        if (this.partIndex && (!segment.parts || !segment.parts.length || !segment.parts[this.partIndex])) {
+          this.logger_(`part fell off on part ${this.partIndex}`);
+          this.resetLoader();


We may be OK since:

EXT-X-PART tags SHOULD be removed from the Playlist after they are
greater than three Target Durations from the end of the Playlist.
https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-08#section-6.2.2

I imagine if we are close to the live edge we shouldn't fall that far back, and if we do, it may be worth doing something else (like seeking to live, or just reverting to standard playback which should happen with the reset).

src/segment-loader.js

test/test-helpers.js

test/loader-common.js

scripts/index-demo-page.js

codecov · 2021-03-12T14:57:44Z

Codecov Report

Merging #1055 (74c785f) into main (87947fc) will decrease coverage by 0.06%.
The diff coverage is 81.60%.

@@            Coverage Diff             @@
##             main    #1055      +/-   ##
==========================================
- Coverage   86.17%   86.10%   -0.07%     
==========================================
  Files          38       38              
  Lines        8838     8905      +67     
  Branches     1965     1996      +31     
==========================================
+ Hits         7616     7668      +52     
- Misses       1222     1237      +15

Impacted Files	Coverage Δ
src/videojs-http-streaming.js	`90.58% <ø> (ø)`
src/playlist-loader.js	`91.16% <53.84%> (-4.40%)`	⬇️
src/manifest.js	`97.43% <84.61%> (-2.57%)`	⬇️
src/segment-loader.js	`95.30% <95.74%> (-0.06%)`	⬇️
src/media-segment-request.js	`95.73% <100.00%> (+0.01%)`	⬆️
src/source-updater.js	`94.83% <0.00%> (+0.32%)`	⬆️

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 87947fc...74c785f. Read the comment docs.

src/manifest.js

gesinger · 2021-03-16T20:10:43Z

src/segment-loader.js

+      // this can happen if we are going to load the first segment, but get a playlist
+      // update during that. mediaIndex would go from 0 to -1 if mediaSequence in the
+      // new playlist was incremented by 1.
+      if (this.mediaIndex < 0) {


I'm wondering if this will get us into trouble for short live playlists. Since we should start at the beginning of the playlist, the case may often happen that we end up with a mediaIndex of -1 while still processing the segment. In that case, the next segment we should load is segment 0 from the new playlist, which, if we do mediaIndex++, we'll be OK, unless we are at -2 or below. That may be a case for us to do a resync or seek.

Do other areas of the code have trouble with a mediaIndex of -1?

Don't we start at the end for live playlists?

The reason that I added this code was:

I have often seen issues with background tabs and live playlist refreshes happing way too slowly, so the segment we are on falls off of the playlist.

having a negative media index doesn't really make sense, as we are dealing with zero based arrays. If the segment we are on falls off of the current playlist then the current media index would no longer be a valid media index for referring to the segment that we have appended.

I think ultimately the difference will be that we will never have a negative mediaIndex. So if we were two segments behind mediaIndex would not become -2 and leave us in a bad state. It would become negative and we would get the currentMediaIndex for time.

It's possible that when mediaIndex and pendingSegment are both null, that we should probably try to jump closer to the live point in the stream. Right now we will just keep trying to request a segment at mediaIndex -1

Looking again, I think we may be OK. My concern was mainly for the case of 3 segment live playlists. We start at the beginning of the first segment in the playlist, and it will probably be pretty often that we'll update our playlist during a segment request. If that happens, we clear out mediaIndex here (since the mediaIndex was 0 and we'd go to -1), so when we get to checkBuffer for the next segment, we'd skip past mediaIndex++, but it looks like we'd still fetch at the end of buffer, so the guess should be OK:

http-streaming/src/segment-loader.js

Lines 1368 to 1378 in 82d83a7

} else if (this.fetchAtBuffer_) {

// Find the segment containing the end of the buffer

const mediaSourceInfo = Playlist.getMediaInfoForTime(

playlist,

lastBufferedEnd,

syncPoint.segmentIndex,

syncPoint.time

);

nextMediaIndex = mediaSourceInfo.mediaIndex;

startOfSegment = mediaSourceInfo.startTime;

src/playlist-loader.js

gesinger · 2021-03-16T20:31:31Z

src/segment-loader.js

+          this.logger_(`currently processing part (index ${this.partIndex}) no longer exists.`);
+          this.resetLoader();
+
+          this.mediaIndex = mediaIndex;


Is there a reason that we retain the media index after a reset?

We want to throw away the partIndex and the data associated with it, but the mediaIndex is still valid.
Example:
We are on mediaIndex 3 and partIndex 2, then a live refresh happens and we are on mediaIndex 2 and it no longer has parts.

👍 would be worth a comment there

brandonocasey force-pushed the poc-ll-hls branch from 1c60b25 to adf44fb Compare February 2, 2021 20:11

brandonocasey changed the title ~~WIP: use EXT-X-PART EXT-X-PRELOAD-HINT for LL-HLS~~ WIP: use EXT-X-PART for LL-HLS Feb 4, 2021

brandonocasey changed the title ~~WIP: use EXT-X-PART for LL-HLS~~ feat: use EXT-X-PART for LL-HLS Feb 10, 2021

brandonocasey force-pushed the poc-ll-hls branch from 18a5476 to 15f9cc4 Compare February 10, 2021 18:39

brandonocasey force-pushed the poc-ll-hls branch 2 times, most recently from 55ad96c to 1523e72 Compare February 22, 2021 20:25

brandonocasey commented Feb 22, 2021

View reviewed changes

brandonocasey force-pushed the poc-ll-hls branch from 1523e72 to 745697a Compare February 22, 2021 20:45

brandonocasey mentioned this pull request Feb 22, 2021

feat: use serverControl and preloadSegment llhls features behind a flag #1078

Merged

gesinger reviewed Mar 1, 2021

View reviewed changes

gkatsev reviewed Mar 1, 2021

View reviewed changes

scripts/index-demo-page.js Outdated Show resolved Hide resolved

brandonocasey force-pushed the poc-ll-hls branch from 1b45396 to e9704a3 Compare March 12, 2021 14:54

brandonocasey added 9 commits March 12, 2021 09:54

feat: Add ll-hls ext-x-part support behind a flag

123aba7

code review

32de23e

change option from llhls -> experimentalLLHLS

3f8ad12

update to camelcase

9c9493a

cr

73278fa

separate updateSegment function

e7598c6

m3u8-parser update

d3236ca

test fixes with m3u8-parser update

c2ebf7b

ie 11 fix

a8957ff

brandonocasey force-pushed the poc-ll-hls branch from e9704a3 to a8957ff Compare March 12, 2021 14:54

gesinger reviewed Mar 16, 2021

View reviewed changes

brandonocasey added 2 commits March 17, 2021 16:05

typo crs

82d83a7

cr add comment

c4b24d3

gesinger approved these changes Mar 19, 2021

View reviewed changes

Merge branch 'main' into poc-ll-hls

74c785f

brandonocasey merged commit b33e109 into main Mar 19, 2021

brandonocasey deleted the poc-ll-hls branch March 19, 2021 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

brandonocasey commented Jan 27, 2021 •

edited

Loading

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021 •

edited by gesinger

Loading

gesinger Mar 1, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

gesinger Mar 1, 2021

codecov bot commented Mar 12, 2021 •

edited

Loading

gesinger Mar 16, 2021

brandonocasey Mar 17, 2021

brandonocasey Mar 17, 2021

gesinger Mar 19, 2021

gesinger Mar 16, 2021

brandonocasey Mar 17, 2021

gesinger Mar 19, 2021

		const remainder = time % targetDuration;
		const count = Math.floor(time / targetDuration) + (remainder ? 1 : 0);

	} else if (this.fetchAtBuffer_) {
	// Find the segment containing the end of the buffer
	const mediaSourceInfo = Playlist.getMediaInfoForTime(
	playlist,
	lastBufferedEnd,
	syncPoint.segmentIndex,
	syncPoint.time
	);

	nextMediaIndex = mediaSourceInfo.mediaIndex;
	startOfSegment = mediaSourceInfo.startTime;

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

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

Conversation

brandonocasey commented Jan 27, 2021 • edited Loading

Description

Future pull requests

Investigation

Choose a reason for hiding this comment

brandonocasey Feb 22, 2021 • edited by gesinger Loading

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

codecov bot commented Mar 12, 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

brandonocasey commented Jan 27, 2021 •

edited

Loading

brandonocasey Feb 22, 2021 •

edited by gesinger

Loading

codecov bot commented Mar 12, 2021 •

edited

Loading