-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix Interstitial snap-out at live edge and BUFFER_APPEND_NO_PROGRESS false positives #7979
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1406,7 +1406,11 @@ transfer tracks: ${stringify(transferredTracks, (key, value) => (key === 'initSe | |
| const key = appendProgressKey(frag); | ||
| const progress = this.fragmentAppendProgress[key]; | ||
| delete this.fragmentAppendProgress[key]; | ||
| const cycle = progress?.stats === frag.stats ? progress : undefined; | ||
| if (!progress) { | ||
| // Tracking miss | ||
| return; | ||
| } | ||
| const cycle = progress.stats === frag.stats ? progress : undefined; | ||
| if (cycle?.errored) { | ||
| // Counted by the append-error path | ||
| return; | ||
|
|
@@ -2441,5 +2445,6 @@ function isFragmentFullyBuffered( | |
| coverage: number, | ||
| fragment: Fragment, | ||
| ): 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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -870,17 +870,17 @@ export default class InterstitialsController | |
| return; | ||
| } | ||
| const dataToAttach = | ||
| transferring && attachMediaSourceData ? attachMediaSourceData : { media }; | ||
| transferring && attachMediaSourceData | ||
| ? { ...attachMediaSourceData } | ||
| : { media }; | ||
| const schedule = this.schedule; | ||
| if (schedule) { | ||
| if (schedule && isAssetPlayer) { | ||
| const isAssetAtEndOfSchedule = | ||
| isAssetPlayer && | ||
| (player as HlsAssetPlayer).assetId === schedule.assetIdAtEnd; | ||
| // Prevent asset players from marking EoS on transferred MediaSource | ||
| dataToAttach.overrides = { | ||
| duration: schedule.duration, | ||
| endOfStream: | ||
| !isAssetPlayer || | ||
| isAssetAtEndOfSchedule || | ||
| (player as HlsAssetPlayer).appendInPlace === false, | ||
| }; | ||
|
|
@@ -1673,22 +1673,21 @@ export default class InterstitialsController | |
| ) { | ||
| const hls = this.hls; | ||
| const { loadingEnabled, bufferingEnabled, startPosition } = hls; | ||
|
|
||
| const instructToSeek = !skipSeekToStartPosition && hls.hasEnoughToStart; | ||
| const hasEnoughToStart = hls.hasEnoughToStart; | ||
|
|
||
| this.log( | ||
| `Start loading primary @${bufferPos} bufferedPos: ${this.bufferedPos} instructToSeek: ${instructToSeek} startPosition: ${startPosition} loadingEnabled: ${loadingEnabled} bufferingEnabled: ${bufferingEnabled}`, | ||
| `Start loading primary @${bufferPos} bufferedPos: ${this.bufferedPos} startPosition: ${startPosition} skip seek: ${skipSeekToStartPosition} has enough ${hasEnoughToStart} loadingEnabled: ${loadingEnabled} bufferingEnabled: ${bufferingEnabled}`, | ||
| ); | ||
| if ( | ||
| instructToSeek || | ||
| (!skipSeekToStartPosition && hasEnoughToStart) || | ||
| !loadingEnabled || | ||
| Math.abs(startPosition - bufferPos) > 0.1 | ||
| ) { | ||
| const details = this.primaryDetails; | ||
| if (details?.live && bufferPos >= details.edge) { | ||
| if (details?.live && bufferPos + 0.5 >= details.edge) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
| const bufferingItem = this.bufferingItem; | ||
| this.log( | ||
| `Resume primary loading when live reaches ${bufferPos} buffering item: ${bufferingItem ? segmentToString(bufferingItem) : null}`, | ||
| `Resume primary loading when live passes ${bufferPos} buffering item: ${bufferingItem ? segmentToString(bufferingItem) : null}`, | ||
| ); | ||
| hls.pauseBuffering(); | ||
| this.bufferPastEdge = true; | ||
|
|
@@ -1749,16 +1748,20 @@ export default class InterstitialsController | |
| } else if (this.bufferPastEdge) { | ||
| const details = this.primaryDetails; | ||
| const bufferedPos = this.bufferedPos; | ||
| if (details?.live && bufferedPos < details.edge) { | ||
| if (details?.live && bufferedPos + 0.5 < details.edge) { | ||
| this.bufferPastEdge = false; | ||
| const bufferingItem = this.bufferingItem; | ||
| this.log( | ||
| `Live edge ${details.edge} reached buffer: ${bufferedPos} buffering item: ${bufferingItem ? segmentToString(bufferingItem) : null}`, | ||
| `Live edge ${details.edge} passed buffer: ${bufferedPos} buffering item: ${bufferingItem ? segmentToString(bufferingItem) : null}`, | ||
| ); | ||
| const primaryWaiting = bufferingItem?.end === Infinity; | ||
| if (primaryWaiting) { | ||
| this.startLoadingPrimaryAt( | ||
| const playingItem = this.playingItem; | ||
| const skipSeekToStartPosition = | ||
| this.isInterstitial(playingItem) && playingItem.event.appendInPlace; | ||
| this.hls.startLoad( | ||
| Math.max(bufferedPos, bufferingItem.start), | ||
| skipSeekToStartPosition, | ||
| ); | ||
| } else { | ||
| const bufferingPlayer = this.getBufferingPlayer(); | ||
|
|
@@ -2312,8 +2315,11 @@ Schedule: ${scheduleItems.map((seg) => segmentToString(seg))} pos: ${this.timeli | |
| this.startLoadingPrimaryAt(bufferedPos); | ||
| } | ||
| } else { | ||
| // If not detached seek to resumption point | ||
| this.startLoadingPrimaryAt(bufferedPos); | ||
| // If not detached check playing item for seek to resumption point | ||
| const playingItem = this.playingItem; | ||
| const skipSeekToStartPosition = | ||
| this.isInterstitial(playingItem) && playingItem.event.appendInPlace; | ||
| this.startLoadingPrimaryAt(bufferedPos, skipSeekToStartPosition); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,18 @@ | ||
| import { interstitialsEnabled } from './base-stream-controller'; | ||
| import { ErrorDetails } from '../errors'; | ||
| import { Events } from '../events'; | ||
| import type { HlsConfig } from '../config'; | ||
| import type Hls from '../hls'; | ||
| import type { LevelDetails } from '../loader/level-details'; | ||
| import type { ComponentAPI } from '../types/component-api'; | ||
| import type { | ||
| ErrorData, | ||
| InterstitialAssetStartedData, | ||
| LevelUpdatedData, | ||
| MediaAttachingData, | ||
| } from '../types/events'; | ||
|
|
||
| export default class LatencyController implements ComponentAPI { | ||
| private hls: Hls | null; | ||
| private readonly config: HlsConfig; | ||
| private media: HTMLMediaElement | null = null; | ||
| private currentTime: number = 0; | ||
| private stallCount: number = 0; | ||
|
|
@@ -21,7 +21,6 @@ export default class LatencyController implements ComponentAPI { | |
|
|
||
| constructor(hls: Hls) { | ||
| this.hls = hls; | ||
| this.config = hls.config; | ||
| this.registerListeners(); | ||
| } | ||
|
|
||
|
|
@@ -34,7 +33,10 @@ export default class LatencyController implements ComponentAPI { | |
| } | ||
|
|
||
| get maxLatency(): number { | ||
| const { config } = this; | ||
| const config = this.hls?.config; | ||
| if (!config) { | ||
| return 0; | ||
| } | ||
| if (config.liveMaxLatencyDuration !== undefined) { | ||
| return config.liveMaxLatencyDuration; | ||
| } | ||
|
|
@@ -46,12 +48,12 @@ export default class LatencyController implements ComponentAPI { | |
|
|
||
| get targetLatency(): number | null { | ||
| const levelDetails = this.levelDetails; | ||
| if (levelDetails === null || this.hls === null) { | ||
| if (levelDetails === null || !this.hls) { | ||
| return null; | ||
| } | ||
| const config = this.hls.config; | ||
| const { holdBack, partHoldBack, targetduration } = levelDetails; | ||
| const { liveSyncDuration, liveSyncDurationCount, lowLatencyMode } = | ||
| this.config; | ||
| const { liveSyncDuration, liveSyncDurationCount, lowLatencyMode } = config; | ||
| const userConfig = this.hls.userConfig; | ||
| let targetLatency = lowLatencyMode ? partHoldBack || holdBack : holdBack; | ||
| if ( | ||
|
|
@@ -69,22 +71,25 @@ export default class LatencyController implements ComponentAPI { | |
| return ( | ||
| targetLatency + | ||
| Math.min( | ||
| this.stallCount * this.config.liveSyncOnStallIncrease, | ||
| this.stallCount * config.liveSyncOnStallIncrease, | ||
| maxLiveSyncOnStallIncrease, | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| set targetLatency(latency: number) { | ||
| if (!this.hls) { | ||
| return; | ||
| } | ||
| this.stallCount = 0; | ||
| this.config.liveSyncDuration = latency; | ||
| this.hls.config.liveSyncDuration = latency; | ||
| this._targetLatencyUpdated = true; | ||
| } | ||
|
|
||
| get liveSyncPosition(): number | null { | ||
| const liveEdge = this.estimateLiveEdge(); | ||
| const targetLatency = this.targetLatency; | ||
| if (liveEdge === null || targetLatency === null) { | ||
| if (liveEdge === null || targetLatency === null || !this.hls) { | ||
| return null; | ||
| } | ||
| const levelDetails = this.levelDetails; | ||
|
|
@@ -96,7 +101,7 @@ export default class LatencyController implements ComponentAPI { | |
| const min = edge - levelDetails.totalduration; | ||
| const max = | ||
| edge - | ||
| ((this.config.lowLatencyMode && levelDetails.partTarget) || | ||
| ((this.hls.config.lowLatencyMode && levelDetails.partTarget) || | ||
| levelDetails.targetduration); | ||
| return Math.min(Math.max(min, syncPosition), max); | ||
| } | ||
|
|
@@ -111,11 +116,11 @@ export default class LatencyController implements ComponentAPI { | |
|
|
||
| get edgeStalled(): number { | ||
| const levelDetails = this.levelDetails; | ||
| if (levelDetails === null) { | ||
| if (levelDetails === null || !this.hls) { | ||
| return 0; | ||
| } | ||
| const maxLevelUpdateAge = | ||
| ((this.config.lowLatencyMode && levelDetails.partTarget) || | ||
| ((this.hls.config.lowLatencyMode && levelDetails.partTarget) || | ||
| levelDetails.targetduration) * 3; | ||
| return Math.max(levelDetails.age - maxLevelUpdateAge, 0); | ||
| } | ||
|
|
@@ -138,6 +143,8 @@ export default class LatencyController implements ComponentAPI { | |
| this.unregisterListeners(); | ||
| this.onMediaDetaching(); | ||
| this.hls = null; | ||
| // @ts-ignore | ||
| this.config = null; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this needed? it looks like |
||
| } | ||
|
|
||
| private registerListeners() { | ||
|
|
@@ -150,6 +157,7 @@ export default class LatencyController implements ComponentAPI { | |
| hls.on(Events.MANIFEST_LOADING, this.onManifestLoading, this); | ||
| hls.on(Events.LEVEL_UPDATED, this.onLevelUpdated, this); | ||
| hls.on(Events.ERROR, this.onError, this); | ||
| hls.on(Events.INTERSTITIAL_ASSET_STARTED, this.onAssetStarted, this); | ||
| } | ||
|
|
||
| private unregisterListeners() { | ||
|
|
@@ -162,6 +170,7 @@ export default class LatencyController implements ComponentAPI { | |
| hls.off(Events.MANIFEST_LOADING, this.onManifestLoading, this); | ||
| hls.off(Events.LEVEL_UPDATED, this.onLevelUpdated, this); | ||
| hls.off(Events.ERROR, this.onError, this); | ||
| hls.off(Events.INTERSTITIAL_ASSET_STARTED, this.onAssetStarted, this); | ||
| } | ||
|
|
||
| private onMediaAttached( | ||
|
|
@@ -196,6 +205,28 @@ export default class LatencyController implements ComponentAPI { | |
| } | ||
| } | ||
|
|
||
| private onAssetStarted( | ||
| event: Events.INTERSTITIAL_ASSET_STARTED, | ||
| data: InterstitialAssetStartedData, | ||
| ) { | ||
| const hls = this.hls; | ||
| const media = | ||
| this.media || | ||
| (data.event.appendInPlace && | ||
| hls?.interstitialsManager?.playerQueue.reduce( | ||
| (found, player) => found || player.media, | ||
| null, | ||
| )); | ||
| if ( | ||
| hls && | ||
| media && | ||
| media.playbackRate > 1 && | ||
| media.playbackRate <= hls.config.maxLiveSyncPlaybackRate | ||
| ) { | ||
| this.changeMediaPlaybackRate(media, 1); | ||
| } | ||
| } | ||
|
|
||
| private onError(event: Events.ERROR, data: ErrorData) { | ||
| if (data.details !== ErrorDetails.BUFFER_STALLED_ERROR) { | ||
| return; | ||
|
|
@@ -211,19 +242,20 @@ export default class LatencyController implements ComponentAPI { | |
| private onTimeupdate = () => { | ||
| const { media } = this; | ||
| const levelDetails = this.levelDetails; | ||
| if (!media || !levelDetails) { | ||
| if (!media || !levelDetails || !this.hls) { | ||
| return; | ||
| } | ||
| this.currentTime = media.currentTime; | ||
|
|
||
| const config = this.hls.config; | ||
| const latency = this.computeLatency(); | ||
| if (latency === null) { | ||
| return; | ||
| } | ||
| this._latency = latency; | ||
|
|
||
| // Adapt playbackRate to meet target latency in low-latency mode | ||
| const { lowLatencyMode, maxLiveSyncPlaybackRate } = this.config; | ||
| const { lowLatencyMode, maxLiveSyncPlaybackRate } = config; | ||
| if ( | ||
| !lowLatencyMode || | ||
| maxLiveSyncPlaybackRate === 1 || | ||
|
|
@@ -245,10 +277,15 @@ export default class LatencyController implements ComponentAPI { | |
| ); | ||
| const inLiveRange = distanceFromTarget < liveMinLatencyDuration; | ||
|
|
||
| const playingInterstitial = | ||
| interstitialsEnabled(config) && | ||
| !!this.hls.interstitialsManager?.playingItem?.event; | ||
|
|
||
| if ( | ||
| inLiveRange && | ||
| distanceFromTarget > 0.05 && | ||
| this.forwardBufferLength > 1 | ||
| this.forwardBufferLength > 1 && | ||
| !playingInterstitial | ||
| ) { | ||
| const max = Math.min(2, Math.max(1.0, maxLiveSyncPlaybackRate)); | ||
| const rate = | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
(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)