fix: browser can't decode ProRes-alpha video, transcode as fallback - #11
fix: browser can't decode ProRes-alpha video, transcode as fallback#11GinoongFlores wants to merge 1 commit into
Conversation
- ProRes 4444 alpha .mov (e.g. from face-cutout pipelines) has no browser decoder at all, so affected clips silently rendered as a blank frame in both preview and export -- decode errors were only console.warn'd - add /api/video/transcode-alpha on ai-backend: ffmpeg re-encode to WebM VP9 alpha (yuva420p), same format the app's own remove-bg-video already produces and can play natively - video-cache: on decode failure, transcode once and retry with the result instead of giving up silently; normal (non-alpha) imports are untouched, this only triggers when native decode actually fails
There was a problem hiding this comment.
Summary
Reviewed — found 5 issue(s). This PR adds a server-side ffmpeg-based ProRes-alpha → WebM VP9 transcode fallback in the AI backend, with a retry-once pattern in VideoCache that catches client-side decode failures, uploads the file to a new /api/video/transcode-alpha endpoint, and retries initialization with the transcoded result. The core retry logic is sound and well-gated, but the backend endpoint has a disk-leak issue and a memory safety concern worth addressing.
Findings
apps/web/src/lib/ai-client.ts
Hardcoded 10-minute timeout duplicates existing constant — The timeout
10 * 60 * 1000on the newtranscodeAlphaVideomethod is exactlyLLM_TIMEOUT_MS(600,000). Reusing the existing constant would prevent drift and make intent clearer.Duplicated fetch/error-handling boilerplate —
transcodeAlphaVideoreplicates therequestFormDatapattern (timeout +AbortController+classifyError) nearly verbatim. Consider a shared helper or parameterized method to avoid maintenance risk.
apps/web/src/services/video-cache/service.ts
No issues found. The retry-once guard (transcodedFiles.has(mediaId)) correctly prevents infinite retry loops, and the transcodedFiles map is properly cleaned up in removeSink.
services/ai-backend/app/routes/video.py
Transcoded output files never cleaned up — The
finallyblock only removes the upload input file (upload_path). The transcoded WebM atoutput_pathinGENERATED_DIRis never deleted afterFileResponseserves it. This will accumulate disk usage over time with no automatic cleanup.Full file read into memory before size check —
contents = await file.read()reads the entire upload into memory before validating againstMAX_UPLOAD_SIZE(500 MB). A large file upload can exhaust RAM before the 413 is raised.No authentication on transcode endpoint — The new
/api/video/transcode-alphaendpoint has no auth checks. An unauthenticated caller could trigger CPU-intensive VP9 ffmpeg encodes. This is consistent with other video routes in the file, but the compute cost of VP9 encoding makes this worth flagging.
Verdict
GENERATED_DIR. The memory-read-before-size-check is a hardening improvement that can be a follow-up.
| const controller = new AbortController(); | ||
| // Re-encoding is slow (VP9), give it more room than the default. | ||
| const timeoutId = setTimeout(() => controller.abort(), 10 * 60 * 1000); | ||
There was a problem hiding this comment.
Suggestion: The hardcoded timeout 10 * 60 * 1000 (10 minutes) is exactly the same value as the existing LLM_TIMEOUT_MS constant (600_000, defined at the top of this file). Consider reusing LLM_TIMEOUT_MS or renaming it to LONG_TIMEOUT_MS so the intent is clearer and values don't drift if the timeout needs adjustment.
| async exportRender( | ||
| projectData: unknown, | ||
| ): Promise<{ videoUrl: string }> { |
There was a problem hiding this comment.
Suggestion: The transcodeAlphaVideo method duplicates the requestFormData pattern (timeout + AbortController + classifyError + finally cleanup) nearly verbatim, changing only the response handler (.blob() vs .json()) and the error message prefix. Consider refactoring to avoid ~30 lines of boilerplate — e.g., a shared helper that accepts a response handler callback, so the error-handling pattern stays centralized if it ever needs to change.
| raise | ||
| except Exception: | ||
| logger.exception("Alpha video transcode failed for '%s'", file.filename) | ||
| raise HTTPException(status_code=500, detail="Alpha video transcode failed.") |
There was a problem hiding this comment.
Issue: The finally block only removes the upload input file (upload_path). The transcoded WebM output at output_path in GENERATED_DIR is never deleted after FileResponse serves it. Every successful transcode permanently writes a .webm file to disk, which will accumulate over time with no automatic cleanup. Consider adding a background task to delete the output file after the response completes (response.background = BackgroundTask(os.remove, output_path)), or implementing a TTL-based sweep for GENERATED_DIR.
| upload_path = os.path.join(settings.UPLOAD_DIR, f"alpha_{upload_id}{ext}") | ||
| output_filename = f"alpha_{upload_id}.webm" | ||
| output_path = os.path.join(settings.GENERATED_DIR, output_filename) | ||
There was a problem hiding this comment.
Improvement:contents = await file.read() reads the entire upload into memory on line 228, and the MAX_UPLOAD_SIZE check happens after on line 229. With MAX_UPLOAD_SIZE set to 500 MB, a single large-file upload can exhaust RAM before the size check can reject it. The size validation should happen first — e.g., by checking the Content-Length header before reading, or using FastAPI's built-in UploadFile size limits. This is more of a hardening concern but worth noting given the 500 MB cap.
| @@ -196,6 +201,77 @@ async def generate_video(req: VideoGenerateRequest, request: Request) -> dict: | |||
| } | |||
There was a problem hiding this comment.
Improvement: The new /api/video/transcode-alpha endpoint has no authentication or authorization checks. An unauthenticated caller could upload arbitrary files and trigger CPU-intensive VP9 ffmpeg transcodes — a potential DoS vector. This is consistent with the rest of the video routes in this file (which also lack auth), but the computational cost of VP9 encoding is significantly higher than typical API operations. If the backend is ever exposed beyond localhost, this should be addressed.
Risk AssessmentSmall, focused diff across three files that adds a server-side ProRes-alpha → WebM VP9 transcode fallback. The core retry logic is well-gated and the ffmpeg command uses safe flags. The main operational concern is a disk space leak from transcoded output files never being cleaned up. Risk score: 3/10 (threshold: 3) — within the configured risk threshold Concerns
Verdict✅ Safe to ship. The disk cleanup issue should be addressed in a follow-up PR, but the change is low-risk overall — it's gated behind a client-side decode failure, uses a retry-once guard, and the fallback path properly integrates with the existing |
There was a problem hiding this comment.
Summary
Reviewed all changes across three files (frontend ai-client.ts, video-cache/service.ts, and backend video.py). The core retry-once transcode fallback logic is sound, well-gated, and properly integrated with the existing initPromises concurrency control. Risk assessment scored 3/10 — within the configured threshold.
Approving with the note that the disk-leak issue (transcoded WebM files never cleaned up from GENERATED_DIR) should be addressed in a follow-up.
ProRes 4444 alpha .mov (e.g. from face-cutout pipelines) has no browser decoder at all -- affected clips silently rendered as a blank frame in both preview and export. Decode errors were only
console.warn'd, never surfaced./api/video/transcode-alphaon ai-backend: ffmpeg re-encode to WebM VP9 alpha (yuva420p), same format the app's own remove-bg-video already produces and can play natively