fix(playback): preserve and expose video color range - #447
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 (1)
📝 WalkthroughWalkthroughVideo color range metadata now flows from ffprobe through persisted video tracks, Jellyfin-compatible playback streams, protocol-v3 source descriptors, catalog records, and web playback information. Missing values become ChangesVideo color range metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FFprobe
participant Scanner
participant MediaFile
participant Playback
participant WebPlayer
FFprobe->>Scanner: Parse color_range
Scanner->>MediaFile: Persist VideoTrack.ColorRange
MediaFile->>Playback: Build source and stream metadata
Playback->>WebPlayer: Return color_range
WebPlayer->>WebPlayer: Format color range label
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/catalogseed/service_test.go (1)
10-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
unknownsentinel in the catalog conversion test.This test covers
tvandpc, but not the persistedunknownvalue. Add a third input/assertion so catalog export mapping is protected from accidentally dropping or normalizing the sentinel.Suggested test extension
- {ColorRange: "pc"}, + {ColorRange: "pc"}, + {ColorRange: "unknown"}, }) - if len(got) != 2 { - t.Fatalf("records length = %d, want 2", len(got)) + if len(got) != 3 { + t.Fatalf("records length = %d, want 3", len(got)) } - if got[0].ColorRange != "tv" || got[1].ColorRange != "pc" { - t.Fatalf("ColorRange values = [%q, %q], want [tv, pc]", got[0].ColorRange, got[1].ColorRange) + if got[0].ColorRange != "tv" || got[1].ColorRange != "pc" || got[2].ColorRange != "unknown" { + t.Fatalf("unexpected ColorRange values") }🤖 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/catalogseed/service_test.go` around lines 10 - 22, Extend TestToVideoTrackRecordsPreservesColorRange with a third VideoTrack whose ColorRange is "unknown", update the expected record count, and assert that the corresponding converted record preserves "unknown" unchanged alongside the existing "tv" and "pc" values.
🤖 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/catalogseed/service_test.go`:
- Around line 10-22: Extend TestToVideoTrackRecordsPreservesColorRange with a
third VideoTrack whose ColorRange is "unknown", update the expected record
count, and assert that the corresponding converted record preserves "unknown"
unchanged alongside the existing "tv" and "pc" values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c45d3dd9-5dd7-417d-932d-277e153a9b09
📒 Files selected for processing (15)
internal/catalogseed/service.gointernal/catalogseed/service_test.gointernal/catalogseed/types.gointernal/jellycompat/deviceprofile_conditions_test.gointernal/jellycompat/dto.gointernal/jellycompat/handlers_playback.gointernal/models/media.gointernal/scanner/probe.gointernal/scanner/probe_repair.gointernal/scanner/probe_repair_audio_test.gointernal/scanner/probe_video_range_test.gointernal/scanner/scanner.gointernal/scanner/types.goweb/src/api/types.tsweb/src/player/types.ts
![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: 827843fd21
ℹ️ 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 videoTracksMissingColorRange(file.VideoTracks) { | ||
| return true |
There was a problem hiding this comment.
When relying on a normal library rescan to backfill existing rows, this new full MediaFile check is bypassed: processFile builds updateReasons via scanStateUpdateReasons, which calls needsCriticalProbeRepairScanState on the lightweight scan state, and that state only exposes HasVideoTracks rather than whether each video_tracks element has color_range. For already-scanned, unchanged matched videos whose JSON lacks color_range, the scan skip path returns actionUnchanged, so the Jellyfin ColorRange field remains absent until an on-demand detail/playback repair happens; add a scan-state boolean for missing color range and include it in needsCriticalProbeRepairScanState so scheduled rescans converge too.
Useful? React with 👍 / 👎.
Quick104
changed the title
1 participant
Summary
color_rangevalues on scanned video tracksMediaStream.ColorRangefrom item detail and PlaybackInfo responsessource.color_rangein playback v3 for native clientsRoot cause
Silo decoded adjacent ffprobe color fields but omitted
color_rangefrom its probe shape, stored model, and Jellycompat MediaStream DTO. Direct-play bytes remained untouched, but clients could not receive the limited-rangetvor full-rangepcmetadata that FFmpeg and Jellyfin expose.FFmpeg semantics
FFmpeg names limited
AVCOL_RANGE_MPEGastv, fullAVCOL_RANGE_JPEGaspc, and unspecifiedAVCOL_RANGE_UNSPECIFIEDasunknown. ffprobe default JSON may omit an unspecified optional field, so Silo storesunknowninternally to make one-time legacy repair converge. Jellyfin-compatible output omits that sentinel; playback v3 preserves the canonical source value so native clients can explicitly ignoreunknown.No client should use this metadata to override valid decoder or container signaling. The coordinated native changes use it only as a fallback when local range metadata is unspecified.
Impact
Newly probed files preserve their exact range. Existing video rows are repaired on first detail or playback access, so users do not need to touch or re-import files. This is additive within
/api/v1and requires no SQL migration because video tracks are stored as JSONB.Part of #446.
Closes #446.
Coordinated clients
The browser decoder remains authoritative in the web app because browser media APIs do not expose a safe limited/full-range override; the web change makes the server-probed source value visible for diagnostics.
Validation
GOWORK=off go test ./internal/models ./internal/playback ./internal/scanner -count=1cd web && pnpm run lint(zero errors; existing warnings only)cd web && pnpm run format:checkcd web && pnpm run buildmake verify-local-pathspc; unspecified is omitted by default and reportsunknownwith-show_optional_fields alwaysThe broad
internal/jellycompatsuite still has two unrelated macOS process-lock identity failures involving an empty current-process token. Both failures reproduce unchanged onorigin/main; the focused changed path passes.Risk and follow-up
The main server-side behavioral change is a one-time lazy ffprobe for legacy video rows whose tracks lack
color_range. When ffprobe cannot determine the value, Silo storesunknowninternally so subsequent requests do not repeatedly probe.The original reporter should verify the same direct-play file after deploying this branch and confirm the Infuse and native client output.
AI use
Implemented with OpenAI Codex. The patch was reviewed with the repository autoreview workflow, focused validation was run across server and web, and review findings in the coordinated native clients were addressed before publication.
Summary by CodeRabbit
New Features
tv/pc/unknown) across scanning, export/import, API types, and playback planning.Bug Fixes
Tests