fix(playback): transcode H.264 sources with conflicting in-band PPS#424
Open
CoffeeKnyte wants to merge 1 commit into
Open
fix(playback): transcode H.264 sources with conflicting in-band PPS#424CoffeeKnyte wants to merge 1 commit into
CoffeeKnyte wants to merge 1 commit into
Conversation
H.264 streams that redefine the same pic_parameter_set_id in-band with different content cannot be safely stream-copied into an avc1/fMP4 HLS segment: the avcC advertises a single parameter set, so VideoToolbox (Safari/Chrome on macOS) decodes with the wrong PPS and desyncs mid-GOP, surfacing as PIPELINE_ERROR_DECODE / kVTVideoDecoderBadDataErr (-12909). At playback start, a bitstream scan (DetectMultiplePPSH264) runs for H.264 files on the probe-ensure path, grouping in-band PPS by id and flagging any id carrying more than one distinct definition. The result is a runtime-only flag (VideoTrack.MultiplePPS, json:"-") memoized per process — never written to the database, no schema change, recomputed on the first play after a restart. The v3 planner and legacy resolver disqualify a copy-unsafe source from the video stream-copy / remux ladder, routing it to a real transcode. Direct play of the original container is left intact: decoders that reparse in-band parameter sets (ExoPlayer, VLC, native) handle the source fine. Part of #135.
📝 WalkthroughWalkthroughThe change adds H.264 PPS conflict scanning, stores the result as runtime media metadata, wires FFmpeg into playback probing, and prevents unsafe video stream-copy routes in the resolver and Protocol v3 planner. ChangesH.264 copy-safety playback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlaybackProbeEnsurer
participant DetectMultiplePPSH264
participant MediaFile
participant SourceDescriptorV3
participant PlanPlaybackV3
PlaybackProbeEnsurer->>DetectMultiplePPSH264: scan eligible H.264 file
DetectMultiplePPSH264-->>PlaybackProbeEnsurer: conflicting PPS result
PlaybackProbeEnsurer->>MediaFile: set runtime MultiplePPS metadata
MediaFile->>SourceDescriptorV3: derive VideoCopyUnsafe
SourceDescriptorV3->>PlanPlaybackV3: provide copy-safety flag
PlanPlaybackV3->>PlanPlaybackV3: select remux or transcode
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/scanner/pps.go`:
- Line 71: Update the PPS comparison logic around byID in the scanner to key
entries by PPS payload rather than the complete NAL unit, excluding the first
header byte containing nal_ref_idc. Ensure identical payloads with different
valid header priority values are treated as non-conflicting and preserve
conflict detection for differing payloads.
In `@internal/scanner/probe_repair.go`:
- Around line 149-152: Update the probe-repair error path around the visible
return so scan failures are represented as copy-unsafe for the current
resolution while remaining uncached for retry. Introduce or reuse a distinct
runtime state for “scan unknown/failed” rather than setting MultiplePPS, and
ensure the resolver and both callers preserve this conservative result instead
of falling back to video stream-copy when the ensured file is discarded.
🪄 Autofix (Beta)
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: a919df4f-9613-4cad-a286-673c3150d48f
📒 Files selected for processing (11)
cmd/silo/main.gointernal/models/media.gointernal/playback/capabilities_v3.gointernal/playback/plan_v3.gointernal/playback/protocol_v3.gointernal/playback/protocol_v3_test.gointernal/playback/resolver.gointernal/playback/resolver_test.gointernal/scanner/pps.gointernal/scanner/pps_test.gointernal/scanner/probe_repair.go
| if byID[id] == nil { | ||
| byID[id] = make(map[string]struct{}) | ||
| } | ||
| byID[id][string(nal)] = struct{}{} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare the PPS payload, not the complete NAL unit.
The first byte contains nal_ref_idc; two NALs with identical PPS content but different valid header priority values are currently treated as conflicting, unnecessarily forcing transcoding.
Proposed fix and regression case
- byID[id][string(nal)] = struct{}{}
+ byID[id][string(nal[1:])] = struct{}{}{
name: "identical payload with different nal_ref_idc is safe",
data: annexB(
append([]byte{0x68}, 0xee, 0x35),
append([]byte{0x48}, 0xee, 0x35),
),
want: false,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| byID[id][string(nal)] = struct{}{} | |
| byID[id][string(nal[1:])] = struct{}{} |
🤖 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/scanner/pps.go` at line 71, Update the PPS comparison logic around
byID in the scanner to key entries by PPS payload rather than the complete NAL
unit, excluding the first header byte containing nal_ref_idc. Ensure identical
payloads with different valid header priority values are treated as
non-conflicting and preserve conflict detection for differing payloads.
Comment on lines
+149
to
152
| if err != nil { | ||
| // Leave the flag unset so a transient failure retries on the next play | ||
| // rather than caching a wrong answer; the copy path stays available. | ||
| return file, err |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not fail open to video stream-copy when the safety scan fails.
Both supplied callers discard the ensured file on error, while the resolver treats an unset flag as safe. Consequently, cancellation, malformed input, or an FFmpeg failure sends an affected source through the exact remux path this change is intended to prevent.
Represent “scan failed/unknown” as conservatively copy-unsafe while leaving it uncached for retry; this likely needs a distinct runtime flag rather than setting MultiplePPS=true.
🤖 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/scanner/probe_repair.go` around lines 149 - 152, Update the
probe-repair error path around the visible return so scan failures are
represented as copy-unsafe for the current resolution while remaining uncached
for retry. Introduce or reuse a distinct runtime state for “scan unknown/failed”
rather than setting MultiplePPS, and ensure the resolver and both callers
preserve this conservative result instead of falling back to video stream-copy
when the ensured file is discarded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Some H.264 movies fail to play in the web player with a hard decode error.
A subset of 1080p H.264 library titles, when played through the browser player on
macOS (both Safari and Chrome, which decode video in hardware via VideoToolbox),
throw a decode error a second or two into playback and never recover:
From the operator's side the session starts normally, the player re-requests the
first couple of segments (the browser retrying after a fatal media error), and
then the session is torn down. The exact same titles play correctly everywhere
else — the mobile and TV native apps, VLC, and Firefox — so nothing looks wrong
with the file itself, and it plays end-to-end when fully decoded offline. The
breakage is specific to the browser + hardware-decode path.
Solution
Root cause. The affected titles are H.264 streams that carry their encoding
parameter sets in-band, redefining the same parameter-set id repeatedly
with different contents across the opening frames (the per-frame reference
count changes). When the server stream-copies such a stream into an
avc1/fMP4HLS segment, the segment's single declared parameter set (
avcC) disagrees withthe per-frame ones. A strict decoder that honors
avc1— VideoToolbox — configuresonce from the declared set and desyncs when the stream switches mid-GOP, which is
exactly the
-12909decode failure. Decoders that re-parse in-band parameter setsper frame (ExoPlayer, VLC, Firefox's software path) are unaffected, which is why
only the browser hardware-decode surface breaks.
Making the container honest by tagging the copy
avc3(which advertises in-bandparameter sets) was evaluated and does not fix it: VideoToolbox only
reconfigures its decode session at IDR boundaries, and these streams switch
mid-GOP, so the honest tag changes nothing the decoder acts on. The reliable fix
is to stop stream-copying these sources and give the client a freshly encoded,
single-parameter-set stream instead.
Detection.
DetectMultiplePPSH264(internal/scanner/pps.go) runs atplayback start for H.264 sources: it stream-copies the opening seconds through
ffmpeg's
filter_unitsbitstream filter, groups the in-band parameter sets byid, and flags any id that carries more than one distinct definition. A source
that legitimately uses several distinct ids (each defined once) is not
flagged — only conflicting redefinitions of a single id are. The result is a
runtime-only flag (
models.VideoTrack.MultiplePPS, taggedjson:"-") memoizedper process in
PlaybackProbeEnsurer(internal/scanner/probe_repair.go). It isnever written to the database — no schema change, no migration — and is
recomputed on the first play after a restart (~0.2s, a stream-copy demux, run
once per file per process).
Routing. The v3 planner (
internal/playback/plan_v3.go) and the legacyresolver (
internal/playback/resolver.go) disqualify a copy-unsafe source fromthe video stream-copy / remux ladder, so it falls through to a real transcode
(
planVideoTranscodeV3/PlayTranscode). Direct play of the original containeris deliberately left intact: clients that reparse in-band parameter sets handle
the untouched source fine and should not be forced to transcode. The flag reaches
the planner through the existing source descriptor (
SourceDescriptorV3.VideoCopyUnsafein
internal/playback/capabilities_v3.go), so the planner stays pure.Risk / follow-ups
That is more server load for that class of file, but it is the correct trade
for reliability — the copy is simply not decodable on the browser path.
scope here.
restart rather than persisted. If the per-play warm-up cost ever matters, the
same detector result could be persisted on the file's probe metadata later.
(the web player uses HLS, so this path is not exercised in practice); it remains
a theoretical residual.
Verification
go build ./...,go vet ./internal/playback/... ./internal/scanner/..., andgo test ./internal/playback/... ./internal/scanner/...all pass.several distinct ids are treated as safe, while the same id redefined with
conflicting content is flagged (
internal/scanner/pps_test.go), plus theExp-Golomb id parser.
while an otherwise-identical copy-safe source still remuxes
(
internal/playback/protocol_v3_test.go,internal/playback/resolver_test.go).-12909decode error on an affected H.264 remux throughmacOS Chrome, confirmed the detector flags the source, and verified the resulting
session runs a real encode instead of a video stream-copy.
AI-use disclosure
Implemented with Claude Code.
Summary by CodeRabbit