diff --git a/docs/superpowers/plans/2026-07-28-transcode-resolution-clamp.md b/docs/superpowers/plans/2026-07-28-transcode-resolution-clamp.md new file mode 100644 index 00000000..c5351b16 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-transcode-resolution-clamp.md @@ -0,0 +1,206 @@ +# Transcode Resolution Clamp Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Prevent encoded-video transports from targeting a resolution above the final effective source file. + +**Architecture:** Add one pure resolution-normalization helper in the playback API handler and invoke it exactly once after alternate-file selection. Mutate the request's target resolution at that boundary so remote transport, local transport, persistence, activity reporting, and reconstruction consume the same normalized recipe. + +**Tech Stack:** Go, `net/http` handler tests, Silo playback session and transcode-node test doubles. + +## Global Constraints + +- Do not apply the new clamp to video-copy requests; preserve their existing + downstream recipe normalization. +- Preserve empty or unrecognized requested/source resolutions unchanged. +- Clamp only recognized encoded-video targets above the recognized effective source. +- Do not alter bitrate, codecs, API shapes, status codes, settings, migrations, clients, or production configuration. +- Prove the regression RED before writing production code. + +--- + +### Task 1: Normalize the Effective Transcode Recipe + +**Files:** +- Modify: `internal/api/handlers/playback.go` +- Test: `internal/api/handlers/playback_test.go` + +**Interfaces:** +- Consumes: `transcodeStartRequest.TargetResolution`, `transcodeStartRequest.TargetCodecVideo`, and `models.MediaFile.Resolution` after alternate-file selection. +- Produces: `clampEncodedTargetResolution(requestedResolution, sourceResolution string) string`. +- Produces: one normalized `req.TargetResolution` consumed unchanged by remote/local transport construction and `buildTranscodeSessionReplacement`. + +- [ ] **Step 1: Strengthen the existing remote fallback test** + +Change `TestHandleStartTranscode_PreservesRecomputedBaseMethodAfterFallback` to +request 2160p at 10000 kbps. Assert that the captured +`transcodenode.TranscodeStartRequest.TargetResolution` is `1080p`, while the +effective alternate remains file 99 and the resulting playback session stores +`TargetResolution == "1080p"`. + +```go +strings.NewReader(`{"session_id":"` + startResp.SessionID + + `","seek_seconds":0,"target_resolution":"2160p",` + + `"target_codec_video":"h264","target_codec_audio":"aac",` + + `"target_bitrate_kbps":10000,"segment_duration":2,` + + `"subtitle_track_index":-1,"subtitle_burn_in":false}`) + +if remoteStartReq.TargetResolution != "1080p" { + t.Fatalf("remote target resolution = %q, want 1080p", remoteStartReq.TargetResolution) +} +if session.TargetResolution != "1080p" { + t.Fatalf("session target resolution = %q, want 1080p", session.TargetResolution) +} +``` + +- [ ] **Step 2: Run the regression and verify RED** + +Run: + +```bash +go test ./internal/api/handlers -run TestHandleStartTranscode_PreservesRecomputedBaseMethodAfterFallback -count=1 +``` + +Expected: FAIL because the captured remote request still contains `2160p`. + +- [ ] **Step 3: Add pure normalization table tests** + +Add `TestClampEncodedTargetResolution` beside the handler tests. Cover: + +```go +tests := []struct { + name, requested, source, want string +}{ + {"clamps 2160p to 1080p", "2160p", "1080p", "1080p"}, + {"keeps lower target", "720p", "1080p", "720p"}, + {"keeps equal target", "1080p", "1080p", "1080p"}, + {"keeps empty target", "", "1080p", ""}, + {"keeps unknown target", "source", "1080p", "source"}, + {"keeps target for unknown source", "2160p", "native", "2160p"}, + {"supports low tiers", "480p", "420p", "420p"}, +} +``` + +- [ ] **Step 4: Add the minimal helper and post-selection call** + +Add a package-level recognized-height map/helper near `resolutionRank`: + +```go +func clampEncodedTargetResolution(requestedResolution, sourceResolution string) string { + requestedHeight, requestedKnown := transcodeResolutionHeight(requestedResolution) + sourceHeight, sourceKnown := transcodeResolutionHeight(sourceResolution) + if !requestedKnown || !sourceKnown || requestedHeight <= sourceHeight { + return requestedResolution + } + return sourceResolution +} +``` + +After the 4K alternate block and before transport planning: + +```go +if !videoCopy { + req.TargetResolution = clampEncodedTargetResolution( + req.TargetResolution, + file.Resolution, + ) +} +``` + +Use an exhaustive switch for the six supported transcode tiers. Do not modify +the FFmpeg filter builder or web client. + +- [ ] **Step 5: Verify focused GREEN** + +Run: + +```bash +go test ./internal/api/handlers \ + -run 'TestHandleStartTranscode_PreservesRecomputedBaseMethodAfterFallback|TestClampEncodedTargetResolution' \ + -count=1 +``` + +Expected: PASS. + +- [ ] **Step 6: Add copy and local-boundary regression coverage** + +Add focused tests proving: + +- a copy-video request bypasses the new clamp and retains its existing + downstream empty-resolution recipe; +- a local encoded-video request receives the clamped target in the persisted + session and generated FFmpeg recipe boundary; +- a requested 720p encode for a 1080p effective file remains 720p. + +Use the existing local fake-FFmpeg and session-manager patterns in +`playback_test.go`; do not introduce production test hooks. + +- [ ] **Step 7: Run focused and package verification** + +Run: + +```bash +gofmt -w internal/api/handlers/playback.go internal/api/handlers/playback_test.go +go test ./internal/api/handlers -count=1 +go test ./internal/playback -count=1 +go test ./internal/api/handlers ./internal/playback -race -count=1 +git diff --check +``` + +Expected: all commands exit 0. + +- [ ] **Step 8: Commit implementation** + +```bash +git add internal/api/handlers/playback.go internal/api/handlers/playback_test.go +git commit -m "fix(playback): prevent transcode resolution upscaling" +``` + +### Task 2: Independent Review and Final Verification + +**Files:** +- Review: `docs/superpowers/specs/2026-07-28-transcode-resolution-clamp-design.md` +- Review: `internal/api/handlers/playback.go` +- Review: `internal/api/handlers/playback_test.go` + +**Interfaces:** +- Consumes: committed spec and implementation diff from `upstream/main`. +- Produces: reviewer verdict and a clean, verified branch suitable for a separate pull request. + +- [ ] **Step 1: Request focused correctness/security review** + +Review the change against these invariants: + +- no encoded output exceeds the effective source tier; +- copy and unknown-resolution compatibility is preserved; +- local, offloaded, persisted, and reconstruction paths share one value; +- no target-bitrate or API behavior was broadened; +- tests fail without the production correction. + +- [ ] **Step 2: Address Critical or Important findings test-first** + +For each accepted behavioral finding, add or strengthen a failing test, observe +RED, make the smallest production correction, and rerun focused GREEN. Do not +bundle unrelated cleanup. + +- [ ] **Step 3: Run final verification** + +Run: + +```bash +go test ./internal/api/handlers ./internal/playback -count=1 +go test ./internal/api/handlers ./internal/playback -race -count=1 +go test ./... -count=1 +make verify-local-paths +git diff --check +git status --short --branch +``` + +Expected: tests and repository policy checks exit 0; status contains no +uncommitted implementation changes. + +- [ ] **Step 4: Prepare handoff** + +Summarize the production evidence, exact normalization boundary, RED/GREEN +proof, focused/full verification, reviewer verdict, and deployment caveat. +Do not deploy or mutate production as part of this plan. diff --git a/docs/superpowers/specs/2026-07-28-transcode-resolution-clamp-design.md b/docs/superpowers/specs/2026-07-28-transcode-resolution-clamp-design.md new file mode 100644 index 00000000..5a35a958 --- /dev/null +++ b/docs/superpowers/specs/2026-07-28-transcode-resolution-clamp-design.md @@ -0,0 +1,86 @@ +# Transcode Resolution Clamp Design + +## Problem + +When video encoding is requested for a 2160p file while 4K transcoding is +disabled, the playback handler selects a lower-resolution alternate file. +Today it continues using the target resolution supplied for the original file. +That lets a 1080p alternate reach FFmpeg with a 2160p target, causing an +unnecessary upscale and making the persisted playback activity claim that +2160p was delivered. + +Production logs confirmed this exact path with a 1080p H.264 alternate and a +QSV `scale_vaapi` filter targeting 2160 lines. + +## Decision + +The server owns the output-resolution invariant. After all alternate-file and +probe selection has completed, and before planning or starting any local or +remote transport, it will normalize an encoded-video target so it cannot +exceed the effective source file's resolution. + +The normalization rules are: + +- Do not apply the new clamp to video-copy requests; preserve their existing + downstream recipe normalization. +- Preserve an empty or unrecognized requested resolution unchanged. +- Preserve an empty or unrecognized effective source resolution unchanged. +- Preserve targets at or below the effective source resolution. +- Clamp a recognized target above a recognized effective source resolution to + the effective source resolution. +- Do not alter target bitrate. The observed 10 Mbps value is already a valid + 1080p-high encoding target and is not the demonstrated defect. + +Recognized resolutions are the same tiers already supported by the transcode +filter builders: 2160p, 1080p, 720p, 480p, 420p, and 328p. + +## Architecture and Data Flow + +A small pure helper in `internal/api/handlers/playback.go` compares requested +and effective-source resolution tiers. `HandleStartTranscode` calls it once +after the 4K alternate guard has selected and probed the effective file. + +The normalized value replaces `req.TargetResolution`. Existing downstream +paths then automatically use one consistent value for: + +- transcode-node planning and requests; +- integrated/local `playback.TranscodeOpts`; +- playback-session replacement and activity reporting; +- later transport reconstruction and restart recipes. + +This boundary is preferred over a web-only correction because every client can +call the endpoint, and over an FFmpeg-only correction because session state and +remote transcode requests must remain truthful. + +## Compatibility and Error Handling + +The change is additive enforcement inside the existing `/api/v1` endpoint. It +does not change request or response shapes, status codes, codec selection, +alternate-file selection, or copy-video behavior. Unsupported resolution +strings retain their existing behavior rather than being silently reinterpreted. + +No server settings, migrations, client changes, or production deployment +changes are part of this work. + +## Verification + +Tests will establish: + +1. A 2160p encode request switched to a 1080p alternate reaches the remote + transcode node as 1080p and persists 1080p in the playback session. +2. The same normalized request reaches the local transcode option boundary. +3. Targets already below the source remain unchanged. +4. Copy-video and unknown-resolution requests remain unchanged. +5. Existing playback-handler and playback-package suites remain green. + +The regression must fail against unmodified main before the implementation is +added. + +## Security and Operational Impact + +The server currently permits an authenticated user with transcoding permission +to amplify compute and bandwidth by requesting a recognized output larger than +the effective source. The clamp removes that unnecessary amplification while +retaining all authorized transcoding behavior. + +Rollback is a single code commit. No data rollback is required. diff --git a/internal/api/handlers/playback.go b/internal/api/handlers/playback.go index 2d974791..9ee4350b 100644 --- a/internal/api/handlers/playback.go +++ b/internal/api/handlers/playback.go @@ -3149,6 +3149,9 @@ func (h *PlaybackHandler) HandleStartTranscode(w http.ResponseWriter, r *http.Re switchedFileID = &alt.ID } } + if !videoCopy { + req.TargetResolution = clampEncodedTargetResolution(req.TargetResolution, file.Resolution) + } if requestedFile != nil && file != nil && requestedFile.ID != file.ID { if err := preflightPlaybackFile(r.Context(), requestedFile, h.MissingMarker, h.EventsHub); err != nil && !isPlaybackFileMissing(err) { slog.WarnContext(r.Context(), "requested transcode file preflight failed; continuing with alternate file", "component", "api", @@ -3936,20 +3939,60 @@ func (h *PlaybackHandler) findAlternateFile(ctx context.Context, source *models. return candidates[0], nil } +const ( + transcodeResolution2160p = "2160p" + transcodeResolution1080p = "1080p" + transcodeResolution720p = "720p" + transcodeResolution480p = "480p" + transcodeResolution420p = "420p" + transcodeResolution328p = "328p" +) + // resolutionRank returns a numeric rank for resolution sorting. func resolutionRank(res string) int { - switch res { - case "2160p": - return 4 - case "1080p": - return 3 - case "720p": - return 2 - case "480p": - return 1 - case "328p": + height, known := transcodeResolutionHeight(res) + if !known { return 0 + } + + switch { + case height >= 2160: + return 4 + case height >= 1080: + return 3 + case height >= 720: + return 2 + case height >= 480: + return 1 default: return 0 } } + +func clampEncodedTargetResolution(requestedResolution, sourceResolution string) string { + requestedHeight, requestedKnown := transcodeResolutionHeight(requestedResolution) + sourceHeight, sourceKnown := transcodeResolutionHeight(sourceResolution) + if !requestedKnown || !sourceKnown || requestedHeight <= sourceHeight { + return requestedResolution + } + return sourceResolution +} + +func transcodeResolutionHeight(resolution string) (int, bool) { + switch resolution { + case transcodeResolution2160p: + return 2160, true + case transcodeResolution1080p: + return 1080, true + case transcodeResolution720p: + return 720, true + case transcodeResolution480p: + return 480, true + case transcodeResolution420p: + return 420, true + case transcodeResolution328p: + return 328, true + default: + return 0, false + } +} diff --git a/internal/api/handlers/playback_test.go b/internal/api/handlers/playback_test.go index 48f93e71..d1100d3f 100644 --- a/internal/api/handlers/playback_test.go +++ b/internal/api/handlers/playback_test.go @@ -1343,7 +1343,7 @@ func TestHandleStartTranscode_PreservesRecomputedBaseMethodAfterFallback(t *test transcodeReq := httptest.NewRequest( "POST", "/api/v1/playback/transcode/start", - strings.NewReader(`{"session_id":"`+startResp.SessionID+`","seek_seconds":0,"target_resolution":"720p","target_codec_video":"h264","target_codec_audio":"aac","target_bitrate_kbps":2000,"segment_duration":2,"subtitle_track_index":-1,"subtitle_burn_in":false}`), + strings.NewReader(`{"session_id":"`+startResp.SessionID+`","seek_seconds":0,"target_resolution":"2160p","target_codec_video":"h264","target_codec_audio":"aac","target_bitrate_kbps":10000,"segment_duration":2,"subtitle_track_index":-1,"subtitle_burn_in":false}`), ) transcodeReq = transcodeReq.WithContext(newAuthorizedPlaybackContext()) @@ -1359,6 +1359,12 @@ func TestHandleStartTranscode_PreservesRecomputedBaseMethodAfterFallback(t *test if remoteStartReq.AudioTrackIndex != 1 { t.Fatalf("remote audio_track_index = %d, want 1", remoteStartReq.AudioTrackIndex) } + if remoteStartReq.TargetResolution != transcodeResolution1080p { + t.Fatalf("remote target resolution = %q, want 1080p", remoteStartReq.TargetResolution) + } + if remoteStartReq.TargetBitrateKbps != 10000 { + t.Fatalf("remote target bitrate = %d, want unchanged 10000", remoteStartReq.TargetBitrateKbps) + } session, err := sessionMgr.GetSession(startResp.SessionID) if err != nil { @@ -1370,6 +1376,12 @@ func TestHandleStartTranscode_PreservesRecomputedBaseMethodAfterFallback(t *test if session.BasePlayMethod != playback.PlayRemux { t.Fatalf("session.BasePlayMethod = %q, want %q", session.BasePlayMethod, playback.PlayRemux) } + if session.TargetResolution != transcodeResolution1080p { + t.Fatalf("session target resolution = %q, want 1080p", session.TargetResolution) + } + if session.TargetBitrateKbps != 10000 { + t.Fatalf("session target bitrate = %d, want unchanged 10000", session.TargetBitrateKbps) + } } // newRemoteAudioSwitchSession builds a session that is already an offloaded @@ -2010,7 +2022,7 @@ func TestHandleStartTranscode_LocalPathPropagatesSelectedAudioTrack(t *testing.T transcodeReq := httptest.NewRequest( "POST", "/api/v1/playback/transcode/start", - strings.NewReader(`{"session_id":"`+startResp.SessionID+`","seek_seconds":0,"target_resolution":"720p","target_codec_video":"h264","target_codec_audio":"aac","target_bitrate_kbps":2000,"segment_duration":2,"subtitle_track_index":-1,"subtitle_burn_in":false}`), + strings.NewReader(`{"session_id":"`+startResp.SessionID+`","seek_seconds":0,"target_resolution":"2160p","target_codec_video":"h264","target_codec_audio":"aac","target_bitrate_kbps":10000,"segment_duration":2,"subtitle_track_index":-1,"subtitle_burn_in":false}`), ) transcodeReq = transcodeReq.WithContext(newAuthorizedPlaybackContext()) @@ -2030,6 +2042,22 @@ func TestHandleStartTranscode_LocalPathPropagatesSelectedAudioTrack(t *testing.T if got := transcodeSession.Opts().AudioTrackIndex; got != 1 { t.Fatalf("local transcode audio track index = %d, want 1", got) } + if got := transcodeSession.Opts().TargetResolution; got != transcodeResolution1080p { + t.Fatalf("local target resolution = %q, want 1080p", got) + } + if got := transcodeSession.Opts().TargetBitrateKbps; got != 10000 { + t.Fatalf("local target bitrate = %d, want unchanged 10000", got) + } + session, err := sessionMgr.GetSession(startResp.SessionID) + if err != nil { + t.Fatalf("GetSession: %v", err) + } + if session.TargetResolution != transcodeResolution1080p { + t.Fatalf("session target resolution = %q, want 1080p", session.TargetResolution) + } + if session.TargetBitrateKbps != 10000 { + t.Fatalf("session target bitrate = %d, want unchanged 10000", session.TargetBitrateKbps) + } } func TestHandleChangeAudioTrack_LocalCopyRestartUsesFreshSeekAnchor(t *testing.T) { @@ -2400,7 +2428,7 @@ func TestHandleStartTranscode_SeekedCopyPreservedOnRemoteNode(t *testing.T) { transcodeReq := httptest.NewRequest( http.MethodPost, "/api/v1/playback/transcode/start", - strings.NewReader(`{"session_id":"`+session.ID+`","seek_seconds":18.261,"target_resolution":"","target_codec_video":"copy","target_codec_audio":"aac","target_bitrate_kbps":0,"segment_duration":2,"subtitle_track_index":-1,"subtitle_burn_in":false}`), + strings.NewReader(`{"session_id":"`+session.ID+`","seek_seconds":18.261,"target_resolution":"2160p","target_codec_video":"copy","target_codec_audio":"aac","target_bitrate_kbps":0,"segment_duration":2,"subtitle_track_index":-1,"subtitle_burn_in":false}`), ).WithContext(newAuthorizedPlaybackContext()) transcodeRR := httptest.NewRecorder() handler.HandleStartTranscode(transcodeRR, transcodeReq) @@ -2411,6 +2439,9 @@ func TestHandleStartTranscode_SeekedCopyPreservedOnRemoteNode(t *testing.T) { remoteStartReq.StreamOriginSeconds != 16 || !remoteStartReq.CopySeekAnchorResolved || remoteStartReq.StartSegmentNumber != 8 { t.Fatalf("remote copy recipe = codec %q seek %v origin %v segment %d", remoteStartReq.TargetCodecVideo, remoteStartReq.SeekSeconds, remoteStartReq.StreamOriginSeconds, remoteStartReq.StartSegmentNumber) } + if remoteStartReq.TargetResolution != transcodeResolution2160p { + t.Fatalf("remote copy target resolution = %q, want unchanged 2160p", remoteStartReq.TargetResolution) + } if !strings.HasPrefix(remoteStartReq.SessionID, session.ID+legacyTransportMarker) { t.Fatalf("remote SessionID = %q, want legacy transport for %q", remoteStartReq.SessionID, session.ID) } @@ -2904,6 +2935,9 @@ func TestHandleStartTranscode_SeekedCopyAllowedWhenVideoTranscodingDisabled(t *t if got := transcodeSession.Opts().TargetCodecVideo; got != "copy" { t.Fatalf("target video codec = %q, want copy", got) } + if got := transcodeSession.Opts().TargetResolution; got != "" { + t.Fatalf("copy-video target resolution = %q, want existing empty recipe", got) + } } func TestHandleStartTranscode_SubtitleBurnInRechecksVideoTranscodePermission(t *testing.T) { @@ -3369,6 +3403,127 @@ func TestFindAlternateFile_DoesNotCrossEdition(t *testing.T) { } } +func TestClampEncodedTargetResolution(t *testing.T) { + tests := []struct { + name string + requested string + source string + want string + }{ + { + name: "clamps 2160p to 1080p", + requested: transcodeResolution2160p, + source: transcodeResolution1080p, + want: transcodeResolution1080p, + }, + { + name: "keeps lower target", + requested: transcodeResolution720p, + source: transcodeResolution1080p, + want: transcodeResolution720p, + }, + { + name: "keeps equal target", + requested: transcodeResolution1080p, + source: transcodeResolution1080p, + want: transcodeResolution1080p, + }, + { + name: "keeps empty target", + requested: "", + source: transcodeResolution1080p, + want: "", + }, + { + name: "keeps unknown target", + requested: "unrecognized-target", + source: transcodeResolution1080p, + want: "unrecognized-target", + }, + { + name: "keeps target for unknown source", + requested: transcodeResolution2160p, + source: "native", + want: transcodeResolution2160p, + }, + { + name: "supports low tiers", + requested: transcodeResolution480p, + source: transcodeResolution420p, + want: transcodeResolution420p, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := clampEncodedTargetResolution(tt.requested, tt.source); got != tt.want { + t.Fatalf( + "clampEncodedTargetResolution(%q, %q) = %q, want %q", + tt.requested, + tt.source, + got, + tt.want, + ) + } + }) + } +} + +func TestTranscodeResolutionHeight(t *testing.T) { + tests := []struct { + resolution string + wantHeight int + wantKnown bool + }{ + {resolution: transcodeResolution2160p, wantHeight: 2160, wantKnown: true}, + {resolution: transcodeResolution1080p, wantHeight: 1080, wantKnown: true}, + {resolution: transcodeResolution720p, wantHeight: 720, wantKnown: true}, + {resolution: transcodeResolution480p, wantHeight: 480, wantKnown: true}, + {resolution: transcodeResolution420p, wantHeight: 420, wantKnown: true}, + {resolution: transcodeResolution328p, wantHeight: 328, wantKnown: true}, + {resolution: "unrecognized-tier", wantHeight: 0, wantKnown: false}, + } + + for _, tt := range tests { + t.Run(tt.resolution, func(t *testing.T) { + gotHeight, gotKnown := transcodeResolutionHeight(tt.resolution) + if gotHeight != tt.wantHeight || gotKnown != tt.wantKnown { + t.Fatalf( + "transcodeResolutionHeight(%q) = (%d, %t), want (%d, %t)", + tt.resolution, + gotHeight, + gotKnown, + tt.wantHeight, + tt.wantKnown, + ) + } + }) + } +} + +func TestResolutionRank(t *testing.T) { + tests := []struct { + resolution string + want int + }{ + {resolution: transcodeResolution2160p, want: 4}, + {resolution: transcodeResolution1080p, want: 3}, + {resolution: transcodeResolution720p, want: 2}, + {resolution: transcodeResolution480p, want: 1}, + {resolution: transcodeResolution420p, want: 0}, + {resolution: transcodeResolution328p, want: 0}, + {resolution: "unrecognized-tier", want: 0}, + } + + for _, tt := range tests { + t.Run(tt.resolution, func(t *testing.T) { + if got := resolutionRank(tt.resolution); got != tt.want { + t.Fatalf("resolutionRank(%q) = %d, want %d", tt.resolution, got, tt.want) + } + }) + } +} + func TestAlignedSeekSeconds(t *testing.T) { tests := []struct { name string