Skip to content

fix(playback): accept constrained baseline on baseline decoders - #597

Merged
Quick104 merged 1 commit into
Silo-Server:mainfrom
Rhainland:suspense/fix-h264-constrained-baseline-direct-play
Aug 27, 2026
Merged

fix(playback): accept constrained baseline on baseline decoders#597
Quick104 merged 1 commit into
Silo-Server:mainfrom
Rhainland:suspense/fix-h264-constrained-baseline-direct-play

Conversation

@Rhainland

@Rhainland Rhainland commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • accept H.264 Constrained Baseline sources when an exact-evidence decoder reports Baseline support
  • keep compatibility directional and preserve strict matching for unrelated codecs and profiles
  • document the profile rule and publish a protected cross-client conformance scenario
  • cover server-level and per-user-transcoding-disabled playback paths

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_disabled because 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:

  • normalizes presentation-only H.264 separators (whitespace, -, _, ., and : — the colon covers ffprobe's High 4:2:2 / High 4:4:4 family spellings)
  • accepts Constrained Baseline source → Baseline decoder
  • deliberately rejects the reverse direction
  • preserves unknown punctuation and does not infer compatibility among Baseline, Main, High, High 10, or unknown profiles
  • retains the prior trimmed, case-insensitive equality behavior for non-H.264 codecs

No API shape, schema, migration, planner flow, handler flow, or protocol-version change is included.

Compatibility surfaces

  • jellycompat: considered and deliberately left unchanged. The Jellyfin VideoProfile device-profile condition keeps literal token matching, which mirrors upstream Jellyfin's own condition evaluation; mainstream Jellyfin clients (jellyfin-web and derivatives) list constrained baseline explicitly 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.
  • silo-apple / silo-android: no client change needed — clients already send video_decode capability profiles unchanged; this PR only widens what the server validates against them.

Verification

$ go test ./internal/playback -run 'Test.*(VideoProfile|ConstrainedBaseline)|TestProtocolV3ConformanceMatrixCoversReleaseTrain' -count=1
ok github.com/Silo-Server/silo-server/internal/playback

$ CGO_LDFLAGS='-L/opt/homebrew/opt/glib/lib' go test ./internal/api/handlers -run 'TestHandleStartPlaybackV3.*ConstrainedBaseline' -count=1
ok github.com/Silo-Server/silo-server/internal/api/handlers

$ make verify-playback-fixtures
playback fixtures are current

$ go test ./internal/playback -skip 'Test(ResolveHWAccelWithFFmpeg|FFmpegSupportsNVENC)'
ok github.com/Silo-Server/silo-server/internal/playback

$ CGO_LDFLAGS='-L/opt/homebrew/opt/glib/lib' golangci-lint run --new-from-rev=origin/main
0 issues.

$ make verify-local-paths
scripts/check-local-path-leaks.sh

The complete handler suite also passed. The repository-wide make test-go remains 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. Full make lint reports 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, and validated_original_playback.

AI Disclosure

  • Tool(s): Codex desktop
  • Model(s): gpt-5, gpt-5.6-sol
  • Involvement: fully AI-generated
  • Adversarial review: the first review found overbroad punctuation stripping and an unprotected generated conformance scenario. Normalization was restricted to the intended separators, malformed punctuation regressions were added, and the release-train test now requires and validates the scenario. A second full-diff review reported no findings.

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):

  • Resolved the protocol_v3_test.go scenario→delivery map conflict as the union of both sides; an adversarial diff review against main confirmed no scenario or assertion from either side was dropped, and the single profile-check line in the merged videoEligibleV3 means the fix now covers software-decode capability entries as well as hardware ones.
  • Regenerated and verified the conformance matrix against the merged fixture generator (make verify-playback-fixtures passes).
  • Added : to the stripped H.264 presentation separators so ffprobe's High 4:2:2 / High 4:4:4 Predictive spellings 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.
  • Re-ran the playback suite (minus host GPU probes), the TestHandleStartPlaybackV3 handler tests, gofmt, make verify-local-paths, and changed-lines golangci-lint (only finding: the pre-existing web/dist embed typecheck on unbuilt worktrees) — all clean.

Summary by CodeRabbit

  • Bug Fixes

    • Improved H.264 profile compatibility matching, including support for Constrained Baseline media on Baseline-capable devices.
    • Enabled direct playback without video transcoding when compatible decoder support is advertised.
    • Preserved original HTTP delivery and media quality for eligible H.264 content.
  • Tests

    • Added coverage for profile normalization, compatibility decisions, and direct-playback scenarios.
    • Added conformance validation for constrained-baseline H.264 playback across supported device capabilities.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 933d1442-74a2-47ae-8259-6d73a9dd4256

