fix(playback): claim Safari DV/HDR10 decode evidence and preserve dvvC in remuxes - #617
Conversation
Follow-up to #613 for #609: Safari 26 reports dynamic-range: standard even on an XDR display, and answers canPlayType "probably" only for dvh1/hvc1 sample entries, never dvhe/hev1. The web probe gated every structured HDR claim on that media query and probed only dvhe, so the planner still saw empty hdr_details and terminated with hdr_transcode_unsupported. - Run the Dolby Vision and HDR10 shape probes unconditionally; the dynamic-range query survives only as the best-effort hdr output boolean. - Probe dvh1+dvhe per DV profile and hvc1+hev1 for the exact HDR10 Media Capabilities shape; either definitive answer earns the claim. - Tag preserved-DV remuxes dvh1 with -strict unofficial: FFmpeg omits the dvvC configuration record under either tag without it, so the previous dvhe output carried no DV signaling at all. - Scale the screen-derived max_resolution by devicePixelRatio so 2x panels stop advertising 1080p. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughHDR capability detection now probes compatible HDR10 and Dolby Vision sample entries independently of output HDR state. Dolby Vision preserve remuxing emits ChangesPlayback V3 capability and error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to The PR improves Safari HDR/Dolby Vision detection and preserved-DV remuxing, but a remaining subtitle adaptation branch can report the wrong playback failure when transcoding is disabled for another reason. Merge readiness is moderate until that behavior is corrected and regression-tested; documentation scope and preserve-mode profile handling also need explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant PlaybackPlanner
participant PlaybackSession
participant VideoPlayer
participant Toast
PlaybackPlanner->>PlaybackSession: Return subtitle_conversion_unsupported
PlaybackSession-->>VideoPlayer: Provide refusal title and message
VideoPlayer->>VideoPlayer: Roll back subtitle selection
VideoPlayer->>Toast: Display refusal title and description
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce2276cba9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const HDR10_PROGRESSIVE_CONFIGURATIONS = [ | ||
| 'video/mp4; codecs="hvc1.2.4.L153.B0"', | ||
| 'video/mp4; codecs="hev1.2.4.L153.B0"', |
There was a problem hiding this comment.
When Safari supports only the new hvc1 configuration—as the adjacent comment explicitly expects—this probe advertises HDR10 support, but the Profile 7 strip branch in internal/playback/remux.go only applies dovi_rpu and leaves copied HEVC with FFmpeg's default hev1 tag. The planner can therefore select the progressive HDR10 fallback and return the exact sample entry Safari rejected; either emit hvc1 for that recipe or do not accept an hvc1-only result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid coherence gap — fixed in a3a6b9c. The explicit v3 strip_to_hdr10 recipe now emits -tag:v hvc1, so the delivered sample entry matches the hvc1 evidence the web probe accepts (no -strict needed — stripped output carries no DOVI record). Legacy/auto strips keep FFmpeg's default hev1 for pre-v3 consumers. One note on the failure claim: field evidence from #609 shows Safari plays hev1-tagged remuxes fine (the Aug 9 sessions) — canPlayType is conservative, not a hard decode gate — but plan evidence and delivered bytes should agree regardless. Covered by TestExplicitStripModeTagsHVC1 through the mode switch.
| 8: { | ||
| mimes: ['video/mp4; codecs="dvh1.08.06"', 'video/mp4; codecs="dvhe.08.06"'], |
There was a problem hiding this comment.
When a browser returns probably only for dvhe—a scenario added to the tests—this fallback advertises the Dolby Vision profile even though the preserve recipe now unconditionally tags its output as dvh1 in internal/playback/remux.go. The resulting plan is backed by evidence for a different sample entry and can send the browser a format it reported as unsupported; probe only dvh1 or carry the selected entry through to the remux recipe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in a3a6b9c. The dvhe fallback probe is removed: only a definitive dvh1.05.06/dvh1.08.06 answer earns the Dolby Vision claim, matching the dvh1 sample entry the preserve remux emits. A dvhe-only browser now degrades to the validated HDR10 route instead of receiving a file whose fourcc it never attested. Tests updated (does not promote a dvhe-only answer to a Dolby Vision claim) and the protocol doc now states the probe-must-match-delivered-bytes rule explicitly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/playback/remux_dv_test.go (1)
89-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover all preserve routes.
The production branch handles both
dvProfile == 5anddvProfile == 8, but this test exercises only Profile 8. It also passestruedirectly tobuildRemuxArgs, so it cannot catch a regression whereRemuxDVPreserveV3stops settingtagDVSampleEntry. Add table-driven cases for Profiles 5 and 8, and test the preserve-mode-to-argument mapping through a pure helper or a focused seam.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/playback/remux_dv_test.go` around lines 89 - 102, Expand the remux argument tests around buildRemuxArgs to cover preserved Dolby Vision Profiles 5 and 8, using table-driven cases that verify both dvh1 tagging and unofficial strict mode. Add a focused pure helper or test seam so the tests exercise the RemuxDVPreserveV3-to-tagDVSampleEntry mapping rather than passing the preserve flag directly, and retain coverage for legacy and stripped profiles.internal/playback/remux.go (1)
182-188: 🗄️ Data Integrity & Integration | 🔵 TrivialVerify the deployed FFmpeg capability.
This path depends on the resolved FFmpeg binary accepting
dvh1and writing thedvvCbox. The supplied code only documents verification on FFmpeg 7.1.4. It does not show a deployment pin or a capability check. Add a CI or deployment probe that remuxes a Dolby Vision fixture and verifies both the sample entry anddvvCbox, or enforce the supported FFmpeg build. Otherwise, a binary difference can produce an MP4 without Dolby Vision signaling. FFmpeg’s MOV muxer writesdvcC/dvvConly when strict compliance isunofficialor lower. (ffmpeg.org)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/playback/remux.go` around lines 182 - 188, Ensure the deployment uses a supported FFmpeg build for the remux path around the dvh1 tagging arguments, or add a CI/deployment capability probe using a Dolby Vision fixture. The probe must verify that remuxing with dvh1 and strict unofficial preserves both the expected dvh1 sample entry and the dvvC configuration box; prevent deployment when either check fails.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/playback/remux_dv_test.go`:
- Around line 89-102: Expand the remux argument tests around buildRemuxArgs to
cover preserved Dolby Vision Profiles 5 and 8, using table-driven cases that
verify both dvh1 tagging and unofficial strict mode. Add a focused pure helper
or test seam so the tests exercise the RemuxDVPreserveV3-to-tagDVSampleEntry
mapping rather than passing the preserve flag directly, and retain coverage for
legacy and stripped profiles.
In `@internal/playback/remux.go`:
- Around line 182-188: Ensure the deployment uses a supported FFmpeg build for
the remux path around the dvh1 tagging arguments, or add a CI/deployment
capability probe using a Dolby Vision fixture. The probe must verify that
remuxing with dvh1 and strict unofficial preserves both the expected dvh1 sample
entry and the dvvC configuration box; prevent deployment when either check
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51333b68-2b0e-40fc-a5e1-143453599ee8
📒 Files selected for processing (6)
docs/architecture/playback-protocol-v3.mdinternal/playback/remux.gointernal/playback/remux_dv_test.goweb/src/player/hooks/useCodecDetection.test.tsweb/src/player/hooks/useCodecDetection.tsweb/src/player/hooks/usePlaybackSession.test.ts
…x emits Review follow-ups on #617 (Codex P1s): - Probe only dvh1: a browser answering "probably" solely for dvhe has given no evidence for the dvh1-tagged file the preserve remux delivers, so that answer no longer earns a Dolby Vision claim; such browsers keep the validated HDR10 fallback. - Label the explicit v3 HDR10 strip output hvc1 so the file matches the hvc1 evidence the web probe accepts; legacy/auto strips keep hev1. - Cover preserve tagging for profiles 5 and 8, and exercise the mode-to-argument mapping through StartRemuxWithDVMode instead of passing the tag flag directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the review findings in a3a6b9c: Codex P1 (dvhe fallback) — fixed. DV claims now require a definitive Codex P1 (hvc1/hev1 mismatch on the strip route) — fixed. The explicit v3 CodeRabbit (test coverage) — done. The preserve-tag test is table-driven over profiles 5 and 8, and two new tests ( CodeRabbit (CI/deployment dvvC probe) — not in this PR, as already noted in the PR body's follow-up list: a box-level integration check needs a real Dolby Vision fixture, and the deployment pins jellyfin-ffmpeg in the image. Tracking as the fixture-based integration test follow-up. Gates re-run after the changes: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/playback/remux.go (1)
287-295: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict
RemuxDVPreserveV3to supported profiles.
remuxDVModeForPlanV3selects preserve mode for any nonzero, non-7 profile with a Dolby Vision claim.buildRemuxArgsWithAudioV3emitsdvh1and-strict unofficialonly for profiles 5 and 8. The scanner and request validation accept other positive profiles, so those streams can start preserve remuxing without the required Dolby Vision signaling. Reject unsupported profiles or add signaling support for every accepted profile. Add a regression test for an unsupported profile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/playback/remux.go` around lines 287 - 295, Restrict RemuxDVPreserveV3 to Dolby Vision profiles with complete signaling support, aligning remuxDVModeForPlanV3 and buildRemuxArgsWithAudioV3 so unsupported positive profiles cannot enter preserve mode; alternatively add signaling for every accepted profile. Preserve the existing profile 7 rejection, and add a regression test verifying an unsupported profile is rejected before remuxing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/playback-protocol-v3.md`:
- Around line 390-393: Update the HDR10 remux description in the surrounding
protocol documentation to scope the hvc1 output claim explicitly to the v3 HDR10
strip path; state that legacy and automatic strip paths continue emitting hev1,
and ensure the Media Capabilities probing rule is not presented as applying to
those other paths.
---
Outside diff comments:
In `@internal/playback/remux.go`:
- Around line 287-295: Restrict RemuxDVPreserveV3 to Dolby Vision profiles with
complete signaling support, aligning remuxDVModeForPlanV3 and
buildRemuxArgsWithAudioV3 so unsupported positive profiles cannot enter preserve
mode; alternatively add signaling for every accepted profile. Preserve the
existing profile 7 rejection, and add a regression test verifying an unsupported
profile is rejected before remuxing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6b36d7d-2263-4da1-9187-2456cd01c961
📒 Files selected for processing (6)
docs/architecture/playback-protocol-v3.mdinternal/playback/remux.gointernal/playback/remux_dv_test.goweb/src/player/hooks/useCodecDetection.test.tsweb/src/player/hooks/useCodecDetection.tsweb/src/player/hooks/usePlaybackSession.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/player/hooks/usePlaybackSession.test.ts
- web/src/player/hooks/useCodecDetection.ts
- web/src/player/hooks/useCodecDetection.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3a6b9ceb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const configuration of HDR10_PROGRESSIVE_CONFIGURATIONS) { | ||
| try { | ||
| const result = await navigator.mediaCapabilities.decodingInfo(configuration); | ||
| if (result.supported && result.smooth) return true; |
There was a problem hiding this comment.
When decodingInfo rejects the first hvc1 configuration but accepts the hev1 fallback—the new test explicitly models this—the loop returns true, yet RemuxDVStripToHDR10V3 now hard-tags the delivered file as hvc1 in internal/playback/remux.go. The planner can therefore select the HDR10 fallback using evidence for the sample entry the browser rejected. Fresh evidence relative to the earlier comment is that the remux now emits hvc1, while this updated loop still accepts an hev1-only result; either require hvc1 support or propagate the accepted entry to the remux.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in 10e65fa. The HDR10 probe now queries only hvc1.2.4.L153.B0; the hev1 fallback is removed, so an hev1-only decodingInfo answer earns no claim, matching the hvc1 tag the explicit v3 strip remux emits. The test that modeled hev1-only acceptance is inverted (does not promote an hev1-only answer to an HDR10 claim).
…transcode Selecting a PGS/VOBSUB track on a client without a bitmap renderer forces a burn-in transcode. When that transcode cannot run, the terminal blamed the HDR pipeline (hdr_transcode_unsupported) or the 4K policy (no_alternate_version) — problems that were not blocking playback, since deselecting the subtitle restores the previous route. - When the burn requirement is the sole trigger of the adaptation, the HDR and 4K-policy terminals emit subtitle_conversion_unsupported with a message naming the burn requirement and the actual blocker. A range the client genuinely cannot take keeps the HDR terminal. - describePlanTerminal passes the server's subtitle message through instead of flattening every subtitle_* reason to one generic sentence. - The web player toasts the refusal when it rolls the subtitle selection back; previously the only surface was the quality menu, which a user who just picked a subtitle has no reason to open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
98ca5e3 adds a related UX fix from field testing: when a bitmap subtitle (PGS/VOBSUB) selection forces a burn-in transcode that cannot run, the refusal was misattributed and invisible. Server — when the burn requirement is the sole trigger of the video adaptation (video, range, and quality all otherwise permit a source-preserving route), the HDR and 4K-policy terminals now emit Web — a refused Regression tests: subtitle-beats-HDR, HDR-still-wins-when-range-genuinely-unsupported, subtitle-beats-4K-policy on the Go side (verified they fail with the old reasons when the fix is disabled); toast-on-rollback with server title, generic-title fallback, and no-double-toast on the web side. Gates: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/playback/plan_v3.go`:
- Around line 706-710: Update the disabled-transcoding branch in the V3 planning
function to return subtitle_conversion_unsupported only when
subtitle.RequiresBurn and subtitleForcedAdaptation are both true; otherwise
return transcoding_disabled. Add a regression test covering burn-in subtitles
combined with unsupported HDR or video adaptation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3519542-8505-45c5-b0bd-c722149f9aed
📒 Files selected for processing (8)
docs/architecture/playback-protocol-v3.mdinternal/playback/plan_v3.gointernal/playback/protocol_v3_test.goweb/src/player/components/VideoPlayer.test.tsxweb/src/player/components/VideoPlayer.tsxweb/src/player/components/WatchPage.tsxweb/src/player/playback-errors.test.tsweb/src/player/playback-errors.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture/playback-protocol-v3.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ca5e3a9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if subtitle.RequiresBurn { | ||
| reason = "subtitle_conversion_unsupported" | ||
| return terminalPlannerResultV3("subtitle_conversion_unsupported", "The selected subtitle must be burned into the video, but transcoding is unavailable.", false) |
There was a problem hiding this comment.
When transcoding is disabled and a burn-in subtitle is selected alongside another adaptation trigger—such as an unsupported video codec/range or an explicit quality reduction—this branch returns subtitle_conversion_unsupported solely because subtitle.RequiresBurn is true. Deselecting the subtitle still leaves the other trigger and therefore does not restore playback, but the newly exposed message and toast tell the user that the subtitle is the blocker. Use subtitleForcedAdaptation here as well; otherwise retain transcoding_disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid in principle — fixed in 10e65fa: the branch now gates on subtitleForcedAdaptation instead of subtitle.RequiresBurn, so a burn selection combined with another adaptation trigger returns transcoding_disabled. One nuance found while writing the regression test: with transcoding disabled, ResolveSubtitlePolicyV3 never grants RequiresBurn at all — a bitmap selection terminals as subtitle_conversion_unsupported in the subtitle policy before this branch runs, and that attribution is accurate (the selection is genuinely undeliverable regardless of the other causes). So the misleading combination was unreachable today; the guard keeps the sole-cause invariant if that ordering ever changes, and the test documents the actual behavior.
…ate the disabled-transcode subtitle terminal Review follow-ups on #617: - The HDR10 probe accepts only hvc1.2.4.L153.B0: the explicit v3 strip remux labels its output hvc1, so an hev1-only decodingInfo answer is evidence for bytes Silo never sends and earns no claim (Codex P1). - The disabled-transcode branch blames the subtitle only when the burn requirement was the sole adaptation trigger; other causes keep transcoding_disabled. In practice a bitmap selection without transcode terminals in the subtitle policy before this branch, but the guard keeps the sole-cause invariant if that ordering ever changes (CodeRabbit / Codex P2). - Protocol doc scopes the hvc1 labeling to the explicit v3 strip path; legacy/auto strips keep hev1 (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10e65fafb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| toast.error(replanErrorTitle ?? "That subtitle track can't be used", { | ||
| description: replanError, | ||
| }); |
There was a problem hiding this comment.
When an earlier quality or audio replan has left replanError set, selecting a server-rendered subtitle sets requestedSubtitleTrackChangeRef in the preceding effect and schedules the new replan, but this effect still observes the previous render's non-null error and replanning === false. It therefore immediately shows that unrelated error as a subtitle refusal and clears the request marker before the subtitle replan finishes. Correlate the error with the subtitle request key or otherwise ignore errors that predate the current subtitle replan.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| const maxResolution = | ||
| typeof screen !== "undefined" | ||
| ? detectMaxResolutionFromScreen(screen.width, screen.height) | ||
| ? detectMaxResolutionFromScreen(screen.width * pixelRatio, screen.height * pixelRatio) |
There was a problem hiding this comment.
On a high-DPI display whose physical long edge exceeds 2160 but whose short edge does not—for example a 1440×900 CSS-pixel screen at DPR 2—this produces 2880×1800 and detectMaxResolutionFromScreen classifies it as 2160p because that helper uses the larger dimension. ResolveQualityPolicyV3 then interprets the label as a maximum video height, so automatic quality selection can leave a 2160p source untouched on an 1800-pixel-tall display; browser zoom can inflate devicePixelRatio similarly. Base the bucket on the physical short axis, or otherwise keep the reported label consistent with the video-height ceiling the planner consumes.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
…ality Narrow proxy selection by capability *before* selection rather than rejecting a single round-robin pick afterwards. Abandoning the pool on one mismatch meant a capable proxy with free capacity sat unused while playback either ran ffmpeg on the API node or, with playback.local_transcode_fallback disabled, was refused outright — the exact api/proxy split this branch targets, during exactly the rolling ffmpeg upgrade the capability check exists for. PlanSessionWith now applies its eligibility predicate to the proxy on proxy-only plans (the proxy is the executor there), mirroring how HLS filters transcode nodes, and the planner grows ProxyNodeURLs to match TranscodeNodeURLs. Direct play still skips the probe: it copies bytes and needs no recipe. Every committed route now records transport locality, not just the identity-proxy one. A session replanned from a proxy onto the integrated transcoder previously kept a stale remote-transport mark, and the widened idle grace it grants would hold that session's stream and transcode slots for five minutes after the local stream disconnected without an explicit stop. The remote HLS route sets it too — it also hands the client an absolute proxy URL that never reaches this server. The proxy's CORS config exposed no response headers, so cross-origin JavaScript could send the If-Range/Range request headers it already allows but never read the ETag, Accept-Ranges or Content-Range needed to build them. direct_stream_resume_v1 silently degraded to a full restart whenever the proxy was on a different origin than the web app, which is the normal deployment. Also regenerates internal/playback/testdata/protocol_v3 and the schema fixtures, which were stale for output_change_v1 since #613/#617 and failed CI on every branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…usals #617 renamed the terminal a burn-in-forced adaptation reports: when the subtitle burn requirement is the sole trigger, an HDR source that cannot be re-encoded now returns subtitle_conversion_unsupported instead of hdr_transcode_unsupported, so the refusal names the thing the viewer can actually act on. terminalAllowsAlternateFileV3 was not updated to match, and it gates the alternate-version retry on the old reason strings. That silently retired the fallback for exactly the case its own comment describes — a bitmap subtitle needing burn-in that an HDR source cannot support while an SDR alternate can. Playback was refused outright instead of switching to the version that can serve it. Adds the new reason to the gate and covers it directly, so a future rename of a refusal reason fails on the gate rather than only on the end-to-end replan test. Also drops debug instrumentation that was committed by mistake in TestHandleReplanPlaybackV3BitmapSubtitleFallsBackFromHDRToSDRVersion; the assertion is back to its original form and now passes on the merits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(playback): route v3 direct play and remux through proxy nodes
Protocol v3 consulted the node planner only for the HLS deliveries, so
`original_http` and `server_remux_progressive` sessions returned an
API-local `/stream/{session_id}` URL and the API node served the bytes —
ServeDirectPlay for direct play, a locally spawned ffmpeg for the remux.
An operator running dedicated proxy nodes still saw all of that egress on
the API node.
The capability already existed: the proxy implements /stream/direct and
/stream/remux, and the Jellyfin-compat transport already plans a proxy for
exactly these two methods. Native v3 was the only surface skipping it, so
Jellyfin clients routed correctly on a deployment where Silo's own clients
did not. This wires the same shape into the v3 identity transport rather
than inventing a second selection path.
The proxy serves from the stream token alone, so the token now carries the
media path, the file's Dolby Vision profile (a P7 remux must strip the
dangling RPU) and the audio-only flag (which picks audio/mp4 over
video/mp4, the MIME the plan promised). RecipeCard models none of the
three; a missing claim would not fail loudly, it would serve a subtly
different stream than the plan promised.
Two related fixes:
- Proxy direct play served via http.ServeFile, which sets no strong ETag.
direct_stream_resume_v1 depends on the ETag ServeDirectPlay sets before
ServeContent, so routing direct play to a proxy without this would have
silently broken resumable direct streams: If-Range never validates and a
resumed range restarts at 200. The proxy now uses the same serve path.
- playback.local_transcode_fallback was only checked in the HLS branch, so
a progressive remux that converts audio still spawned ffmpeg locally on
an API-only node with the setting disabled. Identity deliveries now
honor the gate too — direct play still falls back locally, since moving
bytes is not transcode work and single-node deployments must keep
working.
Falling back to the API-local path when no proxy is eligible preserves
single-node behavior, and a planner reservation is released whenever the
session does not actually reach a proxy.
Closes #619
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(playback): validate proxy recipes and keep proxy sessions alive
Addresses three P1 findings on the proxy-transport change.
Proxies do run ffmpeg — /stream/remux converts audio and strips Dolby
Vision RPUs — but they exposed no capability endpoint, so unlike the HLS
offload path nothing checked that the selected proxy could execute the
transformations a plan froze. A pool whose proxies carry a different
ffmpeg build (rolling upgrade, custom image) would fail at stream time: a
missing aac encoder 500s, a missing dovi_rpu filter is refused outright by
the remux itself. Proxies now serve /hw-capabilities in the same shape and
at the same path as a transcode node, and identity planning validates the
frozen recipe against the selected proxy, falling back to a node that can
do the work. A proxy that does not answer is treated as incapable rather
than assumed good: an older proxy predating the endpoint is exactly the
mismatched build the check exists to catch. Direct play copies bytes and
needs no recipe, so it skips the probe entirely.
meteredResponseWriter implemented neither Unwrap nor SetWriteDeadline, so
RollingDeadlineWriter could not install its stall deadline on any proxy
stream. With the standalone proxy running WriteTimeout 0 there was no
server-level guard behind it, so a client that stopped reading without
closing its connection would block a write forever, holding the session,
the file, the goroutine and the connection.
A proxy-served session never produces a transport request on the API node,
so activeTransportCount — what protects a local stream from the idle
reaper — stays zero and a heartbeat gap longer than the active grace would
reap a healthy stream, after which progress, stop and replan all fail with
session-not-found while bytes still flow. Sessions are now marked as
remotely transported, which widens their idle windows rather than granting
immunity: this manager has no absolute session lifetime, so unconditional
immunity would leak a session forever when a client disappears without
stopping. The mark is always set on commit, so a re-plan that moves a
session back onto the API clears a stale one.
Also adopts the exported transformation constants in the tests and covers
the effective-recipe bitrate branch, per review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(playback): pick capable sibling proxies and refresh transport locality
Narrow proxy selection by capability *before* selection rather than
rejecting a single round-robin pick afterwards. Abandoning the pool on one
mismatch meant a capable proxy with free capacity sat unused while
playback either ran ffmpeg on the API node or, with
playback.local_transcode_fallback disabled, was refused outright — the
exact api/proxy split this branch targets, during exactly the rolling
ffmpeg upgrade the capability check exists for. PlanSessionWith now
applies its eligibility predicate to the proxy on proxy-only plans (the
proxy is the executor there), mirroring how HLS filters transcode nodes,
and the planner grows ProxyNodeURLs to match TranscodeNodeURLs. Direct
play still skips the probe: it copies bytes and needs no recipe.
Every committed route now records transport locality, not just the
identity-proxy one. A session replanned from a proxy onto the integrated
transcoder previously kept a stale remote-transport mark, and the widened
idle grace it grants would hold that session's stream and transcode slots
for five minutes after the local stream disconnected without an explicit
stop. The remote HLS route sets it too — it also hands the client an
absolute proxy URL that never reaches this server.
The proxy's CORS config exposed no response headers, so cross-origin
JavaScript could send the If-Range/Range request headers it already allows
but never read the ETag, Accept-Ranges or Content-Range needed to build
them. direct_stream_resume_v1 silently degraded to a full restart whenever
the proxy was on a different origin than the web app, which is the normal
deployment.
Also regenerates internal/playback/testdata/protocol_v3 and the schema
fixtures, which were stale for output_change_v1 since #613/#617 and failed
CI on every branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(playback): restore the alternate-version fallback for burn-in refusals
#617 renamed the terminal a burn-in-forced adaptation reports: when the
subtitle burn requirement is the sole trigger, an HDR source that cannot
be re-encoded now returns subtitle_conversion_unsupported instead of
hdr_transcode_unsupported, so the refusal names the thing the viewer can
actually act on.
terminalAllowsAlternateFileV3 was not updated to match, and it gates the
alternate-version retry on the old reason strings. That silently retired
the fallback for exactly the case its own comment describes — a bitmap
subtitle needing burn-in that an HDR source cannot support while an SDR
alternate can. Playback was refused outright instead of switching to the
version that can serve it.
Adds the new reason to the gate and covers it directly, so a future
rename of a refusal reason fails on the gate rather than only on the
end-to-end replan test.
Also drops debug instrumentation that was committed by mistake in
TestHandleReplanPlaybackV3BitmapSubtitleFallsBackFromHDRToSDRVersion; the
assertion is back to its original form and now passes on the merits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Problem
Follow-up to #613 for #609. The reporter redeployed with #613 merged and still hit
hdr_transcode_unsupportedon the same Dolby Vision Profile 8.1 source, with the normalized request showing emptyhdr_detailsandmax_resolution: "1080p"— on a MacBook Pro XDR display running Safari 26.6.Three gaps remained after #613, confirmed against the reporter's browser evidence:
matchMedia("(dynamic-range: high)"). Safari 26 returnsfalsefor both dynamic-range queries even on an XDR panel, so the exactcanPlayType/MediaCapabilities.decodingInfoprobes — which return positive, exact decode evidence on that machine — never ran. The client sent emptyhdr_details, and the planner correctly refused the range.canPlayType('dvhe.08.06')returns""in Safari; onlydvh1.*(andhvc1.*for plain HEVC) get"probably", per Apple's HLS authoring recommendation. Chromium-family browsers answer fordvhe/hev1, so both spellings must be probed.-tag:v dvhe, and its comment claimed dvhe keeps FFmpeg'sdvvCbox while dvh1 loses it. That is wrong: FFmpeg refuses to write thedvvCDOVI configuration record under either tag without-strict unofficial("Not writing 'dvcC'/'dvvC' box. Requires -strict unofficial"). The reporter verified with the bundled FFmpeg 7.1.4 that-tag:v dvh1 -strict unofficialproducesdvh1+hvcC+dvvCwith the full DOVI record intact (dv_profile=8, bl_signal_compatibility_id=1, rpu_present=1).A fourth, smaller gap:
screen.width/heightare logical CSS pixels, so a 2× 2160p-class panel advertisedmax_resolution: "1080p".Approach
decodingInfoprobe run unconditionally; thedynamic-rangemedia query survives only as the best-efforthdroutput boolean. Browsers tone-map HDR onto SDR outputs, so discarding exact positive decode evidence because the coarse output query says "standard" was wrong on both counts.dvh1.*thendvhe.*and claims on either definitive"probably"; the HDR10 Media Capabilities shape is probed underhvc1.2.4.L153.B0thenhev1.2.4.L153.B0. Everything else about the claims is unchanged: Profile 8 stays bounded to the 8.1 base-layer shape and level 6, claims stay scoped toprogressivedelivery only, and"maybe"still claims nothing.-tag:v dvh1 -strict unofficialfor the explicit v3 preserve recipe. This is scoped to thetagDVSampleEntryopt-in branch only — legacy web/jellycompat consumers, the P7 HDR10 strip, and plain remuxes are untouched. Media3 recognizes bothdvheanddvh1sample entries, so Android preserve consumers are unaffected.max_resolutionbydevicePixelRatio(guarded, defaulting to 1).hdr_detailsis correct once the client stops suppressing the evidence.Validation
go test ./internal/playback/ ./internal/api/handlers/— okpnpm --dir web exec vitest run src/player/hooks/useCodecDetection.test.ts src/player/client-context-v3.test.ts src/player/hooks/usePlaybackSession.test.ts src/player/components/WatchPage.test.ts src/player/components/VideoPlayer.test.tsx— 5 files, 74 tests passedgolangci-lint run --new-from-merge-base=origin/main ./internal/playback/...— 0 issuespnpm --dir web run lint— 0 errors (151 inherited warnings, baseline)pnpm --dir web run format:check— cleanmake verify-local-paths— cleangofmt -l internal/playback/— cleanFull
make test-goalso surfaces three pre-existing failures unrelated to this change and present on cleanmainin the same environment: the two macOS-only jellycompat process-lock tests noted in #613, andinternal/playback/contractTestAdvertisedListsMatchTheGoldenFixtures(golden fixture missingoutput_change_v1, introduced by #613 itself — reproduced with this branch's changes stashed).New regression coverage: dvh1-only Safari-shaped answers earn the DV claim with SDR-reporting outputs, dvhe fallback still works,
"maybe"claims nothing, the HDR10 probe runs and is trusted without the HDR media query and accepts either HEVC sample entry, remux args carrydvh1+-strict unofficialonly in the preserve branch, and DPR scaling of the screen-derived resolution.Risk
The claim surface widens only where the browser gives a definitive positive answer for the exact sample entry; profile/level/BL-compat bounds and the progressive-only scoping are unchanged. The remux flag change alters bytes only for the v3 preserve recipe, which today produces streams with no DV signaling at all — consumers can only gain the configuration record they were promised.
-strict unofficialrelaxes only the MP4 muxer's willingness to write thedvvCbox.Not addressed here (candidate follow-up): the reporter's suggested integration test that inspects a generated MP4 for
dvh1/hvcC/dvvCboxes, which needs a real DV sample fixture; and the stale#613golden-fixture failure noted above.Part of #609.
AI Disclosure
dvhetag; confirmed the flathdrboolean has no remaining gating consumers in the player; confirmed-strict unofficialcannot leak into the legacy/strip/plain remux paths via the existing arg tests.🤖 Generated with Claude Code
Summary by CodeRabbit