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

GAP Tag/Attribute Support with FRAG_GAP Error #5257

Merged
merged 8 commits into from Mar 23, 2023

Conversation

robwalch
Copy link
Collaborator

@robwalch robwalch commented Feb 23, 2023

This PR will...

Do not load GAP segments or parts. Error when encountering a segment with a GAP in a level to trigger a level switch (clients are expected to look for alternate segments to load when a GAP is encountered). Track segments with a GAP as partially buffered in fragment-tracker to avoid reload looping and expedite finding of the next available fragment. Allow the gap-controller to skip over unbufferable gaps (when level switch is not an option).

  • Segments with a GAP tag are not requested
  • Parts with a GAP=YES attribute are not requested
  • The first time a stream-controller goes to select a GAP segment for loading in a level a FRAG_GAP Error will be emitted
  • The FRAG_GAP error is handled by switching variants when possible
  • When no alternates are available, the next segment/part is selected

Why is this Pull Request needed?

Skips over content with gaps.

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

Future enhancements may include appending silence and/or still frames. Such enhancements are not in scope for this release.

Resolves issues:

#2940 (avoid loading and skips)

@robwalch robwalch changed the title Handle fragments and parts with a GAP tag/attr GAP Tag/Attribute Support with FRAG_GAP Error Feb 24, 2023
@robwalch robwalch added this to the 1.4.0 milestone Feb 24, 2023
@mtoczko
Copy link
Collaborator

mtoczko commented Feb 24, 2023

Hi @robwalch
There is a problem with this stream
https://mtoczko.github.io/hls-test-streams/test-gap-audio/playlist.m3u8.

[warn] > Trying to nudge playhead over buffer-hole
[warn] > Nudging 'currentTime' from 15.759513 to 15.859513
Error: Nudging 'currentTime' from 15.759513 to 15.859513 at GapController

The second case is when we have a large number of segments with a gap attribute. The player loops on a single fragment

[log] > [stream-controller]: SN 675 just loaded, load next one: 676
[log] > [stream-controller]: SN 675 just loaded, load next one: 676
[log] > [stream-controller]: SN 675 just loaded, load next one: 676
.....

Will prepare a more complex stream with more cases.

@robwalch robwalch force-pushed the feature/error-controller-pathway-penalty-network-policy branch from 33224cf to d23e53a Compare February 24, 2023 21:39
@robwalch
Copy link
Collaborator Author

robwalch commented Feb 24, 2023

Hi @mtoczko,

Thanks for taking a look.

There is a problem with this stream
https://mtoczko.github.io/hls-test-streams/test-gap-audio/playlist.m3u8.

I'm not seeing a nudge issue with that stream. With my latest changes gaps are jumped:

hls.js:7484 [warn] > skipping hole, adjusting currentTime from 0 to 4.059333
hls.js:7484 [warn] > skipping hole, adjusting currentTime from 15.957415 to 20.059333000000002

It is possible there is a stall first. I'm testing in Chrome, but maybe you're seeing different behavior in another browser?

The second case is when we have a large number of segments with a gap attribute

I haven't looked at samples with more than one or two consecutive GAP segments. There could be an issue that if another level is not available to switch to. The latest changes make sure that on GAP the a switch is made so that the player looks for segments without GAPs.

@robwalch robwalch force-pushed the feature/error-controller-pathway-penalty-network-policy branch from d23e53a to 90984f9 Compare February 25, 2023 00:14
@robwalch robwalch force-pushed the feature/gap-tag-handling branch 3 times, most recently from 50be628 to 026401c Compare February 25, 2023 01:31
@robwalch robwalch force-pushed the feature/error-controller-pathway-penalty-network-policy branch from e629562 to 981875c Compare February 25, 2023 03:32
Base automatically changed from feature/error-controller-pathway-penalty-network-policy to master February 27, 2023 19:31
@robwalch robwalch force-pushed the feature/gap-tag-handling branch 3 times, most recently from 8dc06f8 to 938a05e Compare March 1, 2023 06:47
@robwalch robwalch marked this pull request as ready for review March 1, 2023 06:53
@mtoczko
Copy link
Collaborator

mtoczko commented Mar 3, 2023

Hi @robwalch

It is possible there is a stall first. I'm testing in Chrome, but maybe you're seeing different behavior in another browser?

It appears so randomly, the same browser. A few hours difference after between tests.
I am attaching the log file.
working.log
stalled.log

@robwalch
Copy link
Collaborator Author

robwalch commented Mar 3, 2023

It appears so randomly, the same browser. A few hours difference after between tests. I am attaching the log file. working.log stalled.log

The difference in stalled.log is that the stall is detected with 0.257s of unplayed media in the buffer, so it chooses to nudge the playback because it hasn't reached the gap yet. In working.log, it stalls in the gap, recognizes that the gap fragments at this location and jumps.

Thinking about the best solution for this. I would have expected to see more nudging, or nudging up to the gap, and then the gap jump taking place.

@robwalch
Copy link
Collaborator Author

robwalch commented Mar 3, 2023

