generate animated GIF previews for videos (upload, share OG, hover)#1790
generate animated GIF previews for videos (upload, share OG, hover)#1790richiemcilroy merged 14 commits intomainfrom
Conversation
brin-security-scanner
Bot
added
contributor:verified
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (err) { | ||
| lastError = err; | ||
| await outputTempFile.cleanup(); | ||
| if (index === attempts.length - 1) { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
When the abortSignal fires during attempt N, runPreviewGifAttempt kills that ffmpeg process and throws. The catch block stores the error and, because index < attempts.length - 1, silently continues to the next attempt. On the next call to runPreviewGifAttempt, abortSignal is already in the aborted state — AbortSignal.addEventListener does not re-fire for an already-aborted signal, so the freshly-spawned ffmpeg process is never killed and runs to completion or hits its 30-second timeout. This means a cancelled job can run up to three extra 30-second ffmpeg processes after cancellation.
Add a guard at the top of the catch block:
if (abortSignal?.aborted) {
await outputTempFile.cleanup();
throw err instanceof Error ? err : new Error("Preview GIF generation aborted");
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/media-server/src/lib/ffmpeg-video.ts
Line: 1297-1303
Comment:
**Abort signal not honoured between retry attempts**
When the `abortSignal` fires during attempt N, `runPreviewGifAttempt` kills that ffmpeg process and throws. The catch block stores the error and, because `index < attempts.length - 1`, silently continues to the next attempt. On the next call to `runPreviewGifAttempt`, `abortSignal` is already in the aborted state — `AbortSignal.addEventListener` does **not** re-fire for an already-aborted signal, so the freshly-spawned ffmpeg process is never killed and runs to completion or hits its 30-second timeout. This means a cancelled job can run up to three extra 30-second ffmpeg processes after cancellation.
Add a guard at the top of the catch block:
```ts
if (abortSignal?.aborted) {
await outputTempFile.cleanup();
throw err instanceof Error ? err : new Error("Preview GIF generation aborted");
}
```
How can I resolve this? If you propose a fix, please make it concise.| const response = NextResponse.redirect(previewUrl, 302); | ||
| response.headers.set("Cache-Control", "public, max-age=300"); | ||
| return response; |
There was a problem hiding this comment.
public cache-control
The redirect target is a pre-signed S3 URL (valid for 1 hour). Responding with Cache-Control: public, max-age=300 allows shared caches and CDNs to cache and replay this redirect. For public videos this is probably acceptable, but the same header is applied to the fallback (no-preview) redirect — callers within the 300-second window will receive the stale fallback even after a preview is later generated. A private directive (or s-maxage=0) on the fallback path would avoid stale negative caches.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/video/preview/route.ts
Line: 64-66
Comment:
**Signed S3 URL redirected with `public` cache-control**
The redirect target is a pre-signed S3 URL (valid for 1 hour). Responding with `Cache-Control: public, max-age=300` allows shared caches and CDNs to cache and replay this redirect. For public videos this is probably acceptable, but the same header is applied to the fallback (no-preview) redirect — callers within the 300-second window will receive the stale fallback even after a preview is later generated. A `private` directive (or `s-maxage=0`) on the fallback path would avoid stale negative caches.
How can I resolve this? If you propose a fix, please make it concise.
|
please re-review the pr @greptileai |
1 participant
Adds server-side preview GIF generation via FFmpeg, uploads it to S3 on process/mux and web upload paths, exposes a signed /api/video/preview redirect, uses that GIF in share Open Graph / Twitter metadata, and shows a hover preview on thumbnails. Also fixes extractAudio so the default timeout is honored when options are merged, and tightens related media-server tests.
Greptile Summary
This PR wires up server-side animated GIF preview generation via FFmpeg across all video ingest paths (process, mux-segments, web upload, Loom import), exposes a signed
/api/video/previewredirect endpoint, surfaces the GIF in share Open Graph/Twitter metadata, and shows it as a hover overlay on video thumbnails. It also fixes a pre-existing bug inextractAudiowhere the caller-suppliedtimeoutMswas silently ignored in favour of the hardcoded constant.ffmpeg-video.ts):generatePreviewGifruns up to four attempts with progressively lower quality settings (fps, dimensions, colours, duration) until the output fits withinmaxBytes; abort-signal propagation and temp-file cleanup are handled at each attempt boundary./api/video/preview): checks S3 for the GIF viaheadObject, returns a 302 to a 1-hour signed URL (5-min public cache), or falls back to the static OG image; the no-GIF fallback correctly usesprivate, no-storeto avoid stale negative caches.extractAudiofix (ffmpeg.ts):timeoutMsis now included inDEFAULT_OPTIONSand the mergedopts.timeoutMsis used in bothextractAudioandextractAudioStream, so callers that pass a custom timeout are no longer silently overridden.Confidence Score: 5/5
The change is safe to merge; the preview GIF path is non-blocking and the core video processing paths are unchanged.
All new code paths are best-effort: failures are swallowed with a warning rather than surfacing to users or failing jobs. The extractAudio timeout fix is straightforward. The only new code concern is unreachable dead code after the retry loop in generatePreviewGif, which is a clarity issue and does not affect runtime behaviour.
No files require special attention.
Important Files Changed
Comments Outside Diff (1)
apps/media-server/src/lib/ffmpeg-video.ts, line 169-207 (link)timeoutMsandmaxBytesare clamped to their defaults, making them effectively one-directionalgetPreviewGifOptionsclamps bothmaxBytesandtimeoutMsagainstDEFAULT_PREVIEW_GIF_OPTIONS.*, so callers can never supply a value larger than the built-in defaults (1.5 MB / 30 s). This may be intentional as a safety cap, but it's a silent footgun: passing{ timeoutMs: 60_000 }silently returns 30 000 with no warning. If the intent is a strict upper-bound, a comment would clarify the design; if callers genuinely need wider ranges, the clamps should be removed.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: address gif preview review" | Re-trigger Greptile