Skip to content

fix: run the export graph the tests cover, not a drifting copy - #1809

Open
chiragjangiir wants to merge 1 commit into
reframe-oss:mainfrom
chiragjangiir:fix/unify-export-graph
Open

chiragjangiir wants to merge 1 commit into
reframe-oss:mainfrom
chiragjangiir:fix/unify-export-graph

Conversation

@chiragjangiir

Copy link
Copy Markdown

Description

src/lib/ffmpeg.ts held a second, complete copy of the FFmpeg argv builder (buildVideoFilter, buildAudioFilter, buildArguments) that nothing imported — buildArguments is not exported and has zero references in the repository at main. The unit tests imported that copy, so they stayed green while src/lib/ffmpeg.worker.ts ran its own copy that had silently drifted behind it.

Drift between the tested copy and the executed copy:

Behaviour ffmpeg.ts (tested, never called) ffmpeg.worker.ts (executed)
Sharpness setting unsharp=5:5:n:5:5:0.0 missing — the slider had no effect on exports
Volume setting volume=n.nn missing — the slider had no effect on exports
trim with null trimEnd trim=start=X (FFmpeg infers the end) trim=start=X:end=999999 placeholder
atrim with null trimEnd atrim=start=X atrim=start=X:end=999999 placeholder
-t output cap unconditional (PR #1289) gated on recipe.speed !== 1

The last row is the user-visible one: PR #1289 ("fix: enforce -t flag on export to prevent infinite loop with background music") is merged, but its diff touched only src/lib/ffmpeg.ts — the uncalled copy. The executed path kept its speed !== 1 gate, so the fix never ran at runtime, issue #1577 was closed without a behavioural change, and the duplicate report #1493 is still open.

What changed

  • New src/lib/exportGraph.ts: one pure implementation of the export graph — buildVideoFilter, buildAudioFilter, buildAudioTrimFilter, buildDurationArgs, buildArguments. No worker globals, no DOM, no FFmpeg instance.
  • src/lib/ffmpeg.worker.ts imports it and deletes its private copy; src/lib/ffmpeg.ts deletes its dead copy and re-exports the two filters the existing tests import.
  • buildDurationArgs centralises the -t decision and rejects durations that cannot be trusted (0 from a failed metadata load, Infinity from MediaRecorder WebM containers, NaN), which would otherwise reach FFmpeg as -t 0 (zero-length export) or -t Infinity (aborts the export).
  • New src/lib/tests/exportGraph.test.ts (22 tests) asserts the argv the worker actually passes: duration capping, the looped-music case, volume and muting, codec-per-container, overlay positioning, and the removal of the 999999 placeholders.

Net effect: the export behaves the way the reviewed code intended — Sharpness, Volume and the -t cap are live — and the tests can no longer pass against a copy nothing runs.

Related Issue

Closes #1493

Related: #1577 (closed by #1289 without a behavioural fix), #1289 (merged, landed in the uncalled copy), #1590 (closed unmerged).

Type of Contribution

  • Bug fix
  • New feature
  • Documentation update
  • Refactor
  • GSSoC contribution

Participant Info

  • GitHub username: chiragjangiir
  • Contribution level (Beginner/Intermediate/Advanced): Advanced

Screen Recording

A recording is being prepared and will be attached to this PR. Suggested demonstration: bun run dev → Audio & Speed → Volume = 0% → Export → the exported file's audio is silent (on main the same export keeps full-volume audio).

Checklist

  • I have read the contribution guidelines
  • My changes follow the project structure
  • bun run lint passes — run as npx eslint . → exit 0 (bun is not installed on this machine)
  • bunx tsc --noEmit passes — run as npx tsc --noEmit → exit 0
  • bun run build passes — run as npx next build --webpack → exit 0
  • New interactive elements have aria-label / accessible names (no new UI added)
  • No console.log statements left in
  • This PR is related to a valid issue ([BUG] Export Can Hang Indefinitely When Video Has No Audio and Background Music Is Looped #1493 — open, unassigned, no linked PR)
  • Tested in Chrome, Firefox and Safari (Chrome verified end to end; Firefox/Safari not run)
  • Screen recording attached above (required for UI/feature changes)

How this was verified

Unit tests

$ npx vitest run src/lib/tests/exportGraph.test.ts src/lib/tests/ffmpeg.test.ts src/lib/tests/videoFilter.test.ts
 ✓ src/lib/tests/exportGraph.test.ts (22 tests)
 ✓ src/lib/tests/ffmpeg.test.ts (9 tests)
 ✓ src/lib/tests/videoFilter.test.ts (16 tests)
 Test Files  3 passed (3)      Tests  47 passed (47)

Static checks: npx tsc --noEmit → 0 · npx eslint . → 0 · npx next build --webpack → 0 (static export generated).

Runtime A/B through the real editor UI. Two dev servers: localhost:3100 = this branch, localhost:3101 = main. Each run: import a ~3 s fixture → set Volume → EXPORT → decode the exported MP4 in the page and measure the audio.

Build Volume Source RMS Exported RMS Ratio Export time
main 0% 0.35235 0.35085 0.996 (setting inert) ~60 s, progress readout showed -287106300%
branch 50% 0.35080 0.17535 0.500 2 s
branch 0% 0.35080 0.000000 (peak 0) 0.000 2 s

On main the Volume slider has no effect on the exported audio; on this branch it is exact — 50% halves the amplitude and 0% produces digital silence.

The -t cap was confirmed in the live argv with a temporary diagnostic (since removed): "-t", "3.004000" for a 3.004 s fixture produced a 2.963 s file — no truncation, no overshoot.

Pre-existing test failures (not introduced here). npx vitest run reports 4 failing files / 6 failing tests out of 131; all four fail identically on an unmodified main worktree: src/lib/exportEstimate.test.ts (duplicated expect import — parse error), useVideoEditor.test.ts and ThemeToggle.test.tsx (react 19.3.0 vs react-dom 19.2.8 mismatch from f013f3c), and editorPersistence.test.ts (localStorage undefined). CI does not run vitest (main.yml runs install + build only).

Limitations

src/lib/ffmpeg.ts carried a second, complete copy of the FFmpeg argv builder
that nothing imported, so only the unit tests exercised it. The copy the worker
actually ran had silently drifted: the Sharpness and Volume settings (both wired
to UI sliders) produced no filter, trim bounds fell back to 999999 placeholders,
and the unconditional -t output cap merged for reframe-oss#1289 landed in the unused copy,
leaving exports uncapped and looped-music exports unbounded.

Keep one implementation in src/lib/exportGraph.ts, used by both the worker and
the tests, and cover the argv the worker passes.

🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
@netlify

netlify Bot commented Oct 11, 2026

Copy link
Copy Markdown

👷 Deploy request for reframe-os pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 54e0b7a

@github-actions

Copy link
Copy Markdown
Contributor

👋 Thanks for your PR, @chiragjangiir!

Welcome to Reframe — a browser-based video editor built for everyone 🎬

🟠 GSSoC'26 PR detected — thanks for contributing under GirlScript Summer of Code 2026!

What happens next

  1. 🤖 Automated checks — build & TypeScript typecheck will run automatically
  2. ✅ Vercel preview — a preview deployment will be created (requires maintainer authorization for fork PRs)
  3. 👀 Code review — a maintainer will review your changes
  4. 🚀 Merge — once approved, your PR will be merged!

Quick checklist

  • PR title follows Conventional Commits (e.g. feat: add dark mode)
  • Linked the issue this PR closes (e.g. Closes #123)
  • Tested the changes locally (bun run dev)
  • Build passes (bun run build)

Useful links

Happy coding! 🎉

@github-actions

Copy link
Copy Markdown
Contributor

✅ PR Format Check Passed — @chiragjangiir

Basic format checks passed. A maintainer will review your code changes.

This does not mean the PR is approved — it just means the format is correct.

@github-actions github-actions Bot added gssoc'26 GirlScript Summer of Code 2026 level:advanced Advanced level - 55 pts type:bug Bug fix type:design UI/UX design type:docs Documentation type:feature New feature type:performance Performance type:refactor Code refactor type:testing Testing labels Oct 11, 2026

This branch has not been deployed

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

Labels

gssoc'26 GirlScript Summer of Code 2026 level:advanced Advanced level - 55 pts type:bug Bug fix type:design UI/UX design type:docs Documentation type:feature New feature type:performance Performance type:refactor Code refactor type:testing Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Export Can Hang Indefinitely When Video Has No Audio and Background Music Is Looped

1 participant