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

brandonocasey · 2021-02-22T20:47:38Z

Description

llhls features are already behind a flag, from the previous pull request #1055.

This implements usage of:

preloadSegment when it has parts. Think of this as the segment that is about to appear in the manifest, but we don't have a uri for yet.
severControl but only PART-HOLD-BACK and HOLD-BACK which specify how far behind a live playlist we should start
partTargetDuration in any way that we current use targetDuration, such as how much of a delay we should have on a playlist refresh

brandonocasey · 2021-02-22T20:48:20Z

src/master-playlist-controller.js

+
+    if (this.masterPlaylistLoader_.master.hasOwnProperty('suggestedPresentationDelay')) {
+      delay = this.masterPlaylistLoader_.master.suggestedPresentationDelay;
+    } else if (media.serverControl && media.serverControl['PART-HOLD-BACK'] && media.partTargetDuration) {


ll-hls has it's own version of when a playlist should be started called HOLD-BACK and PART-HOLD-BACK

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

src/master-playlist-controller.js

+    } else if (media.partTargetDuration) {
+      delay = media.partTargetDuration * 3;
+    } else if (media.targetDuration) {
+      delay = media.targetDuration * 3;


These are the default values for HOLD-BACK as per the spec, so even if a playlist doesn't have HOLD-BACK we can still set the default HOLD-BACK

brandonocasey · 2021-02-22T20:50:42Z

src/playlist-loader.js

-    // if the playlist is unchanged since the last reload or last segment duration
-    // cannot be determined, try again after half the target duration
-    delay = (media.targetDuration || 10) * 500;
+  const lastPart = lastSegment && lastSegment.parts && lastSegment.parts[lastSegment.parts - 1];


use the last part duration if it exists or the last segment duration

brandonocasey · 2021-02-22T20:50:54Z

src/playlist-loader.js

+
+  // if the playlist is unchanged since the last reload or last segment duration
+  // cannot be determined, try again after half the target duration
+  return (media.partTargetDuration || media.targetDuration || 10) * 500;


use partTargetDuration or targetDuration

brandonocasey · 2021-02-22T20:52:06Z

src/playlist-loader.js

@@ -304,7 +313,7 @@ export default class PlaylistLoader extends EventTarget {
    // merge this playlist into the master
    const update = updateMaster(this.master, playlist);

-    this.targetDuration = playlist.targetDuration;
+    this.targetDuration = playlist.partTargetDuration || playlist.targetDuration;


We use a playlists targetDuration in a few places outside of these files. In the case where we have partTargetDuration that is always what we will use as out "true" targetDuration, I understand that this is ambiguous though and I think we might want to rename this.targetDuration to something else, but that might be a breaking change. Should we deprecate this name?

brandonocasey · 2021-02-22T20:54:15Z

src/playlist.js

-  let segment;
-  const numSegments = playlist.segments.length;
+
+  const partSegments = playlist.segments.reduce((acc, segment, si) => {


We need this function to know about parts now. So we break put all of our segments and or parts into one list.

brandonocasey · 2021-02-22T21:07:53Z

src/segment-loader.js

    timeline
  } = segmentInfo;

+  const name = segmentInfo.segment.uri ? 'segment' : 'pre-segment';
+
  return [


Better logging in general, but especially needed now that we have parts.

brandonocasey · 2021-02-22T21:08:31Z

src/segment-loader.js

      const mediaSourceInfo = Playlist.getMediaInfoForTime(
        playlist,
-        currentTime,
+        this.fetchAtBuffer_ ? lastBufferedEnd : currentTime,


It was silly to have 10 lines duplicated for a single argument change.

brandonocasey · 2021-02-22T21:08:47Z

src/segment-loader.js

+      nextPartIndex = mediaSourceInfo.partIndex;
+    }
+
+    if (typeof nextPartIndex !== 'number' && playlist.segments[nextMediaIndex] && playlist.segments[nextMediaIndex].parts) {


if we don't have a nextPartIndex, check if we should.

codecov · 2021-03-12T18:56:00Z

Codecov Report

Merging #1078 (01a6763) into main (1e94680) will increase coverage by 0.06%.
The diff coverage is 94.66%.

@@            Coverage Diff             @@
##             main    #1078      +/-   ##
==========================================
+ Coverage   86.06%   86.13%   +0.06%     
==========================================
  Files          38       38              
  Lines        8923     8967      +44     
  Branches     1998     2020      +22     
==========================================
+ Hits         7680     7724      +44     
  Misses       1243     1243

Impacted Files	Coverage Δ
src/playlist.js	`93.20% <90.62%> (-0.27%)`	⬇️
src/playlist-loader.js	`91.40% <94.11%> (+0.24%)`	⬆️
src/manifest.js	`97.67% <100.00%> (+0.23%)`	⬆️
src/master-playlist-controller.js	`94.13% <100.00%> (+0.27%)`	⬆️
src/segment-loader.js	`95.32% <100.00%> (+0.02%)`	⬆️

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 1e94680...01a6763. Read the comment docs.

src/playlist.js

src/playlist-loader.js

gesinger · 2021-03-17T14:46:38Z

src/playlist.js

+  // dash suggestedPresentationDelay trumps everything
+  if (master && master.suggestedPresentationDelay) {
+    return master.suggestedPresentationDelay;


Might be worth pulling this up to short circuit the rest of the function.

gesinger · 2021-03-17T15:02:56Z

src/segment-loader.js

@@ -1352,41 +1360,36 @@ export default class SegmentLoader extends videojs.EventTarget {
    } else if (currentMediaIndex !== null) {
    // Under normal playback conditions fetching is a simple walk forward
      const segment = playlist.segments[currentMediaIndex];
+      const partIndex = typeof currentPartIndex === 'number' ? currentPartIndex : -1;


Is it possible for it to be a valid case where the currentMediaIndex is the segment we're supposed to be fetching will the partIndex is 0? Are the hinted segments available here to see?

currentPartIndex will come in as null, in which case we need to check if the current media index has parts at index 0. it could also come in as 0 and in that case we know we appended part 0 and need to look for part 1, or increment media index if we don't have it.

gesinger · 2021-03-17T15:05:12Z

src/segment-loader.js

-      startOfSegment,
-      isSyncRequest


These can actually be valuable to contain, in particular startOfSegment, which has been a source of various bugs we've encountered.

These are all on segmentInfo, which is my reasoning behind removing them. But we could keep them as they keep things a bit more relevant to the checkBuffer log.

I don't think these get logged except for here. Maybe worth adding to segmentInfoString?

I have the next pull request in the series (where I refactor our segment selection logic) that adds selection to segmentInfoString. I think we should wait until that pull request. I will revert this logging change here for now.

test/playlist.test.js

gesinger · 2021-03-17T15:08:01Z

test/playlist.test.js

+    2,
+    'uses last three part durations'
+  );
+


May want another test here with fewer than 3 parts available.

in a real scenario the last 3 segments will always have >= 2 parts. as having only one part is the same as not using parts at all. I will update the test for that.

src/playlist.js

test/playlist.test.js

gesinger · 2021-03-19T15:54:44Z

src/playlist.js

+  // we shouldn't ever end up using lastThreeDurations as targetDuration
+  // is usually required, but if we somehow get here, with a missing
+  // targetDuration we should handle it
  } else if (lastThreeDurations) {
    return lastThreeDurations;
  }


Since we shouldn't end up here, it's probably worth pulling all of that logic down into an else here and return either 0 or whatever we calculate as the last three. Might also be worth warning (if we don't warn elsewhere for missing target duration), or logging anyway to help us determine what's happening in the event of a missing target duration.

We use lastThreeDurations in the case where we have parts, but no partTargetDuration so that we get a more accurate hold back for ll-hls. We warn about target duration via the manifest parser warnings that now go to the debug log.

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

src/playlist.js

+  // 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


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?

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

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.

ok I will remove the last three durations logic.

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.

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).

gkatsev

While I don't see any major issues, I think there are a couple of small changes that should be made in playlist.js based on Garrett's comments. Would make that function slightly simpler.

src/playlist.js

gkatsev · 2021-03-29T21:46:11Z

src/playlist.js

+  // 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


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.

gkatsev · 2021-03-29T21:49:55Z

Also, this PR is now out of date with main.

src/playlist.js

gesinger · 2021-03-30T12:55:25Z

src/playlist.js

+  // 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


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).

Co-authored-by: Gary Katsevman <[email protected]>

gesinger · 2021-04-01T14:45:24Z

src/manifest.js

+    const lastDuration = lastSegment && Math.round(lastSegment) || 10;
+
+    if (onwarn) {
+      onwarn(`manifest has no targetDuration defaulting to ${lastDuration}`);


I think we may be better off using the largest segment duration found (and 10 if no segments included), since the TARGETDURATION is supposed to be the highest duration that will be encountered in a stream (and we will run into issues if the target duration is shorter than some segment durations).

Would also be worth having a test for it.

test/manifest.test.js

Co-authored-by: Garrett Singer <[email protected]>

brandonocasey · 2021-04-05T16:14:21Z

src/playlist.js

-    const lastPartDuration = lastSegment && lastSegment.duration || media.targetDuration;
-
-    return lastPartDuration + media.targetDuration * 2;
+    return media.targetDuration * 3;


This was changed in main via #1105

brandonocasey commented Feb 22, 2021

View reviewed changes

brandonocasey force-pushed the feat/llhls-2 branch from ec781dc to 6090bb5 Compare February 22, 2021 20:55

brandonocasey commented Feb 22, 2021

View reviewed changes

brandonocasey force-pushed the feat/llhls-2 branch from 6090bb5 to c3951b2 Compare February 22, 2021 21:12

brandonocasey mentioned this pull request Feb 22, 2021

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

Merged

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

brandonocasey force-pushed the feat/llhls-2 branch from 1161c6f to 3309677 Compare March 12, 2021 14:56

feat(llhls): serverControl, preloadSegment, and partTargetDuration

2ce6e59

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

gesinger reviewed Mar 17, 2021

View reviewed changes

brandonocasey added 3 commits March 18, 2021 10:28

cr add comments

6ce8c1e

test text update

c3bef56

better test

5d78a1f

gesinger reviewed Mar 19, 2021

View reviewed changes

Base automatically changed from poc-ll-hls to main March 19, 2021 16:01

brandonocasey added 2 commits March 22, 2021 12:12

Merge remote-tracking branch 'origin/main' into feat/llhls-2

467832f

code review

d35fa4d

gesinger reviewed Mar 24, 2021

View reviewed changes

brandonocasey and others added 2 commits March 25, 2021 16:39

remove last three durations logic

4374c0e

Merge branch 'main' into feat/llhls-2

e80c0fb

gkatsev reviewed Mar 29, 2021

View reviewed changes

gesinger reviewed Mar 30, 2021

View reviewed changes

brandonocasey and others added 5 commits March 30, 2021 10:58

Update src/playlist.js

b77c94c

Co-authored-by: Gary Katsevman <[email protected]>

Merge remote-tracking branch 'origin/main' into feat/llhls-2

dffa291

code review

3aa8582

null check

1aa7785

Merge branch 'main' into feat/llhls-2

b381d96

brandonocasey added the needs: review label Mar 30, 2021

gesinger reviewed Apr 1, 2021

View reviewed changes

set to largest duration, add test

088ee6e

gesinger previously approved these changes Apr 5, 2021

View reviewed changes

gesinger reviewed Apr 5, 2021

View reviewed changes

test/manifest.test.js Outdated Show resolved Hide resolved

Update test/manifest.test.js

d7c30d2

Co-authored-by: Garrett Singer <[email protected]>

brandonocasey dismissed gesinger’s stale review via d7c30d2 April 5, 2021 15:50

Merge remote-tracking branch 'origin/main' into feat/llhls-2

01a6763

brandonocasey commented Apr 5, 2021

View reviewed changes

gesinger approved these changes Apr 5, 2021

View reviewed changes

brandonocasey merged commit fa1b6b5 into main Apr 5, 2021

brandonocasey deleted the feat/llhls-2 branch April 5, 2021 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

brandonocasey commented Feb 22, 2021 •

edited

Loading

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

brandonocasey Feb 22, 2021

codecov bot commented Mar 12, 2021 •

edited

Loading

gesinger Mar 17, 2021

gesinger Mar 17, 2021

brandonocasey Mar 17, 2021

gesinger Mar 17, 2021

brandonocasey Mar 17, 2021

gesinger Mar 19, 2021

brandonocasey Mar 22, 2021

gesinger Mar 17, 2021

brandonocasey Mar 18, 2021

gesinger Mar 19, 2021 •

edited

Loading

brandonocasey Mar 22, 2021

gesinger Mar 24, 2021

brandonocasey Mar 24, 2021

gesinger Mar 24, 2021

brandonocasey Mar 25, 2021

gkatsev Mar 29, 2021

gesinger Mar 30, 2021

gkatsev left a comment

gkatsev Mar 29, 2021

gkatsev commented Mar 29, 2021

gesinger Mar 30, 2021

gesinger Apr 1, 2021

brandonocasey Apr 5, 2021

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

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

Conversation

brandonocasey commented Feb 22, 2021 • edited Loading

Description

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesinger Mar 19, 2021 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Mar 29, 2021

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 Mar 12, 2021 •

edited

Loading

gesinger Mar 19, 2021 •

edited

Loading