fix(mediastream): serve the requested episode on direct play, and fix dropped progress updates - #926
Open
Vincius-dev wants to merge 2 commits into
Open
Conversation
The direct play endpoint serves whichever file is currently loaded and its stream URL was the same for every file, so the browser reused the cached response of the previous episode. Add the file hash to the URL, reject requests for a file that is not loaded, and stop caching the response. Also ignore the stream URL of the previous episode while the new media container is being requested, otherwise the player loads it for a moment.
Two cases dropped the AniList progress update: - a PlaybackLoadedEvent for a new playback on the same target and client was rejected as stale, so every event of the new session was ignored - the CompletedEvent handler read the active playback state asynchronously, which a TerminatedEvent (auto next episode) could clear first The state is now captured when the event is ingested. Also drop the directstream fallback, its type assertion never matched a real stream and it ignored both the AutoUpdateProgress setting and the current entry progress.
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
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.
Fixes #925.
Opening this alongside the issue as a concrete reference for the approach. Happy to close it and discuss there first, or to split it into two PRs, since the two fixes are independent.
What problem does this solve?
Two bugs hit while watching a local library from a browser with direct play.
Going to the next episode plays the previous one.
/api/v1/mediastream/directserves whichever filecurrentMediaContainerpoints at, and every file was given that exact same URL. The URL identifies a slot, not a file, so the browser treats consecutive episodes as one resource and replays the cached bytes of the previous one. The endpoint also sent no cache headers, unlike/subsand/att. Devtools shows the browser revalidating the new episode with the previous file'sIf-Range, and "Disable cache" makes the bug go away.AniList progress is not updated. Two independent paths in
mediacoreswallow it, described in the issue: aPlaybackLoadedEventfor a new playback is rejected as stale, and theCompletedEventhandler reads the active playback state after the session may already have been cleared.What changed?
Direct play (commit 1)
DirectPlayStreamUrl(hash)builds/api/v1/mediastream/direct?hash=<hash>. The hash is already computed for everyMediaContainer, so each file now gets a distinct URL and the browser cannot mix them up.ServeEchoDirectPlayreturns 404 when the requested hash is not the loaded file. The check is skipped when the param is absent so a client holding an older stream URL keeps working.HandleMediastreamDirectPlaysendsno-store, same as the sibling media endpoints.filePathbefore the new container is requested, so without this the player mounts with the previous episode's URL for one render. Also switched the direct HMAC token separator to&, since the URL now carries a query param.Progress (commit 2)
PlaybackLoadedEventtakes over the session instead of being rejected as stale.CompletedEventis ingested, while it is still guaranteed to belong to that event, and the update runs from that snapshot. This mirrors whatTerminatedEventalready does for continuity.directstream.updateCompletedProgress. Its type assertion never matched a real stream, so it was dead code, and it ignored both theAutoUpdateProgresssetting and the current entry progress. Making it work would have introduced those two bugs, andmediacorealready covers the case correctly.Why this approach?
Putting the hash in the URL rather than, say, disabling the cache alone:
no-storeon its own fixes the symptom but the endpoint would still be able to serve a file the client did not ask for whenever the client and server are briefly out of sync. With the hash the client either gets the file it asked for or a 404, never the wrong episode silently.For the progress fix I kept the coordinator as the single owner of the update rather than reviving the directstream fallback, so the setting and the "progress already ahead" check stay in one place.
How was it tested?
go test ./internal/mediacore/... ./internal/mediastream/... ./internal/directstream/... ./internal/handlers/..., plusnpm run typecheckandnpm testfor the web changes.New tests, all of which fail without the corresponding fix:
internal/mediastream/directplay_test.go— the URL is unique per file, the loaded file is served, a hash for a file that is not loaded gets a 404 and does not leak the loaded file's content, and a request without a hash still works.internal/handlers/mediastream_test.go— the direct play response is never cacheable.internal/mediacore/mediacore_test.go— progress is updated on completion, for the next episode both with and without a terminate in between, when the session is torn down right after completion, and not at all when the setting is off.Note:
TestUsesPrivilegedCommandSettingsininternal/handlersalready fails on a clean checkout here, unrelated to this change.Risks and limitations
no-storemeans seeking far back may refetch instead of reading from the HTTP cache. Media playback buffers in memory anyway, and correctness matters more here./mediastream. The transcode path shares the same constant URL shape (master.m3u8), but HLS re-requests the playlist and the transcoder is restarted per file, so I left it alone rather than widening the change.AI Disclosure
mediacore, which already owns that effect, and the cache headers follow the existing/subsand/atthandlers. The tests usetestmockshelpers and live in{file}_test.goas CONTRIBUTING asks.