fix(export): fast-path simple edited audio tracks#297
Merged
webadderall merged 5 commits intoApr 22, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a filtergraph fast-path for edited-track audio muxing, a classifier and segment builder to choose fast-path vs offline fallback, propagates strategy/segments/sample-rate through exporter and Electron IPC, and includes tests for filterbuilders and strategy logic. Changes
Sequence DiagramsequenceDiagram
participant Renderer
participant Strategy as Strategy<br/>Decision
participant Exporter as Native<br/>Audio Plan
participant ElectronMain as Electron<br/>Main
participant FFmpeg
rect rgba(200,150,100,0.5)
Note over Renderer,FFmpeg: Filtergraph Fast-Path
Renderer->>Strategy: classifyEditedTrackStrategy(input)
Strategy-->>Renderer: "filtergraph-fast-path" or "offline-render-fallback"
Renderer->>Exporter: buildNativeAudioPlan()
Exporter-->>Renderer: Plan (includes editedTrackSegments, audioSourceSampleRate when fast-path)
Renderer->>ElectronMain: nativeVideoExportFinish({editedTrackStrategy, editedTrackSegments, audioSourceSampleRate})
ElectronMain->>ElectronMain: buildEditedTrackSourceAudioFilter(segments, sampleRate)
ElectronMain->>FFmpeg: ffmpeg -i video -i audioSource -filter_complex "[...]" -map 0:v -map [aout]
FFmpeg-->>ElectronMain: Muxed output
end
rect rgba(100,150,200,0.5)
Note over Renderer,FFmpeg: Offline Render Fallback
Renderer->>Strategy: classifyEditedTrackStrategy(input)
Strategy-->>Renderer: "offline-render-fallback"
Renderer->>Exporter: buildNativeAudioPlan()
Renderer->>ElectronMain: nativeVideoExportFinish({audioMode: "edited-track"})
ElectronMain->>ElectronMain: AudioProcessor.renderEditedAudioTrack()
ElectronMain->>FFmpeg: ffmpeg -i video -i renderedAudio -map 0:v -map 1:a
FFmpeg-->>ElectronMain: Muxed output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
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 (1)
electron/ipc/export/native-video.ts (1)
383-403:⚠️ Potential issue | 🟠 MajorValidate the fast-path audio source before returning early.
When
editedTrackStrategy === "filtergraph-fast-path"butaudioSourcePathis missing, Line 401 returns the video unchanged, silently dropping edited-track audio. Fast-path requests should fail before the generic!audioInputPathearly return.Proposed fix
let audioInputPath = options.audioSourcePath ?? null; const useEditedTrackFiltergraph = audioMode === "edited-track" && options.editedTrackStrategy === "filtergraph-fast-path"; if (audioMode === "edited-track" && !useEditedTrackFiltergraph) { if (!options.editedAudioData) { throw new Error("Edited audio data is missing for native export"); } @@ await fs.writeFile(audioInputPath, Buffer.from(options.editedAudioData)); tempArtifacts.push(audioInputPath); } + if (useEditedTrackFiltergraph && !audioInputPath) { + throw new Error("Edited-track source audio path is missing for native export"); + } + if (!audioInputPath) { return videoPath; }Also applies to: 428-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/export/native-video.ts` around lines 383 - 403, When using the fast-path (useEditedTrackFiltergraph true) we must fail if there is no audio source instead of falling through to the generic early return; before the generic `if (!audioInputPath) { return videoPath; }` add a check that if `useEditedTrackFiltergraph` (or equivalently `options.editedTrackStrategy === "filtergraph-fast-path"` and `audioMode === "edited-track"`) and `!audioInputPath` then throw a clear Error (e.g., "Fast-path edited-track export requires an audio source"); apply the same validation in the other similar block (the later edited-track handling around lines 428-436) so fast-path requests never silently drop audio.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/nativeVideoExport.ts`:
- Around line 165-193: buildEditedTrackSourceAudioFilter currently lets
malformed parameters generate invalid FFmpeg filters; update it to reject bad
inputs by returning null: verify sourceSampleRate is finite and after
Math.round(normalized) is >= 1 (reject if normalizedSourceSampleRate < 1); for
each segment ensure segment.startMs and segment.endMs are finite numbers and
that (endMs - startMs) > 0.5; require segment.speed to be Number.isFinite and >
0 (do not silently default to 1) and return null if any segment has invalid
speed; use formatFfmpegSeconds only after these checks. Reference:
buildEditedTrackSourceAudioFilter, segments array items, sourceSampleRate,
formatFfmpegSeconds.
In `@src/lib/exporter/editedTrackStrategy.ts`:
- Around line 23-31: The fast-path check currently only verifies region.speed
via isSafeFiltergraphSpeed and hasSafeFiltergraphSpeedRegions; update these
validations to also require finite, well-formed timeline bounds: ensure
sourceDurationMs is finite, >0, and that each SpeedRegion has finite startMs and
endMs with 0 <= startMs < endMs <= sourceDurationMs; if any of these fail, treat
as unsafe and fall back from the fast path. Modify or add checks around
isSafeFiltergraphSpeed, hasSafeFiltergraphSpeedRegions, and the fast-path opt-in
logic in editedTrackStrategy to incorporate these bounds checks so
open-ended/infinite durations or regions prevent selecting the fast path.
---
Outside diff comments:
In `@electron/ipc/export/native-video.ts`:
- Around line 383-403: When using the fast-path (useEditedTrackFiltergraph true)
we must fail if there is no audio source instead of falling through to the
generic early return; before the generic `if (!audioInputPath) { return
videoPath; }` add a check that if `useEditedTrackFiltergraph` (or equivalently
`options.editedTrackStrategy === "filtergraph-fast-path"` and `audioMode ===
"edited-track"`) and `!audioInputPath` then throw a clear Error (e.g.,
"Fast-path edited-track export requires an audio source"); apply the same
validation in the other similar block (the later edited-track handling around
lines 428-436) so fast-path requests never silently drop audio.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 734600c9-96b4-4f8e-abef-913e7d29bd1a
📒 Files selected for processing (10)
electron/electron-env.d.tselectron/ipc/export/native-video.tselectron/ipc/nativeVideoExport.test.tselectron/ipc/nativeVideoExport.tselectron/preload.tssrc/lib/exporter/editedTrackStrategy.test.tssrc/lib/exporter/editedTrackStrategy.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/videoExporter.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/exporter/editedTrackStrategy.ts (2)
54-81: Consider validatingtrimRegionsbounds alongsidespeedRegions.
buildKeptRangestruststrimRegion.startMs/endMsto be finite numbers, but unlikespeedRegionsthey're never checked. A non-finite or negative entry propagates throughMath.max/Math.minasNaN, which in turn corruptscursorMsand the kept-range set. In practice this currently degrades gracefully (segments come out empty, soclassifyEditedTrackStrategyfalls back to offline render), but it's silent — a malformed trim region takes the fast path off the table without any signal. A cheap finiteness/ordering guard (either reject the whole fast path likehasSafeFiltergraphSpeedRegionsdoes, or skip individual malformed trims) would make the behavior explicit and match the validation rigor already applied to speed regions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/editedTrackStrategy.ts` around lines 54 - 81, buildKeptRanges currently assumes trimRegion.startMs/endMs are finite and ordered which can produce NaN and corrupt kept ranges; add validation like hasSafeFiltergraphSpeedRegions: for each trimRegion in buildKeptRanges check Number.isFinite(startMs) and Number.isFinite(endMs), enforce startMs < endMs, and clamp to [0, sourceDurationMs]; either skip individual malformed entries (so they don't pollute cursorMs) or, to be stricter, return an error/empty marker to force the slow path in classifyEditedTrackStrategy; reference the buildKeptRanges function and trimRegions array when adding these guards.
125-131: First-match speed lookup silently hides overlappingspeedRegions.
speedRegions.find(...)picks the first region containing the midpoint, so if two regions overlap (same midpoint covered), the later one is silently ignored rather than rejected. Since the fast path is opt-in and correctness-critical, consider either (a) rejecting overlapping speed regions inhasSafeFiltergraphSpeedRegions/buildEditedTrackSourceSegmentsand falling back, or (b) asserting at most one match here and returning[]otherwise. Low likelihood in normal UI flows, but worth hardening given silent incorrect audio would be hard to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/editedTrackStrategy.ts` around lines 125 - 131, The current first-match lookup for speedRegions in buildEditedTrackSourceSegments (using speedRegions.find with midpointMs) silently ignores overlapping regions; update the logic to detect multiple matches for a given midpointMs and harden behavior: either (a) add an explicit overlap check earlier (e.g., in hasSafeFiltergraphSpeedRegions or a new validateSpeedRegions helper) that rejects/returns false when any two speedRegions overlap so buildEditedTrackSourceSegments never encounters ambiguous matches, or (b) in buildEditedTrackSourceSegments assert there is at most one matching region for midpointMs (by using filter to collect matches) and if matches.length !== 1 treat it as a failure (return []), ensuring you still call isSafeFiltergraphSpeed and handle non-safe speeds the same way; reference symbols: speedRegions, midpointMs, buildEditedTrackSourceSegments, hasSafeFiltergraphSpeedRegions, isSafeFiltergraphSpeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/nativeVideoExport.ts`:
- Around line 205-215: The multiplication normalizedSourceSampleRate * speed can
produce NaN/Infinity or an integer outside JS safe range before pushing asetrate
into segmentFilter, so validate adjustedSampleRate after computing it in the
block that handles Math.abs(speed - 1) > 0.0001: ensure it's finite,
Number.isSafeInteger(adjustedSampleRate) (or round then check), and >= 1; if the
check fails set hasInvalidSegment = true and return (same behavior as the
current <1 guard) to avoid emitting an invalid asetrate; reference the
adjustedSampleRate, normalizedSourceSampleRate, speed, segmentFilter and
hasInvalidSegment symbols.
- Around line 182-201: Add a validation step inside the segments.forEach loop to
reject negative timestamps before building the atrim filter: check if
segment.startMs < 0 || segment.endMs < 0 and if so set hasInvalidSegment = true
and return (skip that segment), and ensure the surrounding exported function
returns null when hasInvalidSegment is true (so malformed input is rejected).
Place this check before constructing the segmentFilter (near the existing
Number.isFinite and duration checks) and reference the existing symbols
segments, hasInvalidSegment, formatFfmpegSeconds, and the atrim filter builder
so the invalid negatives never reach the atrim invocation.
---
Nitpick comments:
In `@src/lib/exporter/editedTrackStrategy.ts`:
- Around line 54-81: buildKeptRanges currently assumes trimRegion.startMs/endMs
are finite and ordered which can produce NaN and corrupt kept ranges; add
validation like hasSafeFiltergraphSpeedRegions: for each trimRegion in
buildKeptRanges check Number.isFinite(startMs) and Number.isFinite(endMs),
enforce startMs < endMs, and clamp to [0, sourceDurationMs]; either skip
individual malformed entries (so they don't pollute cursorMs) or, to be
stricter, return an error/empty marker to force the slow path in
classifyEditedTrackStrategy; reference the buildKeptRanges function and
trimRegions array when adding these guards.
- Around line 125-131: The current first-match lookup for speedRegions in
buildEditedTrackSourceSegments (using speedRegions.find with midpointMs)
silently ignores overlapping regions; update the logic to detect multiple
matches for a given midpointMs and harden behavior: either (a) add an explicit
overlap check earlier (e.g., in hasSafeFiltergraphSpeedRegions or a new
validateSpeedRegions helper) that rejects/returns false when any two
speedRegions overlap so buildEditedTrackSourceSegments never encounters
ambiguous matches, or (b) in buildEditedTrackSourceSegments assert there is at
most one matching region for midpointMs (by using filter to collect matches) and
if matches.length !== 1 treat it as a failure (return []), ensuring you still
call isSafeFiltergraphSpeed and handle non-safe speeds the same way; reference
symbols: speedRegions, midpointMs, buildEditedTrackSourceSegments,
hasSafeFiltergraphSpeedRegions, isSafeFiltergraphSpeed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bed4b9e9-c747-458f-bf9c-6604beaa46b6
📒 Files selected for processing (4)
electron/ipc/nativeVideoExport.test.tselectron/ipc/nativeVideoExport.tssrc/lib/exporter/editedTrackStrategy.test.tssrc/lib/exporter/editedTrackStrategy.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/exporter/editedTrackStrategy.test.ts
- electron/ipc/nativeVideoExport.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
electron/ipc/nativeVideoExport.test.ts (2)
18-32: Assert the complete edited-track graph to cover segment ordering.The current checks validate the speed-adjusted segment but not the identity-speed segment or concat input order. An exact assertion would better guard the fast-path contract.
🧪 Proposed test tightening
const filter = buildEditedTrackSourceAudioFilter( [ { startMs: 0, endMs: 2_000, speed: 1 }, { startMs: 2_000, endMs: 6_000, speed: 1.5 }, ], 44_100, ); - expect(filter).toContain( - "[1:a]atrim=start=2.000:end=6.000,asetpts=PTS-STARTPTS,asetrate=66150,aresample=44100[edited_audio_1]", - ); - expect(filter).toContain("concat=n=2:v=0:a=1[aout]"); + expect(filter).toBe( + "[1:a]atrim=start=0.000:end=2.000,asetpts=PTS-STARTPTS[edited_audio_0];" + + "[1:a]atrim=start=2.000:end=6.000,asetpts=PTS-STARTPTS,asetrate=66150,aresample=44100[edited_audio_1];" + + "[edited_audio_0][edited_audio_1]concat=n=2:v=0:a=1[aout]", + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/nativeVideoExport.test.ts` around lines 18 - 32, Replace the partial contains assertions in the buildEditedTrackSourceAudioFilter test with a single exact equality assertion that verifies the full generated filter string (so it includes the first identity-speed segment [0:a]atrim=start=0.000:end=2.000,asetpts=PTS-STARTPTS[edited_audio_0], the second speed-shifted segment [1:a]atrim=start=2.000:end=6.000,asetpts=PTS-STARTPTS,asetrate=66150,aresample=44100[edited_audio_1], and the concat tail concat=n=2:v=0:a=1[aout]) in the correct order; use buildEditedTrackSourceAudioFilter as the source and replace the two expect(...).toContain calls with expect(filter).toEqual(<expected full filter string>) to lock segment ordering and exact graph shape.
7-16: Pin the trimmed filtergraph, not just the concat suffix.This builder returns a deterministic filtergraph, so an exact assertion would catch regressions in the trim ranges, labels, and concat inputs that the current
toContainwould miss.🧪 Proposed test tightening
describe("buildTrimmedSourceAudioFilter", () => { it("concatenates trimmed source segments into a single output label", () => { expect( buildTrimmedSourceAudioFilter([ { startMs: 0, endMs: 2_000 }, { startMs: 4_000, endMs: 6_000 }, ]), - ).toContain("concat=n=2:v=0:a=1[aout]"); + ).toBe( + "[1:a]atrim=start=0.000:end=2.000,asetpts=PTS-STARTPTS[trimmed_audio_0];" + + "[1:a]atrim=start=4.000:end=6.000,asetpts=PTS-STARTPTS[trimmed_audio_1];" + + "[trimmed_audio_0][trimmed_audio_1]concat=n=2:v=0:a=1[aout]", + ); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/nativeVideoExport.test.ts` around lines 7 - 16, The test for buildTrimmedSourceAudioFilter is too loose — replace the toContain assertion with an exact equality assertion that compares the full deterministic filtergraph string returned by buildTrimmedSourceAudioFilter for the given segments; construct an expected string that includes the full trim filters (with the correct startMs/endMs ranges), the generated labels for each trimmed segment, the concat input ordering (n=2:v=0:a=1) and the final output label ([aout]) and assert the produced string equals that exact expected string so regressions in ranges, labels, or input ordering are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@electron/ipc/nativeVideoExport.test.ts`:
- Around line 18-32: Replace the partial contains assertions in the
buildEditedTrackSourceAudioFilter test with a single exact equality assertion
that verifies the full generated filter string (so it includes the first
identity-speed segment
[0:a]atrim=start=0.000:end=2.000,asetpts=PTS-STARTPTS[edited_audio_0], the
second speed-shifted segment
[1:a]atrim=start=2.000:end=6.000,asetpts=PTS-STARTPTS,asetrate=66150,aresample=44100[edited_audio_1],
and the concat tail concat=n=2:v=0:a=1[aout]) in the correct order; use
buildEditedTrackSourceAudioFilter as the source and replace the two
expect(...).toContain calls with expect(filter).toEqual(<expected full filter
string>) to lock segment ordering and exact graph shape.
- Around line 7-16: The test for buildTrimmedSourceAudioFilter is too loose —
replace the toContain assertion with an exact equality assertion that compares
the full deterministic filtergraph string returned by
buildTrimmedSourceAudioFilter for the given segments; construct an expected
string that includes the full trim filters (with the correct startMs/endMs
ranges), the generated labels for each trimmed segment, the concat input
ordering (n=2:v=0:a=1) and the final output label ([aout]) and assert the
produced string equals that exact expected string so regressions in ranges,
labels, or input ordering are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac0981de-eb1e-4034-996c-3222a3f3a62a
📒 Files selected for processing (2)
electron/ipc/nativeVideoExport.test.tselectron/ipc/nativeVideoExport.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/ipc/nativeVideoExport.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/ipc/nativeVideoExport.test.ts (1)
49-71: Consider adding coverage for slowdown and boundary speeds.Malformed-input coverage is solid, but the happy-path only exercises
speed=1andspeed=1.5. Aspeed<1case (e.g.,0.5) would guard against regressions in theasetrate/aresampleformula for slowdowns, and an assertion for negative/NaNspeedvalues would round out the rejection set alongside the existingspeed=0andMAX_SAFE_INTEGERcases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/nativeVideoExport.test.ts` around lines 49 - 71, Add test cases to cover slowdown and additional malformed speeds for buildEditedTrackSourceAudioFilter: add a successful (happy-path) assertion using speed < 1 (e.g., 0.5) to verify slowdown handling (ensure it returns a non-null/expected filter), and extend the malformed-input assertions with negative and NaN speed values (e.g., speed: -1 and speed: Number.NaN) alongside the existing 0 and MAX_SAFE_INTEGER checks to ensure those inputs return null; target the same test block that currently exercises buildEditedTrackSourceAudioFilter so the new expectations run with the same sample rate and segment shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@electron/ipc/nativeVideoExport.test.ts`:
- Around line 49-71: Add test cases to cover slowdown and additional malformed
speeds for buildEditedTrackSourceAudioFilter: add a successful (happy-path)
assertion using speed < 1 (e.g., 0.5) to verify slowdown handling (ensure it
returns a non-null/expected filter), and extend the malformed-input assertions
with negative and NaN speed values (e.g., speed: -1 and speed: Number.NaN)
alongside the existing 0 and MAX_SAFE_INTEGER checks to ensure those inputs
return null; target the same test block that currently exercises
buildEditedTrackSourceAudioFilter so the new expectations run with the same
sample rate and segment shapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d2ebf6e-1858-4035-a608-ec6408764e98
📒 Files selected for processing (1)
electron/ipc/nativeVideoExport.test.ts
d344a83 to
383bde9
Compare
9 tasks
10 tasks
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
2 participants
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.
Description
Add a native FFmpeg fast path for simple
edited-trackexports so the app does not always fall back to offline Web Audio rendering for embedded-source trim/speed edits.Motivation
Long exports that land in
edited-trackstill spend a noticeable amount of time in audio finalization even when the edit is only trim/speed on the original embedded audio track. This PR narrows that case to a conservative native filtergraph path while preserving the existing fallback for more complex mixes.Type of Change
Related Issue(s)
Screenshots / Video
N/A. This changes the export path rather than the visible editor UI.
Testing Guide
npx vitest --run src/lib/exporter/editedTrackStrategy.test.ts electron/ipc/nativeVideoExport.test.tsnode ./node_modules/typescript/bin/tsc --noEmitnpx vite buildmodern+breezewith a2000:120000:1.5speed region358873 ms189657 ms31722.6 ms->25524.2 msH.264 + AAC,214.166667 s,266231169 bytesChecklist
Summary by CodeRabbit
New Features
Tests