feat(export): MediaRecorder fallback when WebCodecs encoders are missing#797
Open
avillagran wants to merge 3 commits into
Open
feat(export): MediaRecorder fallback when WebCodecs encoders are missing#797avillagran wants to merge 3 commits into
avillagran wants to merge 3 commits into
Conversation
…llbacks OpenCut's video preview and audio playback both rely on mediabunny, which in turn requires the WebCodecs API (VideoDecoder / AudioDecoder). On browsers where WebCodecs is not exposed (e.g. Linux Chromium builds without proprietary codec support), import and playback both throw "codec not supported" errors and the editor becomes unusable. This change makes the editor work on those browsers without sacrificing the WebCodecs fast-path elsewhere: - Add a server-side /api/convert-video route that transcodes incoming videos to H.264 baseline 3.1 yuv420p via the system ffmpeg binary. processMediaAssets uses it automatically when readVideoFile reports canDecode=false so the asset stored is always something the browser can play. - Add HTMLVideoElementSink, a CanvasSink-shaped sink backed by a hidden <video> element + canvas.drawImage. VideoCache.initializeSink selects it only when WebCodecs VideoDecoder is unavailable; otherwise it keeps using mediabunny's CanvasSink unchanged. - Add FallbackAudioBufferSink, an AudioBufferSink-shaped sink backed by AudioContext.decodeAudioData (which uses the platform's native audio decoders, not WebCodecs). AudioManager and the import-time decode path swap to it on canDecode=false. runClipIterator is wrapped in a try/catch so a sink failure no longer leaves an unhandled rejection. Tests cover frame timing, dispose semantics, FPS clamping, the WebCodecs/AudioDecoder availability probes, AudioBuffer slicing, and the fallback iterator contract.
The existing exporter uses mediabunny, which delegates to the WebCodecs VideoEncoder and AudioEncoder. On browsers where those APIs are not exposed (the same environments that needed the decode fallback in OpenCut-app#794), export currently fails with "AudioEncoder is not supported by this browser" before producing any output. This change adds a parallel fallback path in SceneExporter: - New helpers \`isWebCodecsExportSupported()\` and \`detectMediaRecorderSupport()\` live in a tiny dependency-free module so they can be unit-tested without dragging in WASM. - New \`exportWithMediaRecorder\` drives the existing CanvasRenderer at real-time wall-clock, captures the output canvas with \`HTMLCanvasElement.captureStream(fps)\`, mixes the project audio buffer through a \`MediaStreamAudioDestinationNode\`, and records the combined MediaStream with MediaRecorder, producing a WebM blob the same browser can play back natively. - \`SceneExporter.export()\` detects \`!isWebCodecsExportSupported()\` at entry and delegates to the new path. The WebCodecs path is unchanged for capable browsers. Trade-offs of the fallback path: - Output is always WebM (most browsers don't expose an MP4 muxer via MediaRecorder). The user's requested format is honoured best-effort. - Rendering runs at real time, so a 60-second timeline takes roughly 60 seconds to export. mediabunny's WebCodecs path continues to render faster than real-time on capable browsers. - Bitrate is mapped per quality preset to a numeric value compatible with \`MediaRecorder.videoBitsPerSecond\`. Unit tests cover the feature-detection helpers across the four states of \`VideoEncoder\` / \`AudioEncoder\` / \`MediaRecorder\` availability.
|
@avillagran is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughAdds graceful fallback media paths: server-side ffmpeg H.264 conversion endpoint and client helper, audio decoding fallback via Web Audio API, video frame extraction via HTMLVideoElement+canvas, MediaRecorder-based export fallback, and integrates these into audio/video managers, media processing, and export UI. ChangesWebCodecs Fallback Architecture
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
apps/web/src/media/audio.ts (1)
278-283: ⚡ Quick winExtract decode-capability probing into a shared helper.
This try/catch probe now duplicates the logic in
safeCanDecode; centralizing it avoids drift in fallback behavior.🤖 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 `@apps/web/src/media/audio.ts` around lines 278 - 283, Extract the try/catch probe that sets audioTrackCanDecode (currently using await audioTrack.canDecode() with a catch that sets false) into a shared helper and reuse it instead of duplicating logic; create or use a helper like safeCanDecode (or rename to a more general canDecodeSafely) that encapsulates the await audioTrack.canDecode() call and catch-to-false fallback, then replace the inline block that sets audioTrackCanDecode and any other identical blocks with calls to that helper to ensure consistent fallback behavior.apps/web/src/services/video-cache/html-video-element-sink.ts (1)
129-157: ⚡ Quick winAdd epsilon guard to prevent promise hangs on no-op seeks.
The
seekVideoElementfunction depends on theseekedevent after settingcurrentTime. Per the HTML specification and MDN documentation, browsers do not guarantee firing theseekedevent whencurrentTimeis assigned its existing value. This can cause the promise to hang indefinitely. Guard against no-op seeks by checking if the current playback position already matches the target time before registering event listeners.Proposed fix
function seekVideoElement({ video, time, }: { video: HTMLVideoElement; time: number; }): Promise<void> { return new Promise<void>((resolve, reject) => { + const EPSILON = 1e-4; + if (Math.abs(video.currentTime - time) <= EPSILON) { + resolve(); + return; + } const cleanup = () => { video.removeEventListener("seeked", onSeeked); video.removeEventListener("error", onError); };🤖 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 `@apps/web/src/services/video-cache/html-video-element-sink.ts` around lines 129 - 157, The promise can hang when setting video.currentTime to its existing value because browsers may not fire "seeked"; update seekVideoElement to check current position before attaching listeners: compute a small epsilon (e.g., 0.05s) and if Math.abs(video.currentTime - time) <= epsilon resolve immediately; only add the "seeked" and "error" listeners and attempt assignment when the target time differs beyond the epsilon, and keep existing cleanup/error handling for the real seek case.
🤖 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 `@apps/web/src/app/api/convert-video/route.ts`:
- Line 66: Replace the raw console.error call in the convert-video route (the
console.error("Video conversion error:", error) line) with the project's
structured logger (e.g., logger.error or processLogger.error) and pass the same
message plus the error object as metadata; import the logger used across the
project (or reuse the existing processLogger) at the top of route.ts, remove the
console usage, and keep the message "Video conversion error" while ensuring the
error is supplied as the second argument to the logger call so structured fields
are preserved.
- Around line 29-45: The route currently calls await file.arrayBuffer() which
can OOM for large uploads; add an explicit upload size check before buffering:
define a MAX_UPLOAD_SIZE constant (e.g., in route.ts), check file.size (or
fallback to request.headers.get('content-length')) and if it exceeds the limit
return NextResponse.json({ error: "File too large" }, { status: 413 }); only
proceed to Buffer.from(await file.arrayBuffer()) and writeFile(inputPath, ...)
when the size is within limits. Target symbols: request.formData, the File
object from formData.get("file"), file.arrayBuffer(), and the
inputPath/outputPath handling.
- Around line 85-143: The ffmpeg spawn Promise (the block that creates proc via
spawn("ffmpeg", ...)) lacks a timeout/abort path so a hung ffmpeg will stall
forever; implement a hard timeout by starting a timer (e.g. setTimeout) after
spawning proc that, when fired, kills proc (proc.kill('SIGKILL') or similar) and
rejects the Promise with a descriptive Error, and ensure you clear that timer in
both proc.on("close") and proc.on("error") handlers to avoid leaks; keep using
the existing stderr capture and include the timeout case in the rejection
message so callers can distinguish timeouts from normal ffmpeg failures.
In `@apps/web/src/core/managers/audio-manager.ts`:
- Around line 285-288: Replace the console.warn calls in AudioManager’s clip
iterator fallback paths with the project logger: change the console.warn lines
(the one shown and the other two occurrences around the clip iterator/fallback
logic) to use the AudioManager logger (e.g., this.logger.warn(...) or
logger.warn(...) consistent with how other methods in AudioManager use the
logger) and pass the error object into the logger call; if a logger
instance/import is not present in the file, import or initialize the same logger
used by this class (e.g., getLogger('AudioManager') or the existing logger
symbol) so all three fallback warning sites use the project logging abstraction
instead of console.
In `@apps/web/src/media/audio.ts`:
- Around line 291-293: Replace the direct console.warn call in
apps/web/src/media/audio.ts with the project’s centralized logger (use the same
logger symbol used elsewhere, e.g., logger or processLogger) by importing or
referencing that shared logger and calling logger.warn("[audio] WebCodecs
unavailable; decoding asset via decodeAudioData fallback."); ensure no direct
console usage remains in that module and keep the message text identical when
switching to the shared logger.
In `@apps/web/src/services/renderer/media-recorder-exporter.ts`:
- Around line 109-133: The final 250ms post-render flush doesn't check
cancellation, so if signal.cancelled becomes true after the last frame the
exporter will still stop the recorder and return a buffer; update the
post-render section (after the for loop where startedAt/frame/ticksPerFrame are
used) to re-check signal?.cancelled before calling recorder.stop(): if
cancelled, call recorder.stop(), await recordingFinished.catch(() => null) and
return null (mirroring the in-loop cancellation path), otherwise proceed with
the sleep, recorder.stop(), await recordingFinished.catch(() => null) and
onProgress?.(1).
In `@apps/web/src/services/renderer/scene-exporter.ts`:
- Around line 195-213: The cancellationSignal fallback is only updated inside
onProgress, so make it live: set cancellationSignal.cancelled = this.isCancelled
immediately before calling exportWithMediaRecorder and attach a short-lived
listener that updates cancellationSignal.cancelled whenever this.isCancelled
changes (i.e., subscribe to the same cancellation mechanism your class uses),
then remove the onTickCancel call from the onProgress handler; reference the
cancellationSignal object, the onTickCancel function (remove or repurpose), the
this.isCancelled flag, and the exportWithMediaRecorder call to locate and update
the code.
In `@apps/web/src/services/video-cache/html-video-element-sink.ts`:
- Around line 159-160: In the HTMLVideoElementSink class, replace all
console.warn calls (the three sites currently at lines noted in the review) with
the project's standard logger (e.g., logger.warn or the existing shared logging
instance) so lint/compliance is followed; import or reference the repository
logger used elsewhere in the service, call logger.warn with the same message and
any interpolated variables currently passed to console.warn, and ensure the
logger is used in the same methods inside HTMLVideoElementSink where those
warnings are issued.
In `@apps/web/src/services/video-cache/service.ts`:
- Around line 334-336: Replace the direct console.warn call that logs the
WebCodecs fallback with the project's shared logger (e.g., logger.warn or the
module's existing logger instance) and keep the same message; add the
appropriate import if missing and ensure the log level matches other warnings in
the module (locate the fallback code that emits "[VideoCache] WebCodecs
unavailable; using HTMLVideoElement fallback for video preview." inside the
VideoCache/service initialization and swap console.warn(...) for
logger.warn(...)).
---
Nitpick comments:
In `@apps/web/src/media/audio.ts`:
- Around line 278-283: Extract the try/catch probe that sets audioTrackCanDecode
(currently using await audioTrack.canDecode() with a catch that sets false) into
a shared helper and reuse it instead of duplicating logic; create or use a
helper like safeCanDecode (or rename to a more general canDecodeSafely) that
encapsulates the await audioTrack.canDecode() call and catch-to-false fallback,
then replace the inline block that sets audioTrackCanDecode and any other
identical blocks with calls to that helper to ensure consistent fallback
behavior.
In `@apps/web/src/services/video-cache/html-video-element-sink.ts`:
- Around line 129-157: The promise can hang when setting video.currentTime to
its existing value because browsers may not fire "seeked"; update
seekVideoElement to check current position before attaching listeners: compute a
small epsilon (e.g., 0.05s) and if Math.abs(video.currentTime - time) <= epsilon
resolve immediately; only add the "seeked" and "error" listeners and attempt
assignment when the target time differs beyond the epsilon, and keep existing
cleanup/error handling for the real seek case.
🪄 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
Run ID: c607babe-954e-43bb-988c-ce1b4f7b1760
📒 Files selected for processing (14)
apps/web/src/app/api/convert-video/route.tsapps/web/src/core/managers/audio-manager.tsapps/web/src/media/audio.tsapps/web/src/media/ffmpeg-convert.tsapps/web/src/media/processing.tsapps/web/src/services/audio-cache/__tests__/fallback-audio-buffer-sink.test.tsapps/web/src/services/audio-cache/fallback-audio-buffer-sink.tsapps/web/src/services/renderer/__tests__/media-recorder-exporter.test.tsapps/web/src/services/renderer/media-recorder-exporter.tsapps/web/src/services/renderer/media-recorder-support.tsapps/web/src/services/renderer/scene-exporter.tsapps/web/src/services/video-cache/__tests__/html-video-element-sink.test.tsapps/web/src/services/video-cache/html-video-element-sink.tsapps/web/src/services/video-cache/service.ts
Comment on lines
+29
to
+45
| const formData = await request.formData(); | ||
| const file = formData.get("file") as File | null; | ||
|
|
||
| if (!file || !(file instanceof File)) { | ||
| return NextResponse.json( | ||
| { error: "No file provided" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| workDir = await mkdtemp(path.join(os.tmpdir(), "opencut-convert-")); | ||
| const ext = path.extname(file.name) || ".mp4"; | ||
| const inputPath = path.join(workDir, `input${ext}`); | ||
| const outputPath = path.join(workDir, "output.mp4"); | ||
|
|
||
| const buffer = Buffer.from(await file.arrayBuffer()); | ||
| await writeFile(inputPath, buffer); |
Contributor
There was a problem hiding this comment.
Add an explicit upload size limit before buffering.
await file.arrayBuffer() loads the entire payload into memory. A single oversized upload can exhaust memory and degrade availability. Reject large files early with 413.
🔧 Proposed fix
const GENERIC_CONVERSION_ERROR = "Video conversion failed.";
+const MAX_UPLOAD_BYTES = 250 * 1024 * 1024; // adjust to product limit
export async function POST(request: NextRequest) {
@@
if (!file || !(file instanceof File)) {
@@
}
+
+ if (file.size > MAX_UPLOAD_BYTES) {
+ return NextResponse.json(
+ { error: "File too large" },
+ { status: 413 },
+ );
+ }
workDir = await mkdtemp(path.join(os.tmpdir(), "opencut-convert-"));🤖 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 `@apps/web/src/app/api/convert-video/route.ts` around lines 29 - 45, The route
currently calls await file.arrayBuffer() which can OOM for large uploads; add an
explicit upload size check before buffering: define a MAX_UPLOAD_SIZE constant
(e.g., in route.ts), check file.size (or fallback to
request.headers.get('content-length')) and if it exceeds the limit return
NextResponse.json({ error: "File too large" }, { status: 413 }); only proceed to
Buffer.from(await file.arrayBuffer()) and writeFile(inputPath, ...) when the
size is within limits. Target symbols: request.formData, the File object from
formData.get("file"), file.arrayBuffer(), and the inputPath/outputPath handling.
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Video conversion error:", error); |
Contributor
There was a problem hiding this comment.
Use the project logger instead of console.error.
Switch this to your structured logger to satisfy linting and keep observability consistent.
As per coding guidelines, "Don't use console".
🤖 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 `@apps/web/src/app/api/convert-video/route.ts` at line 66, Replace the raw
console.error call in the convert-video route (the console.error("Video
conversion error:", error) line) with the project's structured logger (e.g.,
logger.error or processLogger.error) and pass the same message plus the error
object as metadata; import the logger used across the project (or reuse the
existing processLogger) at the top of route.ts, remove the console usage, and
keep the message "Video conversion error" while ensuring the error is supplied
as the second argument to the logger call so structured fields are preserved.
Comment on lines
+85
to
+143
| return new Promise((resolve, reject) => { | ||
| const proc = spawn("ffmpeg", [ | ||
| "-i", | ||
| inputPath, | ||
| "-c:v", | ||
| "libx264", | ||
| "-profile:v", | ||
| "baseline", | ||
| "-level", | ||
| "3.1", | ||
| "-preset", | ||
| "veryfast", | ||
| "-crf", | ||
| "23", | ||
| "-pix_fmt", | ||
| "yuv420p", | ||
| "-g", | ||
| "60", | ||
| "-keyint_min", | ||
| "60", | ||
| "-sc_threshold", | ||
| "0", | ||
| "-movflags", | ||
| "+faststart", | ||
| "-tag:v", | ||
| "avc1", | ||
| "-c:a", | ||
| "aac", | ||
| "-ac", | ||
| "2", | ||
| "-ar", | ||
| "48000", | ||
| "-b:a", | ||
| "128k", | ||
| "-y", | ||
| outputPath, | ||
| ]); | ||
|
|
||
| let stderr = ""; | ||
| proc.stderr.on("data", (data) => { | ||
| stderr += data.toString(); | ||
| }); | ||
|
|
||
| proc.on("close", (code) => { | ||
| if (code === 0) { | ||
| resolve(); | ||
| } else { | ||
| reject( | ||
| new Error( | ||
| `ffmpeg exited with code ${code}. stderr: ${stderr.slice(-500)}`, | ||
| ), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| proc.on("error", (err) => { | ||
| reject(err); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "route.ts" --path "*convert-video*"Repository: OpenCut-app/OpenCut
Length of output: 297
🏁 Script executed:
cat -n apps/web/src/app/api/convert-video/route.ts | head -160Repository: OpenCut-app/OpenCut
Length of output: 4170
Enforce a timeout/abort path for ffmpeg execution.
If ffmpeg hangs, this request will stall indefinitely with no cleanup. Add a hard timeout that kills the process and rejects.
🔧 Proposed fix
function runFfmpeg({
inputPath,
outputPath,
}: {
inputPath: string;
outputPath: string;
}): Promise<void> {
return new Promise((resolve, reject) => {
+ const FFMPEG_TIMEOUT_MS = 120_000;
const proc = spawn("ffmpeg", [
@@
outputPath,
]);
+ const timeout = setTimeout(() => {
+ proc.kill("SIGKILL");
+ reject(new Error(`ffmpeg timed out after ${FFMPEG_TIMEOUT_MS}ms`));
+ }, FFMPEG_TIMEOUT_MS);
let stderr = "";
proc.stderr.on("data", (data) => {
stderr += data.toString();
});
proc.on("close", (code) => {
+ clearTimeout(timeout);
if (code === 0) {
resolve();
} else {
@@
});
proc.on("error", (err) => {
+ clearTimeout(timeout);
reject(err);
});
});
}📝 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.
Suggested change
| return new Promise((resolve, reject) => { | |
| const proc = spawn("ffmpeg", [ | |
| "-i", | |
| inputPath, | |
| "-c:v", | |
| "libx264", | |
| "-profile:v", | |
| "baseline", | |
| "-level", | |
| "3.1", | |
| "-preset", | |
| "veryfast", | |
| "-crf", | |
| "23", | |
| "-pix_fmt", | |
| "yuv420p", | |
| "-g", | |
| "60", | |
| "-keyint_min", | |
| "60", | |
| "-sc_threshold", | |
| "0", | |
| "-movflags", | |
| "+faststart", | |
| "-tag:v", | |
| "avc1", | |
| "-c:a", | |
| "aac", | |
| "-ac", | |
| "2", | |
| "-ar", | |
| "48000", | |
| "-b:a", | |
| "128k", | |
| "-y", | |
| outputPath, | |
| ]); | |
| let stderr = ""; | |
| proc.stderr.on("data", (data) => { | |
| stderr += data.toString(); | |
| }); | |
| proc.on("close", (code) => { | |
| if (code === 0) { | |
| resolve(); | |
| } else { | |
| reject( | |
| new Error( | |
| `ffmpeg exited with code ${code}. stderr: ${stderr.slice(-500)}`, | |
| ), | |
| ); | |
| } | |
| }); | |
| proc.on("error", (err) => { | |
| reject(err); | |
| }); | |
| }); | |
| return new Promise((resolve, reject) => { | |
| const FFMPEG_TIMEOUT_MS = 120_000; | |
| const proc = spawn("ffmpeg", [ | |
| "-i", | |
| inputPath, | |
| "-c:v", | |
| "libx264", | |
| "-profile:v", | |
| "baseline", | |
| "-level", | |
| "3.1", | |
| "-preset", | |
| "veryfast", | |
| "-crf", | |
| "23", | |
| "-pix_fmt", | |
| "yuv420p", | |
| "-g", | |
| "60", | |
| "-keyint_min", | |
| "60", | |
| "-sc_threshold", | |
| "0", | |
| "-movflags", | |
| "+faststart", | |
| "-tag:v", | |
| "avc1", | |
| "-c:a", | |
| "aac", | |
| "-ac", | |
| "2", | |
| "-ar", | |
| "48000", | |
| "-b:a", | |
| "128k", | |
| "-y", | |
| outputPath, | |
| ]); | |
| const timeout = setTimeout(() => { | |
| proc.kill("SIGKILL"); | |
| reject(new Error(`ffmpeg timed out after ${FFMPEG_TIMEOUT_MS}ms`)); | |
| }, FFMPEG_TIMEOUT_MS); | |
| let stderr = ""; | |
| proc.stderr.on("data", (data) => { | |
| stderr += data.toString(); | |
| }); | |
| proc.on("close", (code) => { | |
| clearTimeout(timeout); | |
| if (code === 0) { | |
| resolve(); | |
| } else { | |
| reject( | |
| new Error( | |
| `ffmpeg exited with code ${code}. stderr: ${stderr.slice(-500)}`, | |
| ), | |
| ); | |
| } | |
| }); | |
| proc.on("error", (err) => { | |
| clearTimeout(timeout); | |
| reject(err); | |
| }); | |
| }); |
🤖 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 `@apps/web/src/app/api/convert-video/route.ts` around lines 85 - 143, The
ffmpeg spawn Promise (the block that creates proc via spawn("ffmpeg", ...))
lacks a timeout/abort path so a hung ffmpeg will stall forever; implement a hard
timeout by starting a timer (e.g. setTimeout) after spawning proc that, when
fired, kills proc (proc.kill('SIGKILL') or similar) and rejects the Promise with
a descriptive Error, and ensure you clear that timer in both proc.on("close")
and proc.on("error") handlers to avoid leaks; keep using the existing stderr
capture and include the timeout case in the rejection message so callers can
distinguish timeouts from normal ffmpeg failures.
Comment on lines
+285
to
+288
| console.warn( | ||
| `[AudioManager] clip iterator failed for ${clip.id}; aborting playback for this clip.`, | ||
| error, | ||
| ); |
Contributor
There was a problem hiding this comment.
Use the project logger instead of console.warn in new fallback paths.
Please route these warnings through the existing logging/telemetry abstraction to keep lint/compliance and runtime logging behavior consistent.
As per coding guidelines, "Don't use console".
Also applies to: 655-657, 755-757
🤖 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 `@apps/web/src/core/managers/audio-manager.ts` around lines 285 - 288, Replace
the console.warn calls in AudioManager’s clip iterator fallback paths with the
project logger: change the console.warn lines (the one shown and the other two
occurrences around the clip iterator/fallback logic) to use the AudioManager
logger (e.g., this.logger.warn(...) or logger.warn(...) consistent with how
other methods in AudioManager use the logger) and pass the error object into the
logger call; if a logger instance/import is not present in the file, import or
initialize the same logger used by this class (e.g., getLogger('AudioManager')
or the existing logger symbol) so all three fallback warning sites use the
project logging abstraction instead of console.
Comment on lines
+291
to
+293
| console.warn( | ||
| "[audio] WebCodecs unavailable; decoding asset via decodeAudioData fallback.", | ||
| ); |
Contributor
There was a problem hiding this comment.
Replace new console.warn fallback log with the shared logger.
Use the same centralized logger path used elsewhere instead of direct console output.
As per coding guidelines, "Don't use console".
🤖 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 `@apps/web/src/media/audio.ts` around lines 291 - 293, Replace the direct
console.warn call in apps/web/src/media/audio.ts with the project’s centralized
logger (use the same logger symbol used elsewhere, e.g., logger or
processLogger) by importing or referencing that shared logger and calling
logger.warn("[audio] WebCodecs unavailable; decoding asset via decodeAudioData
fallback."); ensure no direct console usage remains in that module and keep the
message text identical when switching to the shared logger.
Comment on lines
+109
to
+133
| const startedAt = performance.now(); | ||
| for (let frame = 0; frame < frameCount; frame++) { | ||
| if (signal?.cancelled) { | ||
| recorder.stop(); | ||
| await recordingFinished.catch(() => null); | ||
| return null; | ||
| } | ||
|
|
||
| const targetWallTimeMs = startedAt + (frame * 1000) / fpsFloat; | ||
| const waitMs = targetWallTimeMs - performance.now(); | ||
| if (waitMs > 0) { | ||
| await sleep({ ms: waitMs }); | ||
| } | ||
|
|
||
| const timeTicks = frame * ticksPerFrame; | ||
| await renderer.render({ node: rootNode, time: timeTicks }); | ||
|
|
||
| onProgress?.(frame / frameCount); | ||
| } | ||
|
|
||
| // Give the recorder a moment to flush the last frames after rendering ends. | ||
| await sleep({ ms: 250 }); | ||
| recorder.stop(); | ||
| onProgress?.(1); | ||
|
|
Contributor
There was a problem hiding this comment.
Handle cancellation during post-render recorder flush.
Cancellation is only checked inside the frame loop. If cancellation happens after the last frame (during the 250ms flush), export can still finalize and return a buffer.
Suggested fix
for (let frame = 0; frame < frameCount; frame++) {
if (signal?.cancelled) {
recorder.stop();
await recordingFinished.catch(() => null);
return null;
}
@@
onProgress?.(frame / frameCount);
}
+ if (signal?.cancelled) {
+ recorder.stop();
+ await recordingFinished.catch(() => null);
+ return null;
+ }
+
// Give the recorder a moment to flush the last frames after rendering ends.
await sleep({ ms: 250 });
+ if (signal?.cancelled) {
+ recorder.stop();
+ await recordingFinished.catch(() => null);
+ return null;
+ }
recorder.stop();
onProgress?.(1);📝 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.
Suggested change
| const startedAt = performance.now(); | |
| for (let frame = 0; frame < frameCount; frame++) { | |
| if (signal?.cancelled) { | |
| recorder.stop(); | |
| await recordingFinished.catch(() => null); | |
| return null; | |
| } | |
| const targetWallTimeMs = startedAt + (frame * 1000) / fpsFloat; | |
| const waitMs = targetWallTimeMs - performance.now(); | |
| if (waitMs > 0) { | |
| await sleep({ ms: waitMs }); | |
| } | |
| const timeTicks = frame * ticksPerFrame; | |
| await renderer.render({ node: rootNode, time: timeTicks }); | |
| onProgress?.(frame / frameCount); | |
| } | |
| // Give the recorder a moment to flush the last frames after rendering ends. | |
| await sleep({ ms: 250 }); | |
| recorder.stop(); | |
| onProgress?.(1); | |
| const startedAt = performance.now(); | |
| for (let frame = 0; frame < frameCount; frame++) { | |
| if (signal?.cancelled) { | |
| recorder.stop(); | |
| await recordingFinished.catch(() => null); | |
| return null; | |
| } | |
| const targetWallTimeMs = startedAt + (frame * 1000) / fpsFloat; | |
| const waitMs = targetWallTimeMs - performance.now(); | |
| if (waitMs > 0) { | |
| await sleep({ ms: waitMs }); | |
| } | |
| const timeTicks = frame * ticksPerFrame; | |
| await renderer.render({ node: rootNode, time: timeTicks }); | |
| onProgress?.(frame / frameCount); | |
| } | |
| if (signal?.cancelled) { | |
| recorder.stop(); | |
| await recordingFinished.catch(() => null); | |
| return null; | |
| } | |
| // Give the recorder a moment to flush the last frames after rendering ends. | |
| await sleep({ ms: 250 }); | |
| if (signal?.cancelled) { | |
| recorder.stop(); | |
| await recordingFinished.catch(() => null); | |
| return null; | |
| } | |
| recorder.stop(); | |
| onProgress?.(1); | |
🤖 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 `@apps/web/src/services/renderer/media-recorder-exporter.ts` around lines 109 -
133, The final 250ms post-render flush doesn't check cancellation, so if
signal.cancelled becomes true after the last frame the exporter will still stop
the recorder and return a buffer; update the post-render section (after the for
loop where startedAt/frame/ticksPerFrame are used) to re-check signal?.cancelled
before calling recorder.stop(): if cancelled, call recorder.stop(), await
recordingFinished.catch(() => null) and return null (mirroring the in-loop
cancellation path), otherwise proceed with the sleep, recorder.stop(), await
recordingFinished.catch(() => null) and onProgress?.(1).
Comment on lines
+195
to
+213
| const cancellationSignal = { cancelled: false }; | ||
| const onTickCancel = () => { | ||
| if (this.isCancelled) { | ||
| cancellationSignal.cancelled = true; | ||
| } | ||
| }; | ||
|
|
||
| const buffer = await exportWithMediaRecorder({ | ||
| renderer: this.renderer, | ||
| rootNode, | ||
| fps: this.renderer.fps, | ||
| bitrate: mediaRecorderBitrateMap[this.quality], | ||
| audioBuffer: this.shouldIncludeAudio ? this.audioBuffer : null, | ||
| onProgress: (progress) => { | ||
| onTickCancel(); | ||
| this.emit("progress", progress); | ||
| }, | ||
| signal: cancellationSignal, | ||
| }); |
Contributor
There was a problem hiding this comment.
Make fallback cancellation signal live instead of progress-driven.
cancellationSignal.cancelled is currently flipped only inside onProgress, so cancel state propagation is delayed and coupled to progress emission.
Suggested fix
- const cancellationSignal = { cancelled: false };
- const onTickCancel = () => {
- if (this.isCancelled) {
- cancellationSignal.cancelled = true;
- }
- };
+ const self = this;
+ const cancellationSignal = {
+ get cancelled() {
+ return self.isCancelled;
+ },
+ };
@@
audioBuffer: this.shouldIncludeAudio ? this.audioBuffer : null,
onProgress: (progress) => {
- onTickCancel();
this.emit("progress", progress);
},
signal: cancellationSignal,
});🤖 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 `@apps/web/src/services/renderer/scene-exporter.ts` around lines 195 - 213, The
cancellationSignal fallback is only updated inside onProgress, so make it live:
set cancellationSignal.cancelled = this.isCancelled immediately before calling
exportWithMediaRecorder and attach a short-lived listener that updates
cancellationSignal.cancelled whenever this.isCancelled changes (i.e., subscribe
to the same cancellation mechanism your class uses), then remove the
onTickCancel call from the onProgress handler; reference the cancellationSignal
object, the onTickCancel function (remove or repurpose), the this.isCancelled
flag, and the exportWithMediaRecorder call to locate and update the code.
Comment on lines
+159
to
+160
|
|
||
| export class HTMLVideoElementSink implements FrameSink { |
Contributor
There was a problem hiding this comment.
Replace console warnings with the project logging path.
Line 159, Line 204, and Line 215 use console.warn. Please route these through the repo’s standard logger to keep lint/compliance consistent.
As per coding guidelines, "Don't use console".
Also applies to: 204-217
🤖 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 `@apps/web/src/services/video-cache/html-video-element-sink.ts` around lines
159 - 160, In the HTMLVideoElementSink class, replace all console.warn calls
(the three sites currently at lines noted in the review) with the project's
standard logger (e.g., logger.warn or the existing shared logging instance) so
lint/compliance is followed; import or reference the repository logger used
elsewhere in the service, call logger.warn with the same message and any
interpolated variables currently passed to console.warn, and ensure the logger
is used in the same methods inside HTMLVideoElementSink where those warnings are
issued.
Comment on lines
+334
to
+336
| console.warn( | ||
| "[VideoCache] WebCodecs unavailable; using HTMLVideoElement fallback for video preview.", | ||
| ); |
Contributor
There was a problem hiding this comment.
Use the shared logger instead of console.warn in fallback initialization.
Line 334 introduces a direct console call. Please switch this to the project logging utility for consistency and lint compliance.
As per coding guidelines, "Don't use console".
🤖 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 `@apps/web/src/services/video-cache/service.ts` around lines 334 - 336, Replace
the direct console.warn call that logs the WebCodecs fallback with the project's
shared logger (e.g., logger.warn or the module's existing logger instance) and
keep the same message; add the appropriate import if missing and ensure the log
level matches other warnings in the module (locate the fallback code that emits
"[VideoCache] WebCodecs unavailable; using HTMLVideoElement fallback for video
preview." inside the VideoCache/service initialization and swap
console.warn(...) for logger.warn(...)).
The MediaRecorder export fallback introduced in OpenCut-app#797 always emits a WebM container (most browsers don't expose an MP4 muxer through MediaRecorder). WebM plays in Chrome / Firefox / VLC but not natively in QuickTime or Safari, so users on browsers without WebCodecs who picked the MP4 format end up with a file they can't open in those players. This adds an opt-in escape hatch in the export popover: - When `isWebCodecsExportSupported()` is false AND the user picked MP4, the popover surfaces a "Browser compatibility" section that explains the situation and offers a "Transcode to H.264 MP4 on the server" checkbox (defaulted to on, since the user did ask for MP4). - When the checkbox is on, the post-export flow wraps the produced WebM buffer in a `File` and runs it through `convertVideoToH264`, which already routes to the existing `/api/convert-video` ffmpeg endpoint. The downloaded file is a real H.264 MP4. - If the user opts out, the popover keeps the WebM extension and mime type so the saved file matches its real container. - If the server transcode fails, the export is preserved as WebM and the user is notified via a toast. A small "Transcoding to MP4 on server…" section is shown in the popover between the export finishing and the download starting, since that step can take a noticeable amount of time on large clips. The WebCodecs-capable code path is unchanged: the warning section is only rendered when `!isWebCodecsExportSupported() && format === "mp4"`.
Author
|
Follow-up commit (6f7dacb): added an opt-in server transcode option in the export popover, addressing the practical follow-up that the WebM fallback output doesn't play in QuickTime/Safari. When WebCodecs is missing and the user picked MP4, the popover surfaces a 'Browser compatibility' section with a checkbox to route the produced WebM through the existing |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/web/src/components/editor/export-button.tsx`:
- Around line 149-176: The export flow can be re-entered while a server
transcode is active; add an in-flight guard to prevent overlapping exports by
checking a dedicated boolean (e.g., isTranscoding or new isExporting) at the
start of handleExport and returning early if set, and set that flag (use
setIsTranscoding or setIsExporting) before starting the transcode/upload and
clear it in finally; also disable export UI controls while the flag is true so
the user cannot trigger another export mid-transcode—update references to
shouldOfferServerTranscode and transcodeToMp4OnServer logic to respect this
guard.
🪄 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
Run ID: e7b38cd2-368d-4866-81bd-382ff525a3ee
📒 Files selected for processing (1)
apps/web/src/components/editor/export-button.tsx
Comment on lines
+149
to
+176
| let outBuffer = result.buffer; | ||
| let outFormat: ExportFormat = format; | ||
|
|
||
| if (shouldOfferServerTranscode) { | ||
| if (transcodeToMp4OnServer) { | ||
| setIsTranscoding(true); | ||
| try { | ||
| const webmFile = new File([outBuffer], "export.webm", { | ||
| type: "video/webm", | ||
| }); | ||
| const mp4File = await convertVideoToH264({ file: webmFile }); | ||
| outBuffer = await mp4File.arrayBuffer(); | ||
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : "Unknown error"; | ||
| toast.error("Server transcode failed", { | ||
| description: `${message}. Saving WebM instead.`, | ||
| }); | ||
| outFormat = "webm"; | ||
| } finally { | ||
| setIsTranscoding(false); | ||
| } | ||
| } else { | ||
| // User opted out of server transcode: keep WebM extension/mime so | ||
| // the file matches its actual container. | ||
| outFormat = "webm"; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Block re-entrant exports while server transcode is running.
handleExport can be triggered again during isTranscoding, which can start overlapping export/transcode jobs and cause duplicate uploads/downloads. Add an in-flight guard and lock the controls while transcoding.
Suggested fix
const handleExport = async () => {
- if (!activeProject) return;
+ if (!activeProject || isExporting || isTranscoding) return;
const result = await editor.project.export({
@@
- {!isExporting && (
+ {!isExporting && !isTranscoding && (
@@
- <Button onClick={handleExport} className="w-full gap-2">
+ <Button onClick={handleExport} className="w-full gap-2" disabled={isTranscoding}>
<Download className="size-4" />
Export
</Button>🤖 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 `@apps/web/src/components/editor/export-button.tsx` around lines 149 - 176, The
export flow can be re-entered while a server transcode is active; add an
in-flight guard to prevent overlapping exports by checking a dedicated boolean
(e.g., isTranscoding or new isExporting) at the start of handleExport and
returning early if set, and set that flag (use setIsTranscoding or
setIsExporting) before starting the transcode/upload and clear it in finally;
also disable export UI controls while the flag is true so the user cannot
trigger another export mid-transcode—update references to
shouldOfferServerTranscode and transcodeToMp4OnServer logic to respect this
guard.
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.
Summary
Companion to #794. The existing exporter goes through mediabunny, which delegates to the WebCodecs
VideoEncoderandAudioEncoder. On browsers where those APIs are not exposed (the same environments that needed the decode fallback in #794), export currently fails withAudioEncoder is not supported by this browserbefore producing any output.This adds a parallel fallback path inside
SceneExporterso export works in those environments too.How
isWebCodecsExportSupported()anddetectMediaRecorderSupport()live in a tiny dependency-free module (media-recorder-support.ts) so they can be unit-tested without dragging in the project's WASM glue.exportWithMediaRecorderdrives the existingCanvasRendererat real-time wall-clock, captures the output canvas withHTMLCanvasElement.captureStream(fps), mixes the project audio buffer through aMediaStreamAudioDestinationNode, and records the combinedMediaStreamwithMediaRecorder— producing a WebM blob the same browser can play back natively.SceneExporter.export()checks!isWebCodecsExportSupported()at entry and delegates to the new path. The WebCodecs path is unchanged for capable browsers.Trade-offs
ExportQualitypreset (2.5 / 5 / 10 / 20 Mbps for low / medium / high / very_high) intoMediaRecorder.videoBitsPerSecond.Test plan
VideoEncoder/AudioEncoder: export now produces a working WebM file with audio. On a capable browser, the existing mediabunny path runs as before.Notes
mainonly contains the export-fallback additions; merging order should be feat(media): support browsers without WebCodecs via FFmpeg + HTML5 fallbacks #794 first, this on top.Summary by CodeRabbit
New Features
Improvements
Tests