Web codecs for encoding in the browser - #49
Conversation
📝 WalkthroughWalkthroughAdds WebCodecs H.264 export with configurable bitrate controls, browser capability detection, sequential Annex-B frame uploads, and server-side FFmpeg muxing. Existing JPEG export remains supported, with realtime fallback behavior retained. ChangesWebCodecs export
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.js (1)
350-369: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAwaiting
draincan hang the request (and the whole write chain) if ffmpeg dies.At Lines 360-361 the promise resolves only on
drain. If ffmpeg exits or the pipe errors while the buffer is full,drainnever fires, this POST never responds, and every later/framePOST queues behindsess.writeLockforever — the client sees a frozen progress bar rather than an error. Race the drain against process close/stdin error.Also,
const pat Line 363 shadows the route-levelp(pathname, Line 252); renaming avoids a future TDZ trap.🐛 Proposed fix
const run = async () => { const proc = sess.proc || (sess.mode === "annexb" ? startAnnexbEncoder(sess) : null); if (!proc) throw new Error("export encoder not started"); if (proc.exitCode !== null) throw new Error("ffmpeg exited: " + sess.err()); - if (!proc.stdin.write(body)) - await new Promise((r) => proc.stdin.once("drain", r)); + if (!proc.stdin.write(body)) { + await new Promise((resolve, reject) => { + const onDrain = () => { cleanup(); resolve(); }; + const onDead = () => { cleanup(); reject(new Error("ffmpeg exited: " + sess.err())); }; + const cleanup = () => { + proc.stdin.off("drain", onDrain); + proc.stdin.off("close", onDead); + proc.off("close", onDead); + }; + proc.stdin.once("drain", onDrain); + proc.stdin.once("close", onDead); + proc.once("close", onDead); + }); + } }; - const p = sess.writeLock.then(run, run); - sess.writeLock = p.catch(() => {}); // keep the chain alive after a failed write - await p; + const write = sess.writeLock.then(run, run); + sess.writeLock = write.catch(() => {}); // keep the chain alive after a failed write + await write;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.js` around lines 350 - 369, Update the write handling in the `/api/export/frame` POST route’s `run` function to race the stdin `drain` wait against ffmpeg process termination and stdin errors, rejecting promptly when either failure occurs so `sess.writeLock` does not remain blocked. Also rename the inner `const p` promise to avoid shadowing the route-level pathname variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.js`:
- Around line 5428-5440: Restore shared playback state after export frame
synchronization: in app.js lines 5428-5440, restore
runtime.clipGain.get(c.id).gain.value to the evaluated clip volume after
Promise.all(waits) or when the clip becomes inactive; in app.js lines 5390-5392,
update playAdvanceVideo cleanup() to restore each element’s captured muted and
playbackRate values. Use playAdvanceVideo and the clip synchronization flow as
the implementation anchors.
- Line 240: Align the default project FPS across the server, UI, and
documentation with app.js’s 25 FPS value: update the Untitled Project
export/import defaults in server.js, the displayed FPS in index.html, and the
documented default in CLAUDE.md. Preserve all other project settings and
behavior.
- Around line 5573-5590: Update waitEncodeQueue to observe the encoder’s error
and abort state in addition to renderCancelled and queue drainage, rejecting
immediately when any failure is reported so polling cannot continue
indefinitely. Mirror the existing waitUploadBackpressure behavior and ensure the
call site propagates the encoder/upload error and abort state into
waitEncodeQueue.
In `@CLAUDE.md`:
- Around line 414-421: Remove the unused hasAudio field from the documented
/api/export/begin payload and from the corresponding client request body in
app.js, leaving audio inclusion controlled solely by whether /api/export/audio
populates sess.wav.
In `@server.js`:
- Around line 209-216: Extend the export session lifecycle around cleanupExport
and the frame/audio write handlers: update each session’s lastTouch timestamp on
successful activity, then add a periodic sweep that removes sessions idle beyond
the chosen timeout via cleanupExport. Ensure the sweep is started with the
server and stopped during shutdown, so abandoned sessions release their ffmpeg
process, temp directory, and .part.mp4.
---
Outside diff comments:
In `@server.js`:
- Around line 350-369: Update the write handling in the `/api/export/frame` POST
route’s `run` function to race the stdin `drain` wait against ffmpeg process
termination and stdin errors, rejecting promptly when either failure occurs so
`sess.writeLock` does not remain blocked. Also rename the inner `const p`
promise to avoid shadowing the route-level pathname variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 403c0800-fb04-4b64-9891-044fdb89e415
📒 Files selected for processing (5)
CLAUDE.mdapp.jsindex.htmlserver.jsstyle.css
| const project = { | ||
| name: "Untitled Project", | ||
| width: 1280, height: 720, fps: 30, | ||
| width: 1280, height: 720, fps: 25, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for documented/hard-coded 30fps defaults that may now disagree
rg -n --iglob '!node_modules' -e 'fps' -g '*.md' -g 'index.html' -g 'project.json' | rg -n '30|25'
rg -n '\bfps\b[^\n]*\|\|\s*30' -g '*.js'Repository: ronak-create/FableCut
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "FILES_WITH_fps:"
rg -n --iglob '!node_modules' '\bfps\b' | sed -n '1,220p'
echo
echo "app.js relevant lines:"
sed -n '220,260p' app.js | nl -ba -v220
echo
echo "Search 30 defaults/mentions:"
rg -n --iglob '!node_modules' '\b30\b' | rg -n 'fps|frame|rate|export|video|project' | sed -n '1,160p' || trueRepository: ronak-create/FableCut
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files containing fps:"
rg -n '\bfps\b' . -g '!node_modules' | sed -n '1,260p' || true
echo
echo "Tracked files containing 30fps references:"
rg -n '30fps|30 fps|fps 30|FPS 30|30' . -g '!node_modules' -g '*.js' -g '*.md' -g '*.html' -g '*.json' \
| rg -i 'fps|frame rate|frame rate|export|video|project|animation' \
| sed -n '1,260p' || true
echo
echo "app.js lines 220-260:"
sed -n '220,260p' app.js | nl -ba -v220 || trueRepository: ronak-create/FableCut
Length of output: 8643
Keep the default project fps aligned across client/server/docs.
app.js creates new projects at fps: 25, but server.js still defaults Untitled Project export/import at 30, index.html shows 30fps, and CLAUDE.md documents fps: 30. If 25 is the intended default, update those references too so new behavior doesn’t split by client/server flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.js` at line 240, Align the default project FPS across the server, UI, and
documentation with app.js’s 25 FPS value: update the Untitled Project
export/import defaults in server.js, the displayed FPS in index.html, and the
documented default in CLAUDE.md. Preserve all other project settings and
behavior.
…uard + listener cleanup)
seekVideosTo: zero gain only while syncing; after Promise.all, restore evalProps(c, t).volume; inactive clips also get evaluated volume restored
…/api/export/audio
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLAUDE.md (1)
419-421: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the optional-audio ordering rule.
The client skips
/api/export/audiowhen no WAV is produced, so “annexb — must be after audio” is inaccurate for silent exports. State that Annex-B frames must follow audio when audio is uploaded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 419 - 421, Update the export endpoint documentation near the Annex-B frame description to clarify that Annex-B frames must follow the audio upload only when audio is uploaded. Preserve the existing ordering requirement for exports that include audio and avoid implying audio is mandatory for silent exports.server.js (1)
167-195: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTemp dir is created before fps validation — leaks on every invalid request.
fs.mkdtempSync(line 169) runs before the fps check (171-174). When fps is missing/invalid,beginExportthrows but the just-created temp directory is never removed, and the caller (/api/export/begin, 366-374) doesn't clean it up either. Each bad request leaks an empty dir underos.tmpdir()that the idle-sweep can't reach (session was never registered).🐛 Proposed fix
function beginExport(fps, name, mode) { + const rate = Number(fps); + if (!Number.isFinite(rate) || rate <= 0) { + throw new Error("export fps required (pass project.fps)"); + } const id = Date.now().toString(36) + Math.random().toString(36).slice(2, 7); const dir = fs.mkdtempSync(path.join(os.tmpdir(), "fablecut-")); const m = mode === "annexb" ? "annexb" : "jpeg"; - const rate = Number(fps); - if (!Number.isFinite(rate) || rate <= 0) { - throw new Error("export fps required (pass project.fps)"); - } const sess = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.js` around lines 167 - 195, Move the temporary-directory creation in beginExport until after the fps validation succeeds, so invalid requests throw before fs.mkdtempSync is called. Keep session construction and registration behavior unchanged for valid rates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.js`:
- Around line 619-620: Update the FPS assignment in the project-loading logic to
use the initial default FPS rather than project.fps when data.fps is missing,
non-positive, or invalid. Convert data.fps once, require it to be finite and
greater than zero, and otherwise use the existing initial-default FPS symbol;
preserve valid FPS values.
In `@server.js`:
- Around line 440-442: Update the /api/export/end flow around
sess.proc.stdin.end() to await sess.writeLock before closing stdin, ensuring all
queued or in-flight frame writes and drain waits have completed. Preserve the
existing sess.proc validation and touchExport behavior.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 419-421: Update the export endpoint documentation near the Annex-B
frame description to clarify that Annex-B frames must follow the audio upload
only when audio is uploaded. Preserve the existing ordering requirement for
exports that include audio and avoid implying audio is mandatory for silent
exports.
In `@server.js`:
- Around line 167-195: Move the temporary-directory creation in beginExport
until after the fps validation succeeds, so invalid requests throw before
fs.mkdtempSync is called. Keep session construction and registration behavior
unchanged for valid rates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78466b5b-74ab-4bf5-847c-e70f8f4bdbb8
📒 Files selected for processing (5)
CLAUDE.mdapp.jsindex.htmlruler-worker.jsserver.js
🚧 Files skipped from review as they are similar to previous changes (1)
- index.html
| width: data.width || 1280, height: data.height || 720, | ||
| fps: (Number(data.fps) > 0 ? Number(data.fps) : project.fps), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a stable fallback FPS when loading invalid project data.
Line 620 preserves the prior project's FPS when data.fps is missing or invalid. Loading a legacy project after a 60fps project can therefore silently render/export it at 60fps. It also accepts Infinity. Fall back to the initial default instead, after a finite-number check.
Proposed fix
function applyProject(data) {
const wa = normalizeWorkArea(data.inPoint, data.outPoint);
const disabledTracks = normalizeDisabledTracks(data.disabledTracks);
+ const fps = Number(data.fps);
Object.assign(project, {
name: data.name || "Untitled Project",
width: data.width || 1280, height: data.height || 720,
- fps: (Number(data.fps) > 0 ? Number(data.fps) : project.fps),
+ fps: Number.isFinite(fps) && fps > 0 ? fps : 30,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.js` around lines 619 - 620, Update the FPS assignment in the
project-loading logic to use the initial default FPS rather than project.fps
when data.fps is missing, non-positive, or invalid. Convert data.fps once,
require it to be finite and greater than zero, and otherwise use the existing
initial-default FPS symbol; preserve valid FPS values.
| touchExport(sess); // keep alive through final mux | ||
| if (!sess.proc) throw new Error("no frames were uploaded"); | ||
| sess.proc.stdin.end(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Ending stdin without waiting for the write-lock chain to settle.
/api/export/end calls sess.proc.stdin.end() right after the sess.proc check, without awaiting sess.writeLock. If a /api/export/frame request is still mid-flight (queued behind the lock, or awaiting drain), ending stdin concurrently can truncate the final frame or throw a "write after end" error in that in-flight request. Current client behavior (sequential awaited uploads) avoids triggering this, but it's a cheap, defensive guard against future concurrency changes or client bugs.
🛡️ Proposed fix
touchExport(sess); // keep alive through final mux
if (!sess.proc) throw new Error("no frames were uploaded");
+ await sess.writeLock; // let any in-flight frame write finish before ending stdin
sess.proc.stdin.end();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| touchExport(sess); // keep alive through final mux | |
| if (!sess.proc) throw new Error("no frames were uploaded"); | |
| sess.proc.stdin.end(); | |
| touchExport(sess); // keep alive through final mux | |
| if (!sess.proc) throw new Error("no frames were uploaded"); | |
| await sess.writeLock; // let any in-flight frame write finish before ending stdin | |
| sess.proc.stdin.end(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 440 - 442, Update the /api/export/end flow around
sess.proc.stdin.end() to await sess.writeLock before closing stdin, ensuring all
queued or in-flight frame writes and drain waits have completed. Preserve the
existing sess.proc validation and touchExport behavior.
1 participant
Replace MediaRecorder with WebCodecs for 'realtime' rendering.
WebCodecs are used for encoding final video instead of MediaRecorder (still there as a fallback). Only encoding is done in the browser, muxing is still processed by ffmpeg on the server. As seek() per each frame was used before ~40ms was added to each frame processsing time. This process was optimized as well. Technically, the speed above realtime can be expected.
Type of change
How was it verified?
node --check server.js && node --check app.js && node --check mcp-server.jspassesCLAUDE.md/README.mdif the schema, props, or API changedChecklist
Summary by CodeRabbit
fps/mode behavior and audio/video sequencing.