📥 Commits

Reviewing files that changed from the base of the PR and between b29aaf9 and 1dfab25.

📒 Files selected for processing (6)
  • cmd/playbackfixtures/main.go
  • docs/architecture/playback-protocol-v3.md
  • internal/api/handlers/playback_v3_test.go
  • internal/playback/capabilities_v3.go
  • internal/playback/protocol_v3_test.go
  • internal/playback/testdata/protocol_v3/conformance_matrix.json
🚧 Files skipped from review as they are similar to previous changes (6)
  • internal/api/handlers/playback_v3_test.go
  • cmd/playbackfixtures/main.go
  • internal/playback/capabilities_v3.go
  • docs/architecture/playback-protocol-v3.md
  • internal/playback/testdata/protocol_v3/conformance_matrix.json
  • internal/playback/protocol_v3_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

Playback 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.

Changes

H.264 profile compatibility

Layer / File(s) Summary
Profile matching rules
internal/playback/capabilities_v3.go, docs/architecture/playback-protocol-v3.md, internal/playback/protocol_v3_test.go
Profile checks normalize case and presentation separators. H.264 Constrained Baseline matches Baseline decoder support. Other codecs retain identity matching. Tests cover supported and rejected profile variants.
Direct playback conformance
internal/playback/testdata/protocol_v3/conformance_matrix.json, cmd/playbackfixtures/main.go, internal/playback/protocol_v3_test.go, internal/api/handlers/playback_v3_test.go
Fixtures and tests validate original HTTP delivery, validated-original decisions, and direct sessions without transcoding for Constrained Baseline H.264.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 1dfab

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: accepting Constrained Baseline H.264 sources on Baseline decoders.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@Rhainland
Rhainland marked this pull request as ready for review August 11, 2026 17:29
@coderabbitai coderabbitai Bot added the v1 Silo v1 scope - auto-adds to the Silo v1 project label Aug 11, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Quick104
Quick104 force-pushed the suspense/fix-h264-constrained-baseline-direct-play branch from 3c58908 to 1dfab25 Compare August 27, 2026 23:45
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

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.

@coderabbitai coderabbitai Bot removed the v1 Silo v1 scope - auto-adds to the Silo v1 project label Aug 27, 2026
@Quick104

Copy link
Copy Markdown
Contributor

Maintainer update: rebased this onto current main and pushed a refreshed head (1dfab25dd), since the branch predated the software-decode negotiation, tone-mapping, and quality-ladder work and had gone conflicting. Summary of what changed:

Rebase

  • Resolved the one conflict — the scenario→delivery expectation map in protocol_v3_test.go — as the union of both sides: main's client_managed_hdr_selected_audio entry plus this PR's h264_constrained_baseline_direct. A diff review against main confirmed no scenario or assertion from either side was dropped.
  • Because the merged videoEligibleV3 still has a single profile-check line, the Constrained Baseline rule now also applies to software_video_decode_v1 capability entries, not just hardware ones — no extra code needed.
  • Regenerated the conformance matrix against the merged fixture generator; make verify-playback-fixtures passes.

One functional addition

  • canonicalH264ProfileV3 now strips : along with the other presentation separators. ffprobe spells the 4:2:2/4:4:4 profiles with colons (High 4:2:2, High 4:4:4 Predictive), so before this a compact decoder spelling like high422 could never match, while high-4.2.2 would — inconsistent with the documented "ignores presentation separators" rule. Two new table cases in TestVideoProfileSupportedV3 cover the colon variants, and all fifteen ffmpeg H.264 profile names remain distinct after canonicalization (High 4:2:2 vs High 4:2:2 Intra still differ).

Ruled out (recorded in the description)

  • jellycompat parity: left unchanged deliberately. Literal VideoProfile condition matching mirrors upstream Jellyfin, and mainstream clients list constrained baseline explicitly in their H.264 conditions. Revisit only if a real client reports the gap.
  • Client follow-ups: none needed — clients send video_decode profiles unchanged; this only widens what the server validates against them.

Verification on the rebased head: full ./internal/playback suite (minus host GPU probes), TestHandleStartPlaybackV3 handler tests, gofmt, make verify-local-paths, and changed-lines golangci-lint all clean (the only lint hit is the pre-existing web/dist embed typecheck on unbuilt worktrees).

Rebase and follow-up produced with Claude Code (Claude Fable 5); review findings were independently verified before applying.

@Quick104
Quick104 merged commit b4e4ed5 into Silo-Server:main Aug 27, 2026
4 checks passed
@github-project-automation github-project-automation Bot moved this to Done in Silo v1 Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants