diff --git a/internal/models/media.go b/internal/models/media.go index d71a2c81..c9d9f0a5 100644 --- a/internal/models/media.go +++ b/internal/models/media.go @@ -236,6 +236,10 @@ type VideoTrack struct { // bitstream scan and held in memory, never serialized to the database // (`json:"-"`). nil means "not analyzed in this process yet". MultiplePPS *bool `json:"-"` + // VideoCopyUnsafe is set when conflicting PPS data is detected or when the + // safety scan cannot establish that video stream-copy is safe. It is + // runtime-only so transient scan failures are retried on a later request. + VideoCopyUnsafe bool `json:"-"` } // AudioTrack represents a probed audio stream stored as JSONB. diff --git a/internal/playback/resolver.go b/internal/playback/resolver.go index 92f6a970..46abf74c 100644 --- a/internal/playback/resolver.go +++ b/internal/playback/resolver.go @@ -270,10 +270,12 @@ func containsStr(slice []string, s string) bool { // videoCopyUnsafeFile reports whether the file's video stream cannot be safely // stream-copied into an avc1/fMP4 segment. It is set once by the multi-PPS // bitstream scan (H.264 sources that redefine a pic_parameter_set_id in-band -// with conflicting content). nil/false means copy is safe or not yet analyzed. +// with conflicting content). Scan failures also disable copy for the current +// decision while remaining eligible for retry on a later request. func videoCopyUnsafeFile(file *models.MediaFile) bool { if file == nil || len(file.VideoTracks) == 0 { return false } - return file.VideoTracks[0].MultiplePPS != nil && *file.VideoTracks[0].MultiplePPS + track := file.VideoTracks[0] + return track.VideoCopyUnsafe || (track.MultiplePPS != nil && *track.MultiplePPS) } diff --git a/internal/playback/resolver_test.go b/internal/playback/resolver_test.go index 7353432a..9679a13a 100644 --- a/internal/playback/resolver_test.go +++ b/internal/playback/resolver_test.go @@ -82,6 +82,23 @@ func TestResolver_CopyUnsafeForcesTranscode(t *testing.T) { } } +func TestResolver_UnknownCopySafetyForcesTranscode(t *testing.T) { + // An inconclusive safety scan must not fail open to video stream-copy. + file := &models.MediaFile{ + CodecVideo: "h264", CodecAudio: "dts", Container: "mkv", + Resolution: "1080p", HDR: false, + VideoTracks: []models.VideoTrack{{ + Codec: "h264", + VideoCopyUnsafe: true, + }}, + } + decision := playback.Resolve(file, defaultCaps(), defaultSettings()) + + if decision.Method != playback.PlayTranscode { + t.Errorf("method = %q, want transcode", decision.Method) + } +} + func TestResolver_CopySafeStillRemuxes(t *testing.T) { safe := false // The same shape with the copy-safety scan resolved to safe keeps remuxing. diff --git a/internal/scanner/pps.go b/internal/scanner/pps.go index 66dd023f..3725af4d 100644 --- a/internal/scanner/pps.go +++ b/internal/scanner/pps.go @@ -68,7 +68,9 @@ func annexBHasConflictingPPS(data []byte) bool { if byID[id] == nil { byID[id] = make(map[string]struct{}) } - byID[id][string(nal)] = struct{}{} + // nal_ref_idc belongs to the NAL header, not the PPS definition. Two + // otherwise-identical PPS payloads may carry different valid priorities. + byID[id][string(nal[1:])] = struct{}{} if len(byID[id]) > 1 { return true } diff --git a/internal/scanner/pps_test.go b/internal/scanner/pps_test.go index 60fcc7d5..1408c269 100644 --- a/internal/scanner/pps_test.go +++ b/internal/scanner/pps_test.go @@ -29,6 +29,15 @@ func TestAnnexBHasConflictingPPS(t *testing.T) { data: annexB(pps(0xee, 0x35, 0x25), pps(0xee, 0x35, 0x25), pps(0xee, 0x35, 0x25)), want: false, }, + { + name: "same pps payload with different nal priority is safe", + // nal_ref_idc is header metadata, not part of the PPS definition. + data: annexB( + []byte{0x28, 0xee, 0x35, 0x25}, + []byte{0x68, 0xee, 0x35, 0x25}, + ), + want: false, + }, { name: "same id redefined with different content is unsafe", // both decode to pic_parameter_set_id=0 (leading bit set) but differ. diff --git a/internal/scanner/probe_repair.go b/internal/scanner/probe_repair.go index f0ef6448..da072a3d 100644 --- a/internal/scanner/probe_repair.go +++ b/internal/scanner/probe_repair.go @@ -2,6 +2,7 @@ package scanner import ( "context" + "log/slog" "strings" "sync" "time" @@ -159,9 +160,15 @@ func (e *PlaybackProbeEnsurer) ensureCopySafety(ctx context.Context, file *model multi, err := DetectMultiplePPSH264(scanCtx, e.ffmpegPath, file.FilePath) cancel() 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 + // Unknown safety must not fail open to the video-copy path this probe is + // intended to guard. Leave MultiplePPS unset and do not cache the result, + // so a later request retries the scan without misreporting the cause. + slog.WarnContext(ctx, "video copy-safety scan failed; disabling stream copy", + "component", "scanner", + "file_id", file.ID, + "error", err, + ) + return fileWithCopySafety(file, nil, true), nil } e.copySafety.Store(file.ID, copySafetyResult{size: file.FileSize, multi: multi}) @@ -172,11 +179,16 @@ func (e *PlaybackProbeEnsurer) ensureCopySafety(ctx context.Context, file *model // MultiplePPS flag set on its first video track, without mutating the caller's // file or its shared VideoTracks slice. func fileWithMultiplePPS(file *models.MediaFile, multi bool) *models.MediaFile { + value := multi + return fileWithCopySafety(file, &value, multi) +} + +func fileWithCopySafety(file *models.MediaFile, multiplePPS *bool, copyUnsafe bool) *models.MediaFile { updated := *file tracks := make([]models.VideoTrack, len(file.VideoTracks)) copy(tracks, file.VideoTracks) - value := multi - tracks[0].MultiplePPS = &value + tracks[0].MultiplePPS = multiplePPS + tracks[0].VideoCopyUnsafe = copyUnsafe updated.VideoTracks = tracks return &updated } diff --git a/internal/scanner/probe_repair_copy_safety_test.go b/internal/scanner/probe_repair_copy_safety_test.go new file mode 100644 index 00000000..a7e67334 --- /dev/null +++ b/internal/scanner/probe_repair_copy_safety_test.go @@ -0,0 +1,43 @@ +package scanner + +import ( + "context" + "path/filepath" + "testing" + + "github.com/Silo-Server/silo-server/internal/models" +) + +func TestEnsureCopySafetyFailureDisablesCopyWithoutCaching(t *testing.T) { + ensurer := &PlaybackProbeEnsurer{ + ffmpegPath: filepath.Join(t.TempDir(), "missing-ffmpeg"), + } + file := &models.MediaFile{ + ID: 42, + FilePath: "/library/movie.mkv", + FileSize: 1234, + CodecVideo: "h264", + VideoTracks: []models.VideoTrack{{Codec: "h264"}}, + } + + got, err := ensurer.ensureCopySafety(context.Background(), file) + if err != nil { + t.Fatalf("ensureCopySafety() error = %v", err) + } + if got == file { + t.Fatal("ensureCopySafety() returned the caller's file instead of an annotated copy") + } + track := got.VideoTracks[0] + if !track.VideoCopyUnsafe { + t.Fatal("VideoCopyUnsafe = false, want true after an inconclusive scan") + } + if track.MultiplePPS != nil { + t.Fatalf("MultiplePPS = %v, want nil when the scan did not produce a result", *track.MultiplePPS) + } + if _, ok := ensurer.copySafety.Load(file.ID); ok { + t.Fatal("inconclusive scan result was cached") + } + if file.VideoTracks[0].VideoCopyUnsafe || file.VideoTracks[0].MultiplePPS != nil { + t.Fatal("ensureCopySafety() mutated the caller's file") + } +}