Repository navigation
Conversation
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (23)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe server DV7-to-HDR10 remux recipe now removes Dolby Vision RPUs and Profile 7 type-63 enhancement-layer NAL units. Capability advertisement requires FFmpeg support for both filters. The recipe version advances to 2, and planning, execution filtering, fixtures, and documentation reflect the updated contract. ChangesDV7-to-HDR10 remux
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant FFmpeg
participant TransformationRegistry
participant PlanV3
participant Executor
FFmpeg-->>TransformationRegistry: Reports dovi_rpu and filter_units
TransformationRegistry-->>PlanV3: Advertises server_dv7_to_hdr10 recipe 2
PlanV3->>Executor: Selects server transformation recipe 2
Executor->>FFmpeg: Applies RPU stripping and type-63 removal
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue was identified in the corrected DV7-to-HDR10 path. Merge after normal checks, with the documented mixed-version limitations understood. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation [ Resolution Add mixed-version fencing for both execution paths. Prevent an old API binary from executing a recipe-2 plan, and prevent an old Jellycompat route from serving recipe-1 output for a newly negotiated session. Add automated coverage for these upgrade cases. Full details: Docstring CoverageExplanation Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 18 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Quick104
added
priority: P1
Cursor
|
Evidence is incomplete for these visible playback changes:
The synthetic NAL-unit counts and capability Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness. Posted via Macroscope — Visible change evidence |
2 participants
Problem
Closes #656
Related issue: N/A
Validation tasks: none
When Silo strips Dolby Vision profile 7 down to HDR10 for a client that only takes HDR10, the "HDR10" stream still carries the profile 7 enhancement layer. A single-track profile 7 source interleaves its enhancement layer in the same video stream as NAL unit type 63. Mapping only
0:V:0can't separate those units, anddovi_rpu=strip=1doesn't remove them (FFmpeg 7.1 removes none; since FFmpeg 8.0 it removes at most the last unit of each picture). So theserver_dv7_to_hdr10output contradicts its ownenhancement_layer_discardedclaim and hands HDR10-only clients bytes they never said they could decode. Since #1535 the same strip also serves Jellyfin-compatible clients.Approach
DV7ToHDR10BitstreamFilteris nowdovi_rpu=strip=1,filter_units=remove_types=63. Every executor uses that one constant: native HLS remux, progressive remux, jellycompat and the RPU probe.dovi_rpuandfilter_unitsinffmpeg -bsfs.server_dv7_to_hdr10moves to recipe version 2. One new constant feeds the registry, the planner, jellycompat's pinned version and the fixture generator. A transcode node that still advertises recipe 1 can't claim the corrected output: it's no longer chosen for new plans or for jellycompat strips.videoBitstreamFilterForPlanV3rather than no filter at all. An old node rejects the unknown chain, and a current executor refuses the recipe 1 plan when it validates versions, so the plan fails and replans instead of copying Dolby Vision unstripped.make playback-fixtures.docs/architecture/playback-protocol-v3.mdanddocs/jellycompat-api.mdnow describe recipe 2.The fork I run used
filter_units=remove_types=63as part of a larger set of Dolby Vision changes. This is the same filter change on its own, written fresh againstmain. The earlier #657 was closed during the remux redesign.No client changes. Clients only validate the recipe versions of client-run transformations, and their vendored copies of these fixtures can pick up recipe 2 on their next sync. This is the same filter jellycompat already uses, so jellycompat gets the fix too.
Validation
Tests cover:
videoBitstreamFilterForPlanV3returning the current chain for a recipe 1 plan.Existing tests that used the old literal or a recipe 1 registry for the local server now use the constants.
Run on
0da5a6404(macOS, FFmpeg 9.0.2):go test ./internal/api/handlers/ ./internal/jellycompat/... ./cmd/playbackfixtures/ -count=1: pass.go test ./internal/playback/... -count=1: everything passes except the hardware-encoder probe tests (VideoToolbox, NVENC, QSV, VAAPI) that fail the same way onmainon this Mac.make verify-playback-fixtures: current.make lint-changed: 0 issues.make verify-local-paths: not run locally (the script crashes under macOS bash); CI runs it.0da5a6404: all required checks pass (run): Go test, Go integration, Go lint, Go DB pins, Go DB external auth, Web build, checks and tests, repository checks (includingverify-local-paths).Benchmarks. Not applicable.
filter_unitsdrops units in the same copy pass, with no decode or encode.Evidence
Evidence: https://evidence.siloserver.org/r/silo-server/pr-2090/
I have no Dolby Vision profile 7 media I can share, so there's no playback capture. The change is to the bytes the remux emits, not to anything a screen shows. The byte-level A/B on a real file is in #656, and the synthetic check below reproduces it.
#656 has the A/B on a real dual-layer profile 7 file (Jellyfin FFmpeg 7.1.4). The old chain left 51 type-63 units in two seconds of output; the new chain left 0. The output stayed HEVC Main 10, PQ/BT.2020, with no Dolby Vision configuration record.
To check this without private media, I injected type-63 units into 24 pictures of a synthetic HEVC Main 10 stream and counted the units each chain left, using FFmpeg 9.0.2:
The recipe 2 output decodes cleanly (hevc, Main 10, 24 frames).
GET /api/v1/playback/capability(protocol fixturecapability_response.json),transformationsentry forserver_dv7_to_hdr10:Risks
Sessions in flight during the upgrade. A DV7 strip session started before the upgrade fails when it restarts its copy on an upgraded executor (seek or node move), because the stored recipe still names the old chain. It then replans onto recipe 2.
Mixed-version clusters. Two cases still serve the wrong bytes while API replicas and nodes run different versions:
remux-dv-v1path segment isn't bumped, so an old replica can still serve recipe 1 bytes for a session a new replica negotiated.Fencing these needs either a
CopyFMP4RecipeVersionbump, which drops every in-flight copy session at deploy, or a new jellycompat path segment. I left both out to keep this change focused and can add either if you want it here. Single-node deployments aren't affected.Checklist
AI Disclosure