Render real video, with OpenCut's engine - #22
Open
deonmenezes wants to merge 5 commits into
Open
deonmenezes wants to merge 5 commits into
deonmenezes wants to merge 5 commits into
Conversation
The README described the timeline tools but not the thing people ask about first: what you can say to it, and what happens at each of the three layers the agent drives (your UI, ours, the ffmpeg encode). It now carries the full 16-tool reference with arguments, the connector story including how to attach an MCP server mid-conversation, both sandboxes and what each is for, the environment table, test counts, and a "Known limits" section, because export writes a render description rather than encoding video and the demo does not make that obvious. The landing page in site/ renders the agent server's own sample project and performs a real ripple delete on it: mapTime() applies the same rule project.ts does, so clips straddling a silence get shorter rather than merely shifting, and 24.0s becomes 21.1s. Its palette is lifted from the editor's stylesheet and favicon rather than invented, so the page and the product read as one thing. Two numbers in the README were wrong and are corrected here: the silence ranges in the walkthrough were sketched rather than read off project.ts (they are 3.2-4.1, 9.6-10.4, 16.8-18.0), and .env.example lives in apps/agent, not the repo root.
The navbar CTA said "GitHub" in text, which is the one link on the page people scan for by icon rather than by reading. The mark is defined once as an SVG symbol and used in all three places the link appears: the navbar, the outro button, and the footer, so they read as one system instead of three unrelated links. The violet button picks up an inset top highlight and a soft cast of its own colour, which is what separates a button from a coloured rectangle at this size. Below 480px the label is clipped rather than hidden, so the button becomes a square mark and still announces "GitHub" to a screen reader.
The timeline was a simulation. Clips named media that did not exist, the preview mapped those names to CSS gradients, transcripts and silences were literals in the seed, and export_project wrote a JSON description of a render to a file called .mp4. The ffmpeg sandbox was real but orphaned: nothing connected a clip to bytes on disk, so it had nothing to point ffmpeg at. This ports OpenCut's renderer, adapted to EditAI's timeline. Import measures a real file with WebCodecs, uploads the bytes to the agent, and analyzes the audio: silences from windowed RMS, a peak envelope for the timeline waveform, tempo from onset autocorrelation. find_silences and detect_beats now answer from the file rather than from a fixture, and the waveform under an audio clip is its own. The preview is a canvas driven by the same engine the exporter uses, so what you watch is what gets encoded. Frames come from a cache taken from OpenCut: a forward iterator with the next frame decoded ahead, falling back to a seek only when the target is behind the decoder or too far in front. Audio is the whole-timeline mixdown, played through one buffer source and re-anchored when a scrub drifts it. Export is a job. The agent queues it, the editor claims it, composites every frame, muxes with mediabunny, and posts the file back; get_export then reports a real path and a real byte count, which is what lets the agent check its own work. Claiming is server-side, so two editors open on one project cannot both encode it, and a finished render is not reopened by a straggling progress or failure report from a losing worker. Compositing is Canvas2D, not OpenCut's wgpu compositor. EditAI stacks video, text and audio with no effects or masks, which 2D covers exactly, and it drops a wasm dependency. Effects would need the real thing. Two tools were missing and are added: list_media, because the agent could not see what had been imported, and add_clip, because it could add text to the timeline but never footage. Verified end to end against generated footage: a 24s timeline of three video clips, two audio tracks and a title renders to 1280x720 H.264 with 48 kHz stereo AAC, decoding real source frames at the right source times. Silence detection recovered all three injected gaps to a tenth of a second and tempo came back at exactly 120 BPM on a 120 BPM click track. data/project.json is no longer tracked. It was both gitignored and committed, so a clone started from whatever timeline was last saved instead of the seed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D8rspEEzFARv7H1opU9ER2
Contributor
PR Summary by QodoRender real media with OpenCut's WebCodecs engine
AI Description
Diagram
High-Level Assessment
Files changed (32)
|
Contributor
Code Review by Qodo
1. Active renders lose lease
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo raised ten bugs and ten rule violations. Eighteen were real. Security. POST /exports took format and resolution as free strings and made them path components, so "../../../outside" escaped the exports directory and the upload wrote attacker-supplied bytes there. Both are now checked against the allowed sets where the path is built rather than only at the MCP edge, which zod already guarded, and the project name is slugged for the same reason. Reliability. A claimed render could never be recovered: only the worker holding it could report failure, so a closed tab stranded the job in rendering forever. Claims now carry a sixty second lease that progress refreshes, and an expired one is claimable again. A reclaim also clears the abandoned partial first, which the review did not raise: writing a fresh encode over a longer previous attempt leaves its trailing bytes behind, which is a corrupt file rather than a retry. Memory. The encode buffered the whole output before uploading, so a long 4K render could exhaust the tab before a byte reached the server. It now streams through mediabunny's StreamTarget in 8 MiB chunks to positional writes on the server, and the muxer is back-pressured by the upload rather than queueing. Correctness. Renders took the live project, so an edit landing between approval and claim silently changed what was encoded; the timeline is now snapshotted beside the job and workers render that. Frames were keyed by media name, so two clips of one file at different offsets overwrote each other. bytes=-500 was read as the first 500 bytes rather than the last, which is where a non faststart mp4 keeps its index. Frame count came from Math.round, leaving the video up to half a frame longer or shorter than its own audio. Playback restarted on every project broadcast, which during a render is every two percent and audible. Pausing during the mixdown left audio playing over a paused preview. Encoders leaked on any failure after start: the finally block cleared the frame cache but left the video and audio sources and the muxer open, which can break the next render. Style, against the repo's own checklist: the void operator is gone in favour of a helper that also stops discarded rejections surfacing unhandled, forEach and nested ternaries are replaced, and the file input has a label. Two findings are not taken. The caption constants are not a precision-loss violation: that rule is about literals that cannot round-trip, and 0.055 does. The landing page's forEach and function expressions are real, but that script came from an earlier commit that only rides along in this branch, and rewriting 197 lines of untested animation code has nothing to do with the renderer. Verified again end to end after the rewrite of the upload path: a streamed 1080p render is valid H.264 and AAC with no leftover temp files, a suffix range returns the tail of the file, and an abandoned render was reclaimed after its lease expired and completed to a valid 720p file. Tests 43 in the agent, 20 in the editor, 8 in the merge gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D8rspEEzFARv7H1opU9ER2
The video-editing skill told the agent all media work happens through the ffmpeg-sandbox connector, but nothing ever created that connector: the package had no manifest, sat outside the workspace, and the README asked for manual clicks in TrueForge settings. Now packages/ffmpeg-sandbox is a workspace package with build:image and start scripts, and setup.ts probes its /health, registers it with the harness, and attaches it to the agent deferred with @destructive approval on run_python, the same gate the timeline's delete tools get. Down means skipped with a hint, not registered broken. Verified end to end: a containerized 9:16 crop rendered and probed through MCP, the harness lists all four tools, setup reruns clean, and a live turn answered "6 clips, 24 seconds", which matches the store. Docs and the tool table now match the code: 19 tools, real renders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYYfW5NnuCUx6sNk2GoA7V
Owner
Author
|
/review |
Comment on lines
+189
to
+190
| "Dockerized media workbench: list and probe media files, run ffmpeg renders, and script " + | ||
| "glue work with python. No network access; paths are relative to its workspace.", |
Contributor
There was a problem hiding this comment.
1. Merge sandbox description literals 📘 Rule violation ⚙ Maintainability
The connector description concatenates two static string literals even though a single literal would produce the same value. This violates the requirement to eliminate redundant static string concatenation.
Agent Prompt
## Issue description
The ffmpeg sandbox description is split across two static string literals joined with `+`, although no dynamic value is interpolated.
## Issue Context
PR Compliance ID 2993243 requires contiguous static strings to be represented as one literal.
## Fix Focus Areas
- apps/agent/scripts/setup.ts[189-190]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+319
to
+321
| ffmpegSandbox | ||
| ? `attached from ${FFMPEG_SANDBOX_URL}` | ||
| : "not running (cd packages/ffmpeg-sandbox && bun run build:image && bun run start), skipped" |
Contributor
There was a problem hiding this comment.
2. Expand multiline sandbox ternary 📘 Rule violation ⚙ Maintainability
The ffmpeg attachment status is selected with a ternary expression spread across several lines. The checklist requires multiline ternaries to be rewritten with clearer if/else control flow.
Agent Prompt
## Issue description
The ffmpeg sandbox status message uses a multiline conditional expression.
## Issue Context
PR Compliance ID 2993282 restricts ternaries to simple value selections and explicitly treats expressions spanning multiple lines as violations.
## Fix Focus Areas
- apps/agent/scripts/setup.ts[318-322]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Ceil, not round: a timeline that is not a whole number of frames long must still be | ||
| // covered to its end, and the final frame is shortened so the video lasts exactly as long | ||
| // as the audio rather than up to half a frame more. | ||
| const frameCount = Math.max(1, Math.ceil(project.duration * fps - 1e-9)) |
Contributor
There was a problem hiding this comment.
3. Frame epsilon loses precision 📘 Rule violation ≡ Correctness
The frame-count adjustment uses the decimal literal 1e-9, which is not exactly representable as an IEEE 754 JavaScript number. The checklist requires an exact representation rather than a precision-losing numeric literal.
Agent Prompt
## Issue description
The frame-count calculation directly subtracts the non-exact binary64 literal `1e-9`.
## Issue Context
PR Compliance ID 2993559 disallows decimal literals that are not exactly representable in IEEE 754 contexts. Preserve the intended whole-frame boundary handling with integer/scaled arithmetic or another exact formulation.
## Fix Focus Areas
- apps/web/src/engine/exporter.ts[126-129]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+65
to
+68
| fireAndForget( | ||
| agentJson(`/exports/${job.id}/progress`, { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, |
Contributor
There was a problem hiding this comment.
4. Active renders lose lease 🐞 Bug ☼ Reliability
The worker refreshes its 60-second lease only from frame-progress callbacks, but renderTimeline fully decodes and mixes audio before entering the frame loop that emits progress. A valid long render can therefore appear abandoned during startup and be claimed by another editor while the original worker is still active.
Agent Prompt
## Issue description
Active workers can exceed the lease before the first frame-progress callback and be reclaimed prematurely.
## Issue Context
Start an independent periodic heartbeat immediately after a successful claim and keep it running through snapshot loading, audio mixing, encoding, uploads, and finalization. Stop it in `finally`; do not rely on percentage changes as liveness.
## Fix Focus Areas
- apps/web/src/components/editor/use-render-worker.ts[34-89]
- apps/agent/src/project.ts[614-690]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+180
to
+192
| app.post("/exports/:id/chunk", async (req, res) => { | ||
| try { | ||
| const rec = store.getExport(req.params.id) | ||
| const position = Number((req.query as Record<string, string | undefined>).position) | ||
| if (!Number.isInteger(position) || position < 0) throw new Error("A non-negative integer ?position= is required.") | ||
| const data = await readBody(req, CHUNK_LIMIT) | ||
| if (data.length === 0) throw new Error("Chunk was empty.") | ||
| writeChunkAt(renderPath(rec.id), position, data) | ||
| res.json({ ok: true, position, bytes: data.length }) | ||
| } catch (err) { | ||
| fail(res, 400, err) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
5. Render endpoints lack claim authorization 🐞 Bug ☼ Reliability
The /exports/:id/chunk, /exports/:id/finish, and /exports/:id/failed routes identify attempts only by export ID and do not verify that the caller holds the current claim or that the export is still rendering. After a lease is reclaimed, a stale, duplicate, or unrelated client can corrupt the shared .render file, delete the replacement attempt's partial output, finalize stale output, or mark another worker's active or completed render as failed.
Agent Prompt
## Issue description
Reclaimed render attempts are not fenced: `/exports/:id/chunk`, `/exports/:id/finish`, and `/exports/:id/failed` accept mutations without verifying that the caller holds the current claim or that the export remains in the expected `rendering` state. This allows stale workers or unrelated clients to mutate the shared partial file and job status.
## Issue Context
Generate a unique attempt or lease token for every successful claim and return it to the render worker. Require that token for progress, chunk, failed, and finish operations, reject requests when it no longer matches the current claim, and verify that the export is still rendering before changing files or status.
## Fix Focus Areas
- apps/agent/src/project.ts[591-674]
- apps/agent/src/server.ts[128-202]
- apps/web/src/components/editor/use-render-worker.ts[34-85]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+636
to
+648
| completeExport(id: string, uploadedPath: string) { | ||
| const rec = this.getExport(id) | ||
| if (!existsSync(uploadedPath)) throw new Error(`No uploaded file at ${uploadedPath}.`) | ||
| // Moved before the commit: commit notifies subscribers synchronously, and a client that | ||
| // reacts to the finished export must not find the file missing. | ||
| mkdirSync(dirname(rec.file), { recursive: true }) | ||
| renameSync(uploadedPath, rec.file) | ||
| const sizeBytes = statSync(rec.file).size | ||
| // The snapshot exists so an abandoned render can be retried faithfully. Done is terminal, | ||
| // so it has nothing left to serve. | ||
| const snapshot = snapshotPath(rec) | ||
| if (existsSync(snapshot)) unlinkSync(snapshot) | ||
| return this.updateExport( |
Contributor
There was a problem hiding this comment.
6. Completeexport skips status validation 🐞 Bug ≡ Correctness
completeExport renames the uploaded file and marks the export done without ever checking that the record's current status is rendering, so it can finalize a pending or already-failed export if a file happens to exist at the render path. This is a state-integrity gap distinct from the already-fixed late-report protections on setExportProgress/failExport.
Agent Prompt
## Issue description
`completeExport` finalizes an export without checking that its status is currently `rendering`, allowing a pending or failed export to be marked done if a file exists at the expected render path.
## Issue Context
Other terminal-state guards exist (e.g. failExport checks for 'done', setExportProgress checks for 'rendering'), but completeExport has none.
## Fix Focus Areas
- apps/agent/src/project.ts[636-660]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+93
to
+97
| if (busy.current) return | ||
| const job = project.exports?.find((e) => isClaimable(e) && !attempted.has(e.id)) | ||
| if (!job) return | ||
| attempted.add(job.id) | ||
| fireAndForget(run(job)) |
Contributor
There was a problem hiding this comment.
7. Abandoned job never reclaimed by same tab 🐞 Bug ☼ Reliability
The module-level attempted Set in use-render-worker.ts permanently excludes an export ID after a tab attempts it, preventing retries when a lost 409 claim later becomes reclaimable after lease expiry. Because project reset clears exports and ID allocation then reuses IDs starting at exp1, the same blacklist can also suppress newly queued exports until the editor is reloaded or a fresh tab/session handles them.
Agent Prompt
## Issue description
Fix the page-global `attempted` Set so it does not permanently blacklist an export ID after a lost 409 claim or mistake a newly created post-reset export for an earlier attempt when IDs are reused.
## Issue Context
`isClaimable` allows an abandoned `rendering` job to be reclaimed after `RENDER_LEASE_MS`, but the tab that encountered the 409 cannot select it again because its ID remains in `attempted`. Reset also clears the export list, while ID allocation derives the next ID from that current list and can therefore reuse `exp1`; preserve React remount protection without suppressing legitimately reclaimable or newly created jobs, for example by using a non-reusable job identity, keying attempts by immutable creation/claim identity, or removing entries when jobs disappear or become eligible for another attempt.
## Fix Focus Areas
- apps/web/src/components/editor/use-render-worker.ts[14-97]
- apps/agent/src/project.ts[181-185]
- apps/agent/src/project.ts[523-529]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+39
to
+47
| const queueExport = useCallback(() => { | ||
| fireAndForget( | ||
| agentJson("/exports", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ format: "mp4", resolution: "1080p" }), | ||
| }), | ||
| ) | ||
| }, []) |
Contributor
There was a problem hiding this comment.
8. Export button allows duplicate queued renders 🐞 Bug ≡ Correctness
TopBar's Export button is disabled only while the local tab's render state is non-null; it does not check whether an export for this project is already pending or rendering on the agent, and requestExport never deduplicates against an existing unfinished job. Repeated clicks, or a click from a second tab while a render is already queued/in-progress, create multiple duplicate export jobs each consuming a full render.
Agent Prompt
## Issue description
Clicking Export repeatedly, or from multiple tabs, can queue multiple duplicate render jobs because neither the UI nor requestExport checks for an existing pending/rendering export before creating a new one.
## Issue Context
canExport/queueExport in apps/web/src/routes/index.tsx and the Export button's disabled logic in apps/web/src/components/editor/top-bar.tsx only consider the local tab's render state, not project.exports' pending/rendering entries; apps/agent/src/project.ts's requestExport has no dedup check.
## Fix Focus Areas
- apps/web/src/routes/index.tsx[39-47]
- apps/agent/src/project.ts[514-539]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit cf11acc |
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.
The problem
The timeline was a simulation end to end:
intro.mp4,voiceover.wavand the rest were strings inseedProject().find . -name "*.mp4"returned nothing.preview.tsxmapped each filename to a CSS gradient, anduse-playback.tswas arequestAnimationFramecounter. There was no<video>or<canvas>in the codebase.exportProject()didwriteFileSync(rec.file, JSON.stringify({export, project})), writing a JSON blob to a file named.mp4.workspace/. Nothing connected a timeline clip to bytes on disk.The approach
Port the renderer from OpenCut, which EditAI's LICENSE already credits but whose engine had never been brought over: mediabunny over WebCodecs for decode/mux/encode, and OpenCut's frame-cache design.
The one deliberate departure is compositing in Canvas2D rather than OpenCut's wgpu compositor. EditAI stacks video, text and audio with no effects, masks or blend modes, which 2D covers exactly, and it drops the
opencut-wasmdependency along with their tick-based scene graph. Effects would need the real thing, and the README says so.What changed
Import (
engine/media.ts,use-media-import.ts) measures a file with WebCodecs, uploads the bytes to the agent, then analyzes the audio: silences from windowed RMS, a peak envelope for the timeline waveform, tempo from onset autocorrelation.find_silencesanddetect_beatsnow answer from the file. The waveform under an audio clip is its own instead of a seeded PRNG.Preview (
engine/compositor.ts,use-preview.ts) is a canvas driven by the same engine the exporter uses, so what you watch is what gets encoded. Frames come fromengine/video-cache.ts, ported from OpenCut: a forward iterator with the next frame decoded ahead, falling back to a real seek only when the target is behind the decoder or too far in front. A naivegetCanvas(t)per frame re-seeks every time and plays at a few frames a second.Export (
engine/exporter.ts,use-render-worker.ts) is a job. The agent queues it, the editor claims it, composites every frame, muxes with mediabunny, and posts the file back.get_exportthen reports a real path and a real byte count, which is what lets the agent check its own work.Two new tools:
list_media(the agent could not see what had been imported) andadd_clip(it could add text to the timeline but never footage).export_projectnow refuses when a clip's media is missing, rather than writing a file that means nothing.Concurrency, found the hard way
The first end-to-end run had two editor tabs open. Both rendered the same job, both uploaded to the same temp path, one renamed it, the other got
ENOENTand marked a finished render failed. Fixed at the source rather than papered over:POST /exports/:id/claim, 409 if taken), so a page-local guard cannot be defeated by a second tab or a StrictMode remount.setExportProgressandfailExportrefuse to reopen a render that is alreadydone.All three are covered by tests.
Verification
Not just "it typechecks". Against footage generated by the new
scripts/make-samples.ts:done, no stray temp filesTests
58 passing, up from 31. New coverage: media registration and clip-placement bounds, the full render lifecycle including the claim and the late-report guards, silence detection against injected gaps and sub-threshold room tone, the peak envelope, tempo recovery, and which clips reach the audio mix.
Also
apps/agent/data/project.jsonwas both gitignored and committed, so a clone started from whatever timeline was last saved rather than from the seed. Untracked.Known limits, stated in the README
transcribe_clipstill reads stored segments; no ASR yet.🤖 Generated with Claude Code
https://claude.ai/code/session_01D8rspEEzFARv7H1opU9ER2