Skip to content

feat(media): support browsers without WebCodecs via FFmpeg + HTML5 fallbacks#794

Open
avillagran wants to merge 1 commit into
OpenCut-app:mainfrom
avillagran:feat/no-webcodecs-fallback
Open

feat(media): support browsers without WebCodecs via FFmpeg + HTML5 fallbacks#794
avillagran wants to merge 1 commit into
OpenCut-app:mainfrom
avillagran:feat/no-webcodecs-fallback

Conversation

@avillagran
Copy link
Copy Markdown

@avillagran avillagran commented May 16, 2026

Summary

OpenCut's video preview and audio playback both rely on mediabunny, which requires the WebCodecs API (VideoDecoder / AudioDecoder). On browsers where WebCodecs is not exposed (e.g. some Linux Chromium builds without proprietary codec support, Firefox in certain configurations), import and playback both throw codec not supported errors and the editor becomes unusable.

This PR makes the editor work on those browsers without changing the WebCodecs fast-path when it is available.

What changed

  • /api/convert-video — server-side route that transcodes incoming videos to H.264 baseline 3.1 + yuv420p via the system ffmpeg binary. processMediaAssets calls it automatically when readVideoFile reports canDecode=false, so the asset persisted to storage is always something the browser can play.
  • HTMLVideoElementSink — a CanvasSink-shaped sink backed by a hidden <video> element and canvas.drawImage. VideoCache.initializeSink selects it only when VideoDecoder is undefined; otherwise it keeps using mediabunny's CanvasSink unchanged.
  • FallbackAudioBufferSink — an AudioBufferSink-shaped sink backed by AudioContext.decodeAudioData, which uses the platform's native audio decoders rather than WebCodecs. AudioManager and the import-time decode path swap to it when audioTrack.canDecode() returns false. runClipIterator is wrapped in a try/catch so a sink failure no longer leaves an unhandled rejection.