It looks like the stall is because the buffer is low enough to put the player in a waiting state (we don't check that) and the nudge only puts the player into a seeking state which will never resolve because the next fragment is a gap and the nudge only reduces the forward buffer.

I haven't been able to repro, but the code that is failing us here is in gap-controller. It's probably the dependency on activeFrag that's an issue (stream-controller passes fragCurrent or null and my guess is it is the last fragment or null).

if (seeking) {
// Waiting for seeking in a buffered range to complete
const hasEnoughBuffer = bufferInfo.len > MAX_START_GAP_JUMP;
// Next buffered range is too far ahead to jump to while still seeking
const noBufferGap =
!nextStart ||
(activeFrag && activeFrag.start <= currentTime) ||
(nextStart - currentTime > MAX_START_GAP_JUMP &&
!this.fragmentTracker.getPartialFragment(currentTime));
if (hasEnoughBuffer || noBufferGap) {
return;
}
// Reset moved state when seeking to a point in or before a gap
this.moved = false;

@mtoczko
Copy link
Collaborator

mtoczko commented Mar 3, 2023

Hi @robwalch
It looks like this:
Zrzut ekranu 2023-03-3 o 16 59 41

@robwalch
Copy link
Collaborator Author

robwalch commented Mar 3, 2023

It looks like this:

Yes. I can reproduce by seeking to with 0.1 (maxBufferHole) of the end of the first buffered TimeRange (video.currentTime = 15.85).

@mtoczko
Copy link
Collaborator

mtoczko commented Mar 13, 2023

Hi @robwalch
https://mtoczko.github.io/hls-test-streams/test-gap/playlist.m3u8
Player should be able to play the stream up to 00:01:53 using only HLS redundant.

@mtoczko
Copy link
Collaborator

mtoczko commented Mar 17, 2023

Hi @robwalch
I noticed that the video can get stuck at one point. What is a regression to branch master. There should be a switchover to the backup stream (B).

stream: https://mtoczko.github.io/hls-test-streams/test-gap/playlist.m3u8
Chrome 111, Safari 15.5

[log] > [stream-controller]: Skipping loaded main SN 9 at buffer end

log] > [stream-controller]: PARSED->IDLE
 [log] > [stream-controller]: Skipping loaded main SN 9 at buffer end
 [log] > [stream-controller]: Loading fragment 67 cc: 0 of [0-132] level: 0, target: 134
 [log] > [stream-controller]: IDLE->FRAG_LOADING
 [log] > [stream-controller]: FRAG_LOADING->PARSING
 [log] > [stream-controller]: Loaded fragment 67 of level 0
 [log] > [transmuxer.ts]: Flushed fragment 67 of level 0
 [log] > [stream-controller]: PARSING->PARSED
 [log] > [stream-controller]: Buffered main sn: 67 of level 0 (frag:[134.000-136.000] > buffer:[0.000-16.000][28.000-30.000][42.000-100.000][112.000-114.000][126.000-136.000])
 [log] > [stream-controller]: PARSED->IDLE
 [log] > [stream-controller]: Skipping loaded main SN 9 at buffer end
 [log] > [stream-controller]: Loading fragment 68 cc: 0 of [0-132] level: 0, target: 136
 [log] > [stream-controller]: IDLE->FRAG_LOADING
 [log] > [stream-controller]: FRAG_LOADING->PARSING
 [log] > [stream-controller]: Loaded fragment 68 of level 0
 [log] > [transmuxer.ts]: Flushed fragment 68 of level 0
 [log] > [stream-controller]: PARSING->PARSED
 [log] > [stream-controller]: Buffered main sn: 68 of level 0 (frag:[136.000-138.000] > buffer:[0.000-16.000][28.000-30.000][42.000-100.000][112.000-114.000][126.000-138.000])
 [log] > [stream-controller]: PARSED->IDLE
 [log] > [stream-controller]: Skipping loaded main SN 9 at buffer end
 [log] > [stream-controller]: Loading fragment 69 cc: 0 of [0-132] level: 0, target: 138
 [log] > [stream-controller]: IDLE->FRAG_LOADING

@robwalch
Copy link
Collaborator Author

Hi @robwalch I noticed that the video can get stuck at one point. What is a regression to branch master. There should be a switchover to the backup stream (B).

stream: https://mtoczko.github.io/hls-test-streams/test-gap/playlist.m3u8 Chrome 111, Safari 15.5

[log] > [stream-controller]: Skipping loaded main SN 9 at buffer end

What is the expectation here? That the player would switch to redundant path B after gaps are found in path A?

I found an issue with audio-stream-contoller buffering when the video buffer is segmented. Once I address that, I will circle back to the error handling and look at what's missing.

@robwalch robwalch force-pushed the feature/gap-tag-handling branch 2 times, most recently from e85d938 to eafcd6c Compare March 21, 2023 19:30
@robwalch
Copy link
Collaborator Author

robwalch commented Mar 21, 2023

Hi @mtoczko,

These changes fix most of the level switching and forward buffering issues with https://mtoczko.github.io/hls-test-streams/test-gap/playlist.m3u8

  • fecd4bf Handle forward buffering past GAP tags in alt audio Playlists
  • eafcd6c Fix GAP tag handling with Redundant Streams
  • c9461c0 Implement penalty box for Redundant Streams

@robwalch
Copy link
Collaborator Author

@mtoczko merging for upcoming beta release (v1.4.0-beta.1). #2940 will remain open for feedback (also feel free to drop comments here if you find something specific to these changes). Thank you for the excellent test assets.

@robwalch robwalch merged commit 3e959c2 into master Mar 23, 2023
14 of 15 checks passed
@robwalch robwalch deleted the feature/gap-tag-handling branch March 23, 2023 20:53
@robwalch robwalch mentioned this pull request Mar 27, 2023
5 tasks
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.

None yet

2 participants