fix: preserve vhs-json initial ABR selection and pre-buffer paused players#1609
Draft
kpoojari-li wants to merge 3 commits into
Draft
fix: preserve vhs-json initial ABR selection and pre-buffer paused players#1609kpoojari-li wants to merge 3 commits into
kpoojari-li wants to merge 3 commits into
Conversation
…urces For vhs-json sources, PlaylistLoader.media() resolves via a no-XHR fast path that sets this.media_ without setting this.request. setupInitialPlaylist used `!this.request` as the signal that ABR had not yet selected a playlist, so for vhs-json it always fell back to playlists[0], overriding ABR's choice. Also check this.media_ so an existing selection is preserved. Updates scripts/dash-manifest-object.json to a multi-variant resolved manifest so the issue reproduces on the demo page. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vhs-json fires loadedmetadata synchronously, so the segment loader starts before handleUpdatedMediaPlaylist pauses it. With the initial-selection fix, the playlists[0] override no longer accidentally restarts it, so paused players with preload enabled stopped pre-buffering. Explicitly resume the loader after handleUpdatedMediaPlaylist, gated to VoD + preload to mirror the existing loadedmetadata pre-buffer condition; live behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1608.
Fixes two coupled
vhs-jsonbugs.Initial ABR selection was discarded.
PlaylistLoader.setupInitialPlaylistused!this.requestto decide whether ABR had already selected a playlist. Forvhs-json,media()resolves via a no-XHR fast path that setsthis.media_without ever settingthis.request, so the guard was always true and the selection was overridden back toplaylists[0]. Now also checksthis.media_.Paused players stopped pre-buffering. That
playlists[0]override had been accidentally restarting the segment loader. Sincevhs-jsonfiresloadedmetadatasynchronously (beforehandleUpdatedMediaPlaylistpauses the loader), removing the override left pausedpreloading players with no segment downloads. We now explicitly resume the loader afterhandleUpdatedMediaPlaylist, gated to VoD + preload to mirror the existingloadedmetadatapre-buffer condition (media.endList && tech_.preload() !== 'none'). Live behavior is unchanged.Why this wasn't caught earlier: the demo manifest (
scripts/dash-manifest-object.json) had only a single playlist, so theplaylists[0]override was a no-op there — with one variant there is no other selection for ABR to make, so the bug was invisible on the demo page. This PR updates that manifest to multiple resolved variants, which makes the issue reproducible and reviewable on the demo page.Specific Changes proposed
src/playlist-loader.js: guard is nowif (!this.request && !this.media_).src/playlist-controller.js: afterhandleUpdatedMediaPlaylist, callmainSegmentLoader_.load()forvhs-jsonVoD whenpreload !== 'none'.scripts/dash-manifest-object.json: updated to a multi-variant resolved manifest. The previous demo manifest had only a single playlist, so neither bug could surface on the demo page — with one variant there is no alternative for ABR to select, so theplaylists[0]override was never observable. Adding multiple resolved variants makes both issues reproducible on the demo page and gives reviewers a way to verify the fix.test/playlist-loader.test.jsandtest/playlist-controller.test.js: tests that fail without the fix and pass with it.Testing Done
Added unit tests in both
test/playlist-loader.test.jsandtest/playlist-controller.test.jsthat fail onmainand pass with this change (verified by reverting the source fixes and confirming only the two new tests fail). Also verified on the demo page with the updated multi-variant manifest.Before — on a throttled (3G) connection the player fetches
playlists[0](1920×1080, ~9.9 Mbps) first, then ABR corrects down to 320×180:issue-compressed.mp4
After — the ABR-selected variant is honored from the start, and a paused
preload="auto"player pre-buffers segments:fix-compressed.mp4
Requirements Checklist