fix(playback): accept constrained baseline on baseline decoders - #597
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughPlayback Protocol V3 now accepts H.264 Constrained Baseline media for Baseline decoder capabilities. The change adds profile normalization, planner fixtures, conformance coverage, and API handler regression coverage for direct playback without transcoding. ChangesH.264 profile compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change narrowly expands validated H.264 Constrained Baseline direct-playback compatibility without changing APIs, schemas, migrations, or planner flow. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3c58908 to
1dfab25
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Maintainer update: rebased this onto current Rebase
One functional addition
Ruled out (recorded in the description)
Verification on the rebased head: full Rebase and follow-up produced with Claude Code (Claude Fable 5); review findings were independently verified before applying. |
Summary
Problem
A confirmed production playback attempt reported exact H.264 hardware support for Baseline, Main, High, and High 10, while ffprobe identified the source as Constrained Baseline. The protocol-v3 planner compared profile strings literally, rejected direct playback, selected a full HLS video transcode, and then returned
adaptation_unavailable/transcoding_disabledbecause video transcoding was disabled for the user.Constrained Baseline is a restricted subset of Baseline, so this source should use validated original playback without consuming video-transcode entitlement.
Scope source: confirmed production incident; no matching public GitHub issue was found.
Related issue: N/A — narrow fix
Approach
The exact-evidence profile predicate now:
-,_,., and:— the colon covers ffprobe'sHigh 4:2:2/High 4:4:4family spellings)No API shape, schema, migration, planner flow, handler flow, or protocol-version change is included.
Compatibility surfaces
VideoProfiledevice-profile condition keeps literal token matching, which mirrors upstream Jellyfin's own condition evaluation; mainstream Jellyfin clients (jellyfin-web and derivatives) listconstrained baselineexplicitly in their H.264 conditions, so they are unaffected. Teaching jellycompat a Baseline ⊇ Constrained Baseline rule would diverge from Jellyfin-faithful behavior and can be revisited if a real client reports the gap.video_decodecapability profiles unchanged; this PR only widens what the server validates against them.Verification
The complete handler suite also passed. The repository-wide
make test-goremains non-green on this macOS host because four existing GPU-probe tests select/kill host NVENC/QSV probes and two existing Jellyfin web-operation process-lock tests fail. Fullmake lintreports the repository's documented whole-tree backlog; changed-lines lint is clean.Risk and rollout
The compatibility expansion is limited to exact-evidence H.264 profile matching and protected by directional, malformed-punctuation, unsupported-profile, non-H.264, High 10, planner, handler, and generated conformance tests. Rollout requires no migration or feature flag. Verify affected playback decisions log
original_http,direct, andvalidated_original_playback.AI Disclosure
Maintainer update (2026-08-27)
Rebased onto current
main(post software-decode negotiation, tone mapping, and quality-ladder work) and refreshed by a maintainer-side review pass (Claude Code / Claude Fable 5):protocol_v3_test.goscenario→delivery map conflict as the union of both sides; an adversarial diff review againstmainconfirmed no scenario or assertion from either side was dropped, and the single profile-check line in the mergedvideoEligibleV3means the fix now covers software-decode capability entries as well as hardware ones.make verify-playback-fixturespasses).:to the stripped H.264 presentation separators so ffprobe'sHigh 4:2:2/High 4:4:4 Predictivespellings match compact decoder spellings (e.g.high422), matching the documented "ignores presentation separators" rule; verified canonicalization keeps all fifteen ffmpeg H.264 profile names distinct. Two new test cases cover the colon variants.TestHandleStartPlaybackV3handler tests,gofmt,make verify-local-paths, and changed-linesgolangci-lint(only finding: the pre-existingweb/distembed typecheck on unbuilt worktrees) — all clean.Summary by CodeRabbit
Bug Fixes
Tests