Fix Interstitial snap-out at live edge and BUFFER_APPEND_NO_PROGRESS false positives - #7979
Conversation
506ff84 to
dcaab0c
Compare
…he end of the main playlist has passed the start by half a second Fixes #7978
06ee9c0 to
dc98ef0
Compare
| if (!progress) { | ||
| // Tracking miss | ||
| return; | ||
| } | ||
| const cycle = progress.stats === frag.stats ? progress : undefined; |
There was a problem hiding this comment.
This early return also disables the "count parsed fragments that produced no append operations" protection from #7941
I think that checking buffering stats should fix the same false positive (appends were queued but displaced before completing by transfer/end-of-stream), and the original tests should then pass unmodified:
- if (!progress) {
- // Tracking miss
- return;
- }
- const cycle = progress.stats === frag.stats ? progress : undefined;
+ const cycle = progress?.stats === frag.stats ? progress : undefined;
if (cycle?.errored) {
// Counted by the append-error path
return;
}
+ const fragBuffering = frag.stats.buffering;
+ if (fragBuffering.start > 0 && fragBuffering.first === 0) {
+ // Queued appends never completed (displaced by end-of-stream or transfer)
+ return;
+ }(A per-SourceBuffer in-flight counter would potentially be even more precise, but I didn't want to suggest introducing new stuff if you think the buffering stats we already have are enough)
rudemateo
left a comment
There was a problem hiding this comment.
Tested this branch against the stream in #7978. The resumption fix works: parts at the item start load as soon as they're published, and no more catchup rate creep during breaks.
One issue that reproduces now that primary buffers past the break: the asset ends ~33ms short of the resume point, and the gap-controller nudge out of that hole can land just before the boundary of the item that ended a few ms earlier. The position handler treats it as a backward seek and restarts the interstitial. With X-CONTENT-MAY-VARY=YES one session got an empty asset list back and the break collapsed:
138.649 [interstitials]: Stalled at 7.8899 of 8 in HlsAssetPlayer: ["synthmid-...259200-2" 769.60-777.60]
138.653 [interstitials]: INTERSTITIAL_ENDED ["synthmid-...259200" 753.60-777.60]
138.663 [gap-controller]: Nudging 'currentTime' from 777.489901 to 777.589901
138.672 [interstitials]: INTERSTITIAL_STARTED ["synthmid-...259200": 753.60-777.60]
138.755 [interstitials]: Loaded asset-list with duration: 0 (was: 24)
Ignoring regressions within a buffer hole of the item start (onSeeking in interstitials-controller, line 953) fixes it here without affecting seek-back replay:
if (
- (backwardSeek && currentTime < playingItem.start) ||
+ (backwardSeek &&
+ playingItem.start - currentTime > this.hls.config.maxBufferHole) ||
currentTime >= playingItem.end
) {The narrow edge-gate suggestion comment I left in interstitials-controller also prevents the nudge on this stream, so these should be complementary.
| ): boolean { | ||
| return fragment.duration - coverage <= MIN_BUFFERED_PROGRESS; | ||
| // .05 BUFFER_APPEND_NO_PROGRESS coverage tolerance accounts for large composition times | ||
| return fragment.duration - coverage <= 0.05; |
There was a problem hiding this comment.
Would <= config.maxBufferHole make sense here?
| this.onMediaDetaching(); | ||
| this.hls = null; | ||
| // @ts-ignore | ||
| this.config = null; |
There was a problem hiding this comment.
is this needed? it looks like this.config was removed from the constructor
| ) { | ||
| const details = this.primaryDetails; | ||
| if (details?.live && bufferPos >= details.edge) { | ||
| if (details?.live && bufferPos + 0.5 >= details.edge) { |
There was a problem hiding this comment.
Regarding the resumption TODO: I tested this branch with just the gate narrowed so primary resumes as soon as the playlist reaches the resume point
- if (details?.live && bufferPos + 0.5 >= details.edge) {
+ if (
+ details?.live &&
+ bufferPos - details.edge > ALIGNED_END_THRESHOLD_SECONDS
+ ) {combined with Line 1751 changed back to bufferedPos < details.edge.
Breaks in the #7978 stream are segment-aligned, so it looks like the +0.5 costs a full reload cycle at every cue-in. With the narrow gate the primary append merges across the boundary before the playhead gets there, and the end-of-asset stall/nudge shouldn't happen.
This PR will...
Why is this Pull Request needed?
These issues interfere with append-in-place buffering across scheduled mid-rolls at a low-latency live edge.
Are there any points in the code the reviewer needs to double check?
[DRAFT] TODO: Improve primary buffer resumption at low-latency edge
Resolves issues:
Fixes #7978