Test plan

  • Unit tests (`bun test`) for FPS clamping, frame timing, dispose semantics, WebCodecs/AudioDecoder availability probes, AudioBuffer slicing, and the fallback iterator contract — 19 tests passing.
  • Manual verification on a browser without WebCodecs: import an H.264 AVC mp4, scrub the timeline, play with audio. Video frames render and audio is audible. Confirmed via console:
    • `[VideoCache] WebCodecs unavailable; using HTMLVideoElement fallback for video preview.`
    • `[AudioManager] WebCodecs unavailable; using decodeAudioData fallback for audio clip.`
  • On a WebCodecs-capable browser, behavior is unchanged (no fallback warnings, mediabunny's CanvasSink / AudioBufferSink remain in use).

Notes

  • The fallback sinks duck-type mediabunny's sink interfaces (`canvases(start, end)`, `buffers(start, end)`) so consumers can swap implementations transparently.
  • Server-side ffmpeg conversion currently runs synchronously per upload; future improvement could stream the response or report progress.
  • The fallback path decodes the entire audio file upfront via `decodeAudioData`. For very large clips this trades memory for the ability to play at all on browsers that otherwise can't.

Summary by CodeRabbit

  • New Features

    • On-demand video conversion endpoint producing H.264/MP4 downloads.
    • Client-side video conversion integration for undecodable codecs.
    • HTML-video–based frame-extraction fallback for broader browser support.
  • Enhancements

    • WebCodecs-aware audio decoding with a robust fallback path.
    • Improved media processing flow with safer error handling, resource cleanup, and progress reporting.
  • Tests

    • New unit tests covering audio fallback, video fallback, slicing, and frame extraction behaviors.

Review Change Stack

@vercel
Copy link
Copy Markdown

vercel Bot commented May 16, 2026

@avillagran is attempting to deploy a commit to the OpenCut OSS Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

📝 Walkthrough

Walkthrough

Adds server-side ffmpeg conversion API and client helper, implements fallback audio/video decoding sinks (with tests) for environments lacking WebCodecs, and integrates these fallbacks into audio manager, media processing, and the video cache service.

Changes

Video Codec Conversion and WebCodecs Fallback Support

Layer / File(s) Summary
Server-side ffmpeg video conversion API
apps/web/src/app/api/convert-video/route.ts
POST endpoint accepts multipart file, writes temp input, runs ffmpeg to convert to H.264/AAC MP4, returns video/mp4 attachment with sanitized filename, and attempts temp-dir cleanup.
Fallback audio decoding sink implementation
apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts
Adds AudioBufferSinkLike, isWebAudioDecoderAvailable, FallbackAudioBufferSink (decodes via AudioContext), decodeArrayBuffer, and sliceAudioBuffer.
Fallback audio decoding tests
apps/web/src/services/audio-cache/__tests__/fallback-audio-buffer-sink.test.ts
Tests availability detection, sliceAudioBuffer slicing/clamping, and FallbackAudioBufferSink.buffers() semantics and metadata getters.
AudioManager and media/audio integration
apps/web/src/core/managers/audio-manager.ts, apps/web/src/media/audio.ts
Switches sinks to AudioBufferSinkLike, probes canDecode() safely via safeCanDecode, selects WebCodecs or fallback sink, updates getAudioSink/decodeClipBuffer, and adds iterator error handling.
Client-side video conversion wrapper
apps/web/src/media/ffmpeg-convert.ts
convertVideoToH264 POSTs a File to /api/convert-video, surfaces JSON/status errors, and returns a .mp4 File.
Media processing video conversion integration
apps/web/src/media/processing.ts
Imports convertVideoToH264, converts undecodable videos to H.264 MP4, manages temporary URLs and storage checks, re-reads metadata, emits toasts, and pushes processed video assets.
Fallback video decoding sink implementation
apps/web/src/services/video-cache/html-video-element-sink.ts
Adds FrameSink and VideoElementHandle interfaces, createHTMLVideoElementHandle, HTMLVideoElementSink producing canvas frames with FPS clamping, and helpers isWebCodecsAvailable/clampFallbackFps.
Fallback video decoding tests
apps/web/src/services/video-cache/__tests__/html-video-element-sink.test.ts
Tests FPS clamping, WebCodecs detection, async frame iteration/timestamps, seek error handling, disposal, and canvas sizing.
Video cache service fallback integration
apps/web/src/services/video-cache/service.ts
Stores generic FrameSink + dispose() in cache, initializes HTMLVideoElementSink fallback when WebCodecs unavailable, and updates cleanup to call dispose().

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant API as POST /api/convert-video
  participant FFMPEG as ffmpeg
  participant FS as Filesystem

  User->>API: POST video file
  API->>FS: write temp input
  API->>FFMPEG: spawn conversion
  FFMPEG->>FS: write output MP4
  API->>FS: read result
  API->>User: return MP4 blob
  API->>FS: cleanup temps
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through bytes and codecs tight,
Poked ffmpeg to set video right,
When WebCodecs hides, I softly fall back,
With audio buffers and canvas on track,
A joyful hop—now every file takes flight.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The repository's PR template explicitly states 'We are not currently accepting PRs except for critical bugs' and requires issue approval first. The PR description does not acknowledge or address this policy. Address the PR template requirements: either confirm this is a critical bug with prior issue approval, or open an issue for discussion as instructed by the template.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding fallback support for browsers without WebCodecs API using FFmpeg and HTML5 technologies.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🤖 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`:
- Around line 45-49: The catch handlers in route.ts currently return
error.message directly (leaking ffmpeg stderr and file paths); change both catch
blocks around the runFfmpeg usage to keep detailed diagnostics server-side by
logging the full error (e.g., console.error("Video conversion error:", error))
but always return a generic client-facing NextResponse.json({ error: "Conversion
failed" }, { status: 500 }) or similar string, never error.message; locate the
handlers around runFfmpeg and the second block noted (the block at ~114-117) and
update them accordingly.
- Around line 22-26: The temp file naming using Date.now() in route.ts
(inputPath/outputPath) is not collision-safe; change to create a unique temp
directory or filename per request (e.g. use fs.mkdtemp() under os.tmpdir() or
append a crypto.randomUUID() or similar secure random token to the filename) and
derive inputPath/outputPath from that unique slot so concurrent uploads cannot
collide; update any cleanup logic that references those paths to use the new
unique dir/paths (references: inputPath, outputPath, file.name, os.tmpdir()).
- Around line 7-12: The ffmpeg child process must be tied to request
cancellation and a timeout: modify POST to create an AbortController with a
configurable timeout, pass a composed signal (e.g., race of request.signal and
the controller.signal) into runFfmpeg, and ensure runFfmpeg is invoked with that
signal so it can abort the child; on abort (either client disconnect or timeout)
kill the ffmpeg child process and clean up temp files, and catch AbortError in
POST to return an appropriate aborted response. Ensure references: POST,
runFfmpeg, request.signal, AbortController, and the ffmpeg child-killing logic
are updated so the ffmpeg process does not outlive the request or timeout.

In `@apps/web/src/core/managers/audio-manager.ts`:
- Around line 285-288: Replace the direct console.warn(...) calls in
AudioManager where the clip iterator error is logged (the message containing
"clip iterator failed for ${clip.id}; aborting playback for this clip.") with
the project's centralized logger/telemetry API (e.g., use the existing
logger/telemetry instance used elsewhere in audio-manager) and pass the error
object as structured context so the message and error details are recorded
consistently; ensure you use the same logging level that matches warn/exception
semantics and follow the project's telemetry formatting conventions when
invoking the logger.

In `@apps/web/src/media/audio.ts`:
- Around line 291-293: Replace the direct console.warn call that logs "[audio]
WebCodecs unavailable; decoding asset via decodeAudioData fallback." with the
project's shared logger (e.g., logger.warn or projectLogger.warn) so the message
goes through the centralized logging system; ensure the shared logger is
imported into apps/web/src/media/audio.ts and use the same message and
formatting when calling logger.warn in place of console.warn.

In `@apps/web/src/media/ffmpeg-convert.ts`:
- Around line 1-6: The exported function convertVideoToH264 currently declares
an onProgress parameter but never invokes it; remove the unused onProgress
parameter from the function signature and any related type annotations to match
the actual behavior, and update all callers to stop passing onProgress (or
implement progress reporting later and add it back consistently). Locate
convertVideoToH264 in ffmpeg-convert.ts, delete the onProgress property from the
parameter destructuring and the type annotation { onProgress?: (progress:
number) => void }, and run a global search for calls that pass onProgress to
update them accordingly.

In `@apps/web/src/media/processing.ts`:
- Around line 141-150: The conversion branch for handling non-decodable videos
(inside the block using convertVideoToH264, currentFile/currentUrl, and
readVideoFile) must validate the converted file before proceeding: after
obtaining convertedFile and re-reading videoData, re-run the storage/quota check
using convertedFile.size and verify videoData.canDecode is true; if quota would
be exceeded or videoData.canDecode remains false, revoke the object URL, surface
an error/toast and abort enqueuing instead of calling toast.success or
continuing, otherwise proceed to createObjectURL, toast.success and enqueue the
asset. Ensure the same validation is applied to the analogous block around lines
162-173 that performs conversion.

In `@apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts`:
- Around line 56-65: Guard against non-finite startTimestamp before clamping:
validate startTimestamp (and optionally endTimestamp) and default non-finite
values to 0 (or totalDuration for end) so NaN/Infinity can't flow into
sliceAudioBuffer and break the iterator. Update the AsyncGenerator function that
computes start/end (use the symbols startTimestamp, endTimestamp, totalDuration
= this.decoded.duration, start, end) to coerce startTimestamp to 0 when not a
finite number and only then apply Math.max/Math.min; ensure sliceAudioBuffer is
only called with finite start and end values.

In `@apps/web/src/services/video-cache/html-video-element-sink.ts`:
- Around line 165-174: The constructor for the class that takes handle:
VideoElementHandle and fps currently assigns this.fps without validation;
clamp/validate fps there (same logic used in create()) so fps <= 0 is replaced
with DEFAULT_FALLBACK_FPS (or a minimum positive value) before storing to
this.fps to avoid division-by-zero/negative stepping used later (see the
division by fps around where create() uses it).
- Around line 204-217: The two console.warn calls inside HTMLVideoElementSink
(the "seek failed:" branch and the "2d context unavailable, aborting iterator"
branch) must use the project's logger instead of console; replace them with the
instance logger (e.g., this.logger.warn or the class' logger instance) keeping
the original messages and include the error object where present (for the seek
branch) so the logs preserve context and metadata, then keep the existing
control flow (return/abort) unchanged.

In `@apps/web/src/services/video-cache/service.ts`:
- Around line 334-336: Replace the direct console.warn call in the WebCodecs
fallback branch with the project's shared logger (e.g., logger.warn or
projectLogger.warn) so the message "[VideoCache] WebCodecs unavailable; using
HTMLVideoElement fallback for video preview." is emitted via the centralized
logging utility; update or add the import for the shared logger at the top of
apps/web/src/services/video-cache/service.ts if missing and use the same logger
instance used elsewhere in this module (same file symbols like the VideoCache
service or the function handling WebCodecs fallback) to keep logs consistent
with project guidelines.
🪄 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: 4732af8b-121d-4cd9-bc43-51bf6335bb18

📥 Commits

Reviewing files that changed from the base of the PR and between fbe3db7 and 4d89b80.

📒 Files selected for processing (10)
  • apps/web/src/app/api/convert-video/route.ts
  • apps/web/src/core/managers/audio-manager.ts
  • apps/web/src/media/audio.ts
  • apps/web/src/media/ffmpeg-convert.ts
  • apps/web/src/media/processing.ts
  • apps/web/src/services/audio-cache/__tests__/fallback-audio-buffer-sink.test.ts
  • apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts
  • apps/web/src/services/video-cache/__tests__/html-video-element-sink.test.ts
  • apps/web/src/services/video-cache/html-video-element-sink.ts
  • apps/web/src/services/video-cache/service.ts

Comment thread apps/web/src/app/api/convert-video/route.ts
Comment thread apps/web/src/app/api/convert-video/route.ts Outdated
Comment thread apps/web/src/app/api/convert-video/route.ts Outdated
Comment thread apps/web/src/core/managers/audio-manager.ts
Comment thread apps/web/src/media/audio.ts
Comment thread apps/web/src/media/processing.ts Outdated
Comment thread apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts
Comment thread apps/web/src/services/video-cache/html-video-element-sink.ts
Comment thread apps/web/src/services/video-cache/html-video-element-sink.ts
Comment thread apps/web/src/services/video-cache/service.ts
@avillagran
Copy link
Copy Markdown
Author

Thanks for the review @coderabbitai. Pushed a force-push that addresses the actionable findings:

  • Temp-file collisions (route.ts): swapped Date.now() for a per-request unique dir via fs.mkdtemp(...). Cleanup uses rm -rf on the directory in finally.
  • Server diagnostics leak (route.ts): catch handler no longer returns error.message; client gets a generic "Video conversion failed." while the full error is still console.error'd server-side.
  • Unused onProgress param (ffmpeg-convert.ts): removed.
  • Post-conversion validation (processing.ts): re-runs the quota check against the converted file's size and fails the import if it no longer fits. If the converted file still reports canDecode === false, we now emit a soft "preview may be limited" toast rather than silently importing — the HTMLVideoElementSink fallback handles preview rendering in that case.
  • NaN guard in FallbackAudioBufferSink.buffers: non-finite startTimestamp is coerced to 0 before clamping. Added a unit test.
  • HTMLVideoElementSink constructor fps clamp: constructor now routes the incoming fps through clampFallbackFps, so direct new HTMLVideoElementSink({ handle, fps: 0 }) callers can no longer divide by zero. Added a unit test.

Skipped:

  • The "don't use console" suggestions. The web app currently uses console.warn / console.error in ~40 places across services (services/storage/service.ts, services/renderer/*, etc.) and there is no shared logger module. Routing only these new warnings through a logger that doesn't exist yet would be inconsistent. Happy to follow up separately if a logger lands.

Tests still green (bun test on the two fallback test files, 21/21).

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
apps/web/src/app/api/convert-video/route.ts (1)

2-5: 💤 Low value

Use the node: protocol for Node.js builtin module imports.

Per coding guidelines, Node.js builtin modules should use the node: protocol prefix.

♻️ Suggested fix
-import { writeFile, readFile, rm, mkdtemp } from "fs/promises";
-import { spawn } from "child_process";
-import path from "path";
-import os from "os";
+import { writeFile, readFile, rm, mkdtemp } from "node:fs/promises";
+import { spawn } from "node:child_process";
+import path from "node:path";
+import os from "node:os";
🤖 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 2 - 5, Replace bare
built-in module imports with the node: protocol so Node.js builtin modules are
imported correctly: update the fs/promises import (symbols writeFile, readFile,
rm, mkdtemp) to use "node:fs/promises", change the child_process import for
spawn to "node:child_process", and switch path and os imports (symbols path, os)
to "node:path" and "node:os" respectively; ensure you only change the module
specifiers and keep existing imported symbols unchanged.
🤖 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 42: The Content-Disposition header construction uses file.name directly
(see the route that builds `"Content-Disposition": \`attachment;
filename="${path.basename(file.name, ext)}.mp4"\``) which can be broken by
quotes/control chars; fix by sanitizing the base name returned from
path.basename(file.name, ext) (strip or replace control characters, quotes,
backslashes, and newlines) and additionally include an RFC 5987 encoded
filename* parameter for non-ASCII names (e.g., percent-encode UTF-8 and add
filename*=UTF-8''encodedName) so the header contains both a safe ASCII filename
and the encoded fallback. Ensure the sanitizedName is used for filename= and the
RFC5987-encoded value for filename*.

---

Nitpick comments:
In `@apps/web/src/app/api/convert-video/route.ts`:
- Around line 2-5: Replace bare built-in module imports with the node: protocol
so Node.js builtin modules are imported correctly: update the fs/promises import
(symbols writeFile, readFile, rm, mkdtemp) to use "node:fs/promises", change the
child_process import for spawn to "node:child_process", and switch path and os
imports (symbols path, os) to "node:path" and "node:os" respectively; ensure you
only change the module specifiers and keep existing imported symbols unchanged.
🪄 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: 05eb5012-9ed6-472f-874f-6ab88c5f4bb6

📥 Commits

Reviewing files that changed from the base of the PR and between 4d89b80 and f5856c6.

📒 Files selected for processing (10)
  • apps/web/src/app/api/convert-video/route.ts
  • apps/web/src/core/managers/audio-manager.ts
  • apps/web/src/media/audio.ts
  • apps/web/src/media/ffmpeg-convert.ts
  • apps/web/src/media/processing.ts
  • apps/web/src/services/audio-cache/__tests__/fallback-audio-buffer-sink.test.ts
  • apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts
  • apps/web/src/services/video-cache/__tests__/html-video-element-sink.test.ts
  • apps/web/src/services/video-cache/html-video-element-sink.ts
  • apps/web/src/services/video-cache/service.ts
🚧 Files skipped from review as they are similar to previous changes (8)
  • apps/web/src/media/audio.ts
  • apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts
  • apps/web/src/services/video-cache/service.ts
  • apps/web/src/services/audio-cache/tests/fallback-audio-buffer-sink.test.ts
  • apps/web/src/media/processing.ts
  • apps/web/src/core/managers/audio-manager.ts
  • apps/web/src/services/video-cache/html-video-element-sink.ts
  • apps/web/src/services/video-cache/tests/html-video-element-sink.test.ts

Comment thread apps/web/src/app/api/convert-video/route.ts Outdated
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

@avillagran — great follow-up, this addresses everything cleanly. A few quick notes on the individual items:

  • mkdtemp + rm -rf — the right call; eliminates the race condition and ensures both files are cleaned up atomically even if the output write fails mid-stream.
  • Generic client error — exactly the right split: operational context server-side, no internal paths/messages leaking to the client.
  • Post-conversion quota + canDecode toast — good defensive layering; the soft toast path is a much better UX than a silent import that later fails in unexpected ways.
  • NaN guard + fps clamp with tests — solid; guarding invariants at the constructor boundary rather than relying on callers is the right approach.
  • Console skip — the rationale holds. Introducing a logger abstraction only for the new code, when the rest of the service layer doesn't have one, would create more inconsistency than it solves. A follow-up that lands a unified logger and migrates all ~40 sites makes sense as a separate concern.

All 21/21 tests green and every actionable finding addressed. I'll go ahead and resolve my open review comments. Would you like me to trigger a fresh review pass on the force-pushed branch?

(╯°□°)╯ ✨🐇

✅ Actions performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

…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.
@avillagran avillagran force-pushed the feat/no-webcodecs-fallback branch from f5856c6 to 7204146 Compare May 17, 2026 00:23
@avillagran
Copy link
Copy Markdown
Author

Addressed the follow-up: Content-Disposition filename is now sanitized (strips control chars / quotes / backslashes / non-ASCII, capped at 200 chars, falls back to "video" if empty) via a small sanitizeContentDispositionName helper.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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`:
- Around line 123-126: The current stderr accumulation in the
proc.stderr.on("data") handler (variable stderr) can grow unbounded; change the
handler to append incoming chunks but truncate to a fixed max tail (e.g., 8KB or
configurable MAX_STDERR_BYTES) so only the most recent bytes are kept for
diagnostics: in the proc.stderr.on("data", ...) callback append data.toString()
(or buffer) and if stderr.length > MAX_STDERR_BYTES slice off the oldest bytes
(keep the rightmost tail), ensuring you preserve complete UTF‑8 characters if
using strings or operate on Buffers to avoid splitting multi-byte characters;
replace existing stderr growth logic with this size-limited tailing behavior.
- Line 19: The regex in the .replace call inside the convert-video route uses
explicit control-character escapes and fails Biome lint; update the .replace
call (the method invocation using .replace(/[\\"\r\n\x00-\x1f]/g, "_")) to use a
Unicode property class for control characters (Cc) with the u flag and include
the double-quote and backslash literals so behavior is preserved; ensure the
pattern uses the global and unicode flags (g and u) and still replaces control
characters, backslashes and quotes with the same replacement string.

In `@apps/web/src/services/video-cache/html-video-element-sink.ts`:
- Around line 198-199: The assignment let t = Math.max(0, startTimestamp) can
produce NaN if startTimestamp is non-finite; change it to guard first (e.g., let
t = Number.isFinite(startTimestamp) ? Math.max(0, startTimestamp) : 0) so
non-finite/undefined values fall back to 0 and the iterator yields frames
correctly; update the occurrence of this assignment in html-video-element-sink
(where variable t and startTimestamp are used) to use this guarded expression.
🪄 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: 9a84cb46-2133-4684-a7bd-b2448bbec847

📥 Commits

Reviewing files that changed from the base of the PR and between f5856c6 and 7204146.

📒 Files selected for processing (10)
  • apps/web/src/app/api/convert-video/route.ts
  • apps/web/src/core/managers/audio-manager.ts
  • apps/web/src/media/audio.ts
  • apps/web/src/media/ffmpeg-convert.ts
  • apps/web/src/media/processing.ts
  • apps/web/src/services/audio-cache/__tests__/fallback-audio-buffer-sink.test.ts
  • apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts
  • apps/web/src/services/video-cache/__tests__/html-video-element-sink.test.ts
  • apps/web/src/services/video-cache/html-video-element-sink.ts
  • apps/web/src/services/video-cache/service.ts
🚧 Files skipped from review as they are similar to previous changes (8)
  • apps/web/src/services/video-cache/tests/html-video-element-sink.test.ts
  • apps/web/src/services/video-cache/service.ts
  • apps/web/src/media/audio.ts
  • apps/web/src/media/ffmpeg-convert.ts
  • apps/web/src/media/processing.ts
  • apps/web/src/core/managers/audio-manager.ts
  • apps/web/src/services/audio-cache/tests/fallback-audio-buffer-sink.test.ts
  • apps/web/src/services/audio-cache/fallback-audio-buffer-sink.ts

// "filename=" parameter stays unambiguous across clients.
const stripped = rawName
// eslint-disable-next-line no-control-regex
.replace(/[\\"\r\n\x00-\x1f]/g, "_")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Regex currently violates the control-character rule (Biome lint error).

Use a Unicode property class for control chars to satisfy lint while preserving behavior.

Suggested patch
-		.replace(/[\\"\r\n\x00-\x1f]/g, "_")
+		.replace(/[\p{Cc}"\\\r\n]/gu, "_")
As per coding guidelines, "Don't use control characters and escape sequences that match control characters in regular expression literals".
📝 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
.replace(/[\\"\r\n\x00-\x1f]/g, "_")
.replace(/[\p{Cc}"\\\r\n]/gu, "_")
🧰 Tools
🪛 Biome (2.4.15)

[error] 19-19: Unexpected control character in a regular expression.

(lint/suspicious/noControlCharactersInRegex)


[error] 19-19: Unexpected control character in a regular expression.

(lint/suspicious/noControlCharactersInRegex)

🤖 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 19, The regex in the
.replace call inside the convert-video route uses explicit control-character
escapes and fails Biome lint; update the .replace call (the method invocation
using .replace(/[\\"\r\n\x00-\x1f]/g, "_")) to use a Unicode property class for
control characters (Cc) with the u flag and include the double-quote and
backslash literals so behavior is preserved; ensure the pattern uses the global
and unicode flags (g and u) and still replaces control characters, backslashes
and quotes with the same replacement string.

Comment on lines +123 to +126
let stderr = "";
proc.stderr.on("data", (data) => {
stderr += data.toString();
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bound stderr buffering to avoid unbounded memory growth.

stderr += data.toString() grows without limit; noisy/long-running ffmpeg failures can inflate memory unnecessarily. Keep only the tail you need for diagnostics.

Suggested patch
 		let stderr = "";
 		proc.stderr.on("data", (data) => {
-			stderr += data.toString();
+			stderr = (stderr + data.toString()).slice(-4000);
 		});
📝 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
let stderr = "";
proc.stderr.on("data", (data) => {
stderr += data.toString();
});
let stderr = "";
proc.stderr.on("data", (data) => {
stderr = (stderr + data.toString()).slice(-4000);
});
🤖 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 123 - 126, The
current stderr accumulation in the proc.stderr.on("data") handler (variable
stderr) can grow unbounded; change the handler to append incoming chunks but
truncate to a fixed max tail (e.g., 8KB or configurable MAX_STDERR_BYTES) so
only the most recent bytes are kept for diagnostics: in the
proc.stderr.on("data", ...) callback append data.toString() (or buffer) and if
stderr.length > MAX_STDERR_BYTES slice off the oldest bytes (keep the rightmost
tail), ensuring you preserve complete UTF‑8 characters if using strings or
operate on Buffers to avoid splitting multi-byte characters; replace existing
stderr growth logic with this size-limited tailing behavior.

Comment on lines +198 to +199
let t = Math.max(0, startTimestamp);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard non-finite startTimestamp before clamping.

Line 198 uses Math.max(0, startTimestamp) directly; if startTimestamp is NaN, t becomes NaN and the iterator exits silently without yielding frames.

🔧 Proposed fix
-		let t = Math.max(0, startTimestamp);
+		const normalizedStart =
+			typeof startTimestamp === "number" && Number.isFinite(startTimestamp)
+				? startTimestamp
+				: 0;
+		let t = Math.max(0, normalizedStart);
📝 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
let t = Math.max(0, startTimestamp);
const normalizedStart =
typeof startTimestamp === "number" && Number.isFinite(startTimestamp)
? startTimestamp
: 0;
let t = Math.max(0, normalizedStart);
🤖 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
198 - 199, The assignment let t = Math.max(0, startTimestamp) can produce NaN if
startTimestamp is non-finite; change it to guard first (e.g., let t =
Number.isFinite(startTimestamp) ? Math.max(0, startTimestamp) : 0) so
non-finite/undefined values fall back to 0 and the iterator yields frames
correctly; update the occurrence of this assignment in html-video-element-sink
(where variable t and startTimestamp are used) to use this guarded expression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant