Fix permanent stall loading fragment-hint parts of encrypted low-latency streams (v1.6.x) - #7976
Conversation
| // Same stale-reference hazard as above: `keyLoadedData.frag` | ||
| // is the pre-swap fragment when this load advanced onto the | ||
| // fragment hint; compare the current context instead (#7975). |
There was a problem hiding this comment.
Please do not add multi-line comments like this.
| // Same stale-reference hazard as above: `keyLoadedData.frag` | ||
| // is the pre-swap fragment when this load advanced onto the | ||
| // fragment hint; compare the current context instead (#7975). | ||
| if (!keyLoadedData || this.fragContextChanged(frag)) { |
There was a problem hiding this comment.
This change is OK since we only want to exit if frag != this.fragCurrent.
| keyLoadingPromise = this.keyLoader.load(frag).then((keyLoadedData) => { | ||
| if (!this.fragContextChanged(keyLoadedData.frag)) { | ||
| // Check the context against the live `frag` binding rather than | ||
| // `keyLoadedData.frag`: when the LL-HLS parts branch below advances | ||
| // this load onto the fragment hint (`frag = this.fragCurrent = | ||
| // part.fragment`), the key was requested with the pre-swap fragment, | ||
| // and comparing that stale reference reads as an abort. The load is | ||
| // still current — dropping it here parks the stream-controller in | ||
| // FRAG_LOADING with no loader in flight, so no load-policy timeout | ||
| // can ever fire and playback of encrypted low-latency streams stalls | ||
| // permanently (#7975). master is unaffected: it selects the part | ||
| // before arming the key promise. | ||
| if (!this.fragContextChanged(frag)) { |
There was a problem hiding this comment.
This is not correct. The problem is we're loading the key based on fragment selection that has not been finalized in the case of LL-HLS.
Changing fragContextChanged here only changes whether or not KEY_LOADED is emitted and state changed. The correct fix is to call keyLoader.load only once we have finalized fragment selection. I don't think we can, or want to, cherry-pick #7874, but the fix should involve a similar change.
Backports the ordering of video-dev#7874 to v1.6.x: extract key loading from the top of _doFragLoad into loadKeyFor() and call it only once fragment selection is final - after the LL-HLS part/hint swap in the parts branch, and after the loading-parts fallback in the whole-fragment path. Previously the key promise was armed with the fragment passed in, so when the parts branch advanced the load onto the fragment hint, both resolution-time context checks compared the stale pre-swap fragment against the new fragCurrent, read the mismatch as an abort, and resolved silently - leaving the stream-controller in FRAG_LOADING with no loader in flight and no reachable timeout. Video fragment loading stopped permanently on encrypted low-latency streams while the playlist kept polling. Fixes video-dev#7975 on the v1.6.x line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
37b6149 to
cdd71a3
Compare
|
Thanks for the review @robwalch — reworked as suggested. The context-check patch is gone; key loading now moves into a Re-validated a build of this branch against the same live Widevine LL-HLS stream: the resize storm that froze unpatched 1.6.18 within 1–3 toggles now survives 20 toggles with no stall — and unlike the previous check-only patch, this also eliminates the KEY_LOADING-parked sibling shape mentioned in #7975. tsc / eslint / prettier / 767 unit tests pass. |
github-project-automation
Bot
moved this to Top priorities
in HLS.js Release Planning and Backlog
github-project-automation
Bot
moved this from Top priorities
to Done
in HLS.js Release Planning and Backlog
2 participants
This PR will...
Fix a permanent video stall on the v1.6.x line when an encrypted LL-HLS load advances onto the fragment hint while its key-loading promise is in flight. Fixes #7975 for
patch/v1.6.x.Why is this Pull Request needed?
_doFragLoadarmedkeyLoadingPromisewith the fragment passed in, before the LL-HLS parts branch could advance the load onto the fragment hint (frag = this.fragCurrent = part.fragment). Both resolution-time context checks then compared the stale pre-swap fragment against the newfragCurrent, read the mismatch as an abort, and resolved silently — leaving the stream-controller inFRAG_LOADINGwith no loader in flight, no reachablefragLoadPolicytimeout, and no other escape indoTick. Video fragment loading stopped permanently while the media playlist kept polling and alt audio kept playing (full debug logs in #7975).Per review feedback, this now follows the approach of #7874 rather than patching the context checks: key loading moves into a
loadKeyFor()helper called only once fragment selection is finalized — after the part/hint selection in the parts branch, and after the loading-parts fallback in the whole-fragment path — with afragContextChanged(frag)bail after each call site, mirroring master.Are there any points in the code the reviewer needs to double check?
capLevelToPlayerSizeflapping) that froze the unpatched 1.6.18 within 1–3 toggles now survives 20 toggles with no stall — including theKEY_LOADING-parked sibling shape, which the earlier check-only patch did not cover. Only transient non-fatalbufferStalledError+ nudge during the storm.Context changed in KEY_LOADINGlog, as on master) whenfragwas hint-swapped without a part selection; the next tick reloads cleanly withloadingPartsoff.tsc,eslint,prettier, andnpm run test:unit(767 tests) pass on this branch.Resolves issues:
#7975 (v1.6.x line)
Checklist
patch/v1.6.xdeliberately; master already has this ordering via Load key for fragment of selected LL-HLS part #7874