fix(audio): replace ffmpeg probes with ffprobe for duration probing#216
Conversation
|
|
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
FFmpeg Duration Probe Command electron/ipc/handlers.ts |
Simplified FFmpeg invocation by removing null format output arguments (-f null -), retaining only input file specification while preserving existing duration parsing from stderr. |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
🐰 FFmpeg commands, sleek and refined,
Null formats removed, simplicity signed,
One-iflag does the trick,
Duration parsing, swift and slick! ✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'fix(audio): replace ffmpeg probes with ffprobe for duration probing' directly and clearly describes the main change: replacing ffmpeg-based probing with ffprobe for audio duration detection to fix audio sync issues. |
| Description check | ✅ Passed | The PR description comprehensively covers all key sections from the template: detailed problem statement with reproduction steps, root cause analysis, specific changes made, testing verification, and risk assessment. It far exceeds the minimum requirements. |
✏️ 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.
Comment @coderabbitai help to get the list of available commands and usage tips.
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/handlers.ts`:
- Around line 1197-1199: In the ffprobe path adjustment inside the
app.isPackaged branch, remove the unnecessary escape of the forward slash in the
regex character class used in ffprobeStatic.replace (change the capture group
from ([\/\\]) to ([/\\]) in the regex /\.asar([/\\])/), leaving the rest of the
replace call and logic (ffprobeStatic.replace and app.isPackaged) intact.
In `@scripts/benchmark-export-queues.mjs`:
- Line 10: Add a fail-fast validation for the imported ffprobeStatic at the
start of main(): check that ffprobeStatic is available (truthy) similar to the
existing ffmpegStatic/electron guards, and if missing log an error (use the same
logger used elsewhere, e.g., processLogger or console.error) and exit with a
non-zero status (process.exit(1)) so inspectOutput() can’t silently return null
and hide a setup failure; reference ffprobeStatic and main() when making the
change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ddbf42a0-ee86-41fa-89ae-8020b7a2a688
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
electron-builder.json5electron/ipc/handlers.tspackage.jsonscripts/benchmark-export-queues.mjsvite.config.ts
electron/ipc/handlers.ts
Outdated
| if (app.isPackaged) { | ||
| return ffprobeStatic.replace(/\.asar([\/\\])/, '.asar.unpacked$1') | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant lines
if [ -f "electron/ipc/handlers.ts" ]; then
echo "File found. Reading lines 1190-1210:"
sed -n '1190,1210p' "electron/ipc/handlers.ts"
else
echo "File not found at electron/ipc/handlers.ts"
# Try to find similar files
find . -name "handlers.ts" -type f 2>/dev/null | head -5
fiRepository: webadderall/Recordly
Length of output: 695
🌐 Web query:
Biome regex character class escaping lint rules unnecessary escape forward slash
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
Remove unnecessary escape in regex character class.
Line 1198 has an unnecessary escape of the forward slash inside the character class. The forward slash is not a special character within character classes and doesn't need escaping.
Proposed fix
- return ffprobeStatic.replace(/\.asar([\/\\])/, '.asar.unpacked$1')
+ return ffprobeStatic.replace(/\.asar([/\\])/, '.asar.unpacked$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.
| if (app.isPackaged) { | |
| return ffprobeStatic.replace(/\.asar([\/\\])/, '.asar.unpacked$1') | |
| } | |
| if (app.isPackaged) { | |
| return ffprobeStatic.replace(/\.asar([/\\])/, '.asar.unpacked$1') | |
| } |
🧰 Tools
🪛 Biome (2.4.10)
[error] 1198-1198: The character doesn't need to be escaped.
(lint/complexity/noUselessEscapeInRegex)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 1197 - 1199, In the ffprobe path
adjustment inside the app.isPackaged branch, remove the unnecessary escape of
the forward slash in the regex character class used in ffprobeStatic.replace
(change the capture group from ([\/\\]) to ([/\\]) in the regex
/\.asar([/\\])/), leaving the rest of the replace call and logic
(ffprobeStatic.replace and app.isPackaged) intact.
scripts/benchmark-export-queues.mjs
Outdated
| import { promisify } from "node:util"; | ||
| import electron from "electron"; | ||
| import ffmpegStatic from "ffmpeg-static"; | ||
| import ffprobeStatic from "@derhuerst/ffprobe-static"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate the file and check its structure
fd -t f benchmark-export-queues.mjsRepository: webadderall/Recordly
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Read the file to verify the import, usage, and main() function
# Get file size first to ensure it's not too large
wc -l scripts/benchmark-export-queues.mjsRepository: webadderall/Recordly
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Read the import section and first part of the file
head -n 30 scripts/benchmark-export-queues.mjs | cat -nRepository: webadderall/Recordly
Length of output: 1952
🏁 Script executed:
#!/bin/bash
# Find the main function definition
grep -n "async function main\|function main" scripts/benchmark-export-queues.mjsRepository: webadderall/Recordly
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read around line 725 to see ffprobeStatic usage
sed -n '715,735p' scripts/benchmark-export-queues.mjs | cat -nRepository: webadderall/Recordly
Length of output: 866
🏁 Script executed:
#!/bin/bash
# Read the main() function (starting around line 825)
sed -n '825,900p' scripts/benchmark-export-queues.mjs | cat -nRepository: webadderall/Recordly
Length of output: 3150
🏁 Script executed:
#!/bin/bash
# Find the inspectOutput function
grep -n "function inspectOutput\|const inspectOutput" scripts/benchmark-export-queues.mjsRepository: webadderall/Recordly
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Read the inspectOutput function
sed -n '306,340p' scripts/benchmark-export-queues.mjs | cat -nRepository: webadderall/Recordly
Length of output: 1195
Add validation guard for ffprobeStatic in main() to fail fast when unavailable.
The ffprobeStatic import at line 10 is currently never validated in the main() function, unlike ffmpegStatic (line 826) and electron (line 830). When ffprobeStatic is invalid or unavailable, the inspectOutput() function silently catches the error and returns null, causing benchmark duration metrics to degrade without surfacing the setup failure.
Add this validation to the beginning of main():
if (typeof ffmpegStatic !== "string" || ffmpegStatic.length === 0) {
throw new Error("ffmpeg-static is unavailable for this platform");
}
+if (typeof ffprobeStatic !== "string" || ffprobeStatic.length === 0) {
+ throw new Error("@derhuerst/ffprobe-static is unavailable for this platform");
+}
+
if (typeof electron !== "string" || electron.length === 0) {
throw new Error("The Electron binary is unavailable in this workspace");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/benchmark-export-queues.mjs` at line 10, Add a fail-fast validation
for the imported ffprobeStatic at the start of main(): check that ffprobeStatic
is available (truthy) similar to the existing ffmpegStatic/electron guards, and
if missing log an error (use the same logger used elsewhere, e.g., processLogger
or console.error) and exit with a non-zero status (process.exit(1)) so
inspectOutput() can’t silently return null and hide a setup failure; reference
ffprobeStatic and main() when making the change.
|
This PR is great, but ffprobe isn't bundled. We only have ffmpeg-static. Easy fix is drop -f null - from the existing ffmpeg call so it reads the header instead of decoding full file |
- Remove -f null - from probeMediaDurationSeconds() ffmpeg args so the command always exits non-zero (no output file specified) - Without -f null -, ffmpeg exits code 1 and stderr lands in the catch block where Duration is parsed, fixing the bug where duration returned 0 on valid recordings - probeMediaDurationSeconds returning 0 caused the videoDuration > 0 gate to skip all audio sync correction, leaving system audio misaligned in the final recording
MohamedV0
force-pushed
the
fix/audio-sync-corruption
branch
from
f5f8388 to
0054beb
Compare
- Remove dead time= progress matching (no decode pass = no progress output) - Remove stale comments about ffmpeg success/fallback behavior - Lower timeout from 30s to 5s (header read is near-instant) - Drop maxBuffer override (minimal stderr output now) - Add -hide_banner to reduce stderr noise
2 participants
Summary
Fix system audio playing at wrong timestamps in recordings that use both system audio (Windows audio) and microphone. The root cause was a probing function that always returned duration 0, silently disabling all audio sync correction.
The Problem
When recording with both system audio and mic enabled, all system audio events — YouTube audio, notification sounds, UI clicks — appear at incorrect timestamps in the final recording and editor timeline.
How to reproduce
What you see before this fix
What you see after this fix
Why it happened
On Windows, WASAPI loopback (system audio capture) starts recording slightly after the video capture begins — typically 0.5-2 seconds late. The application is supposed to detect this delay and align the system audio track with the video. Instead, the duration detection function always returned 0, so the alignment step was silently skipped entirely. System audio and mic audio were mixed into the video with zero correction.
Root Cause
probeMediaDurationSeconds()usedffmpeg -i file -f null -which decodes every frame just to read the container duration. FFmpeg writes metadata to stderr and can return exit code 0 on valid files — the code only captured stderr inside thecatchblock, so when ffmpeg succeeded, stderr was discarded and duration always returned 0.Diagnostic logging confirmed:
This caused the
if (videoDuration > 0)gate inmuxNativeWindowsVideoWithAudio()to always be false, silently skipping all audio sync correction.hasEmbeddedAudioStream()had the same misuse — it decoded an entire audio frame just to check if an audio stream exists.Changes
probeMediaDurationSeconds()with ffprobe JSON parsing — reads container headers only (O(1), ~84ms vs ~70s for a 1-hour recording)hasEmbeddedAudioStream()with ffprobe-show_streams -select_streams a— metadata-only, zero frame decodinginspectOutput()inscripts/benchmark-export-queues.mjswith the same ffprobe approach@derhuerst/ffprobe-static@^5.3.0(same FFmpeg b6.1.1 build asffmpeg-static)Testing
app.asar.unpackedinspectOutput()returns correct duration valuesManual test — Windows 11 Pro (Build 26200):
npx vite build && npx electron-builder --win --dir[mux-win]log shows sync correction activeRisks & Impact
validateRecordedVideo()andparseFfmpegDurationSeconds()intentionally unchanged — they decode frames for codec validation (correct use of ffmpeg)wasapi_loopback.cppand is tracked separately.Checklist
loadFfmpegStatic()/getFfmpegBinaryPath()patternsSummary by CodeRabbit