Ask ffmpeg for the frames the capturer actually wrote - #284
Merged
Merged
Conversation
Every animated banner encode failed in production. The frame filename pattern was written down twice and the copies drifted: the capturer emits f-0000.png, and the GIF encoder asked ffmpeg for frame-0000.png, so palettegen exited with "Could find no file with path" on the first unit of every render. There is an exported FRAME_PATTERN for exactly this reason and the GIF path should have used it. It does now. The guard that existed to catch this did not, because it counted PNG files rather than looking at their names — fifty files were present, none of them named what ffmpeg was asked for. It now checks that each expected filename is present and names the first one missing, so the same class of mistake fails here with a sentence that says what happened rather than inside ffmpeg. The test could not have caught it either: its synthetic capturer wrote the encoder's spelling instead of the real one, which is how a divergence between two hardcoded copies passed CI. It derives both from FRAME_PATTERN now. Separately, a failed banner was failing the whole revision. The comment beside that catch claimed it would not — everything pushed onto `problems` fails the render, and the video for this campaign was fine. Three renders of a perfectly good pre-roll were thrown away over a GIF. The failure is logged and the revision proceeds; the banners are optional profiles and their absence is already visible in the asset list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan48 finding(s) HIGH/CRITICAL: 2 | MEDIUM: 31 | LOW: 15
Snippets are redacted; ThreatCrush never prints matched credential material. |
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.
Every animated banner encode failed in production. Caught by queueing one real render after the deploy and reading the worker log rather than assuming green CI meant working.
The frame pattern was written down twice and drifted
The capturer emits
f-0000.png. The GIF encoder asked ffmpeg forframe-0000.png. There is an exportedFRAME_PATTERNconstant for exactly this reason and the GIF path should have used it — it does now.Three things failed to catch it, and all three are fixed
The guard counted files instead of checking names. Fifty PNGs were present; none was named what ffmpeg was asked for, so
captured.length !== GIF_FRAMESpassed happily and the mismatch surfaced inside ffmpeg instead. It now verifies each expected filename is present and names the first one missing.The test wrote the encoder's spelling. Its synthetic capturer used
frame-%04d.png— the same wrong literal — so two hardcoded copies agreed with each other and disagreed with production. Both now derive fromFRAME_PATTERN, which is what makes the test capable of catching this class of bug at all.A failed banner failed the whole revision. The comment beside that
catchclaimed it wouldn't. Everything pushed ontoproblemsfails the render, so three renders of a perfectly good pre-roll were thrown away over a GIF. The failure is now logged and the revision proceeds; the banners are optional profiles and their absence is already visible in the asset list.That third one is the more serious of the two bugs: it means my "a banner must not fail the video" design existed only in prose.
Verification
2563 passed / 1 failed repo-wide — the pre-existing
tracker-geofailure (mmdb not installed locally). Typechecks clean.CI cannot prove this one end to end, since the real capturer needs Chromium. I'll queue a live render after this deploys and confirm three GIFs land before reporting the feature as working — and look at them.