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

Bugfix/ll hls endofstream #4516

Merged
merged 4 commits into from
Jan 20, 2022

Conversation

cjpillsbury
Copy link
Collaborator

@cjpillsbury cjpillsbury commented Jan 20, 2022

This PR will...

Fix a couple of end of stream issues with ll-hls content to ensure ended will still occur.

I've also created a simple TimeRangesMock for the testing side.

Since this is an ENDLIST/end of stream issue, I can't provide a test stream, but I can assist with starting/ending ll-hls streams for reviewers if needed (I'm in Chicago/US Central Time for a rough sense of availability)

Why is this Pull Request needed?

The conditions for detecting end of stream for are insufficient ll-hls content, which results in a stall instead of ended. Part of this is related to the fact that fragCurrent for playlists with parts may in fact be "ahead" of the current endSN. Part of this is because FragmentTracker/its usage to determine endOfStream does not account for parts.

Are there any points in the code the reviewer needs to double check?

I'm currently relying on the levelDetails.partList to detect end of stream conditions.

  1. I'm only checking the last part, but I believe it's possible that there could be a gap in the current implementation where hypothetically e.g. lastPart - 1 should also be checked and is missing from the buffer but lastPart is not.
  2. Aside from some of the basic conditional checks, this feels like it belongs in FragmentTracker. Unfortunately, there isn't equivalent modeling for activeParts when they're buffered, evicted, etc, so this would be a much larger effort. I've confirmed that we could check the last activePart against the relevant timeRanges in getState(), but this feels too operationally intensive for that method, something like
// ...
  else if (this.activeParts && this.activeParts.length) {
      const { activeParts } = this;
      const lastActivePart = activeParts[activeParts.length - 1];
      const partFragKey = getFragmentKey(lastActivePart.fragment); // or filter for all activeParts w/same key as fragment
      if (partFragKey === fragKey) {
        const timeRange = getTimeRangeForType(fragment.type);
        if (this.isBuffered(lastActivePart.start, lastActivePart.start + lastActivePart.duration) {
          return APPROPRIATE_STATE/STATES;
        }
      }
  }

Resolves issues:

Fixes #4463

Checklist

  • [x ] changes have been done against master branch, and PR does not conflict
  • [ x] new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

robwalch
robwalch previously approved these changes Jan 20, 2022
Comment on lines +145 to +149
// NOTE: Because of the way parts are currently parsed/represented in the playlist, we can end up
// in situations where the current fragment is actually greater than levelDetails.endSN. While
// this feels like the "wrong place" to account for that, this is a narrower/safer change than
// updating e.g. M3U8Parser::parseLevelPlaylist().
fragCurrent.sn >= levelDetails.endSN &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

…Stream instead of relying on inline 'as' type conversion.
@cjpillsbury cjpillsbury merged commit 6d636d2 into video-dev:master Jan 20, 2022
@robwalch robwalch added this to the 1.2.0 milestone Jan 20, 2022
littlespex pushed a commit to cbsinteractive/hls.js that referenced this pull request Dec 9, 2022
Clearing the buffer seems to be not enough for this case.
This avoids the choppy video start on HLS fMP4 playback.

Tested with Chrome 106 & Edge 106.

Closes video-dev#4516
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#EXT-X-ENDLIST behavior is different for low latency live streams
2 participants