Repository navigation
feat(gpui): normalize exported audio to -14 LUFS - #2777
YoavLevavi wants to merge 4 commits into
Conversation
Adds the loudness pass (cap_export::loudness) and an audio setting to run it after MP4 and MOV exports from the GPUI editor. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@YoavLevavi is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
| path = %path.display(), | ||
| "Normalizing export loudness" | ||
| ); | ||
| let temp_path: PathBuf = path.with_extension(format!( |
There was a problem hiding this comment.
P2: Predictable loudness temp file lets a local attacker redirect the export rewrite through a symlink
Loudness scratch file uses predictable <output>.loudness.<ext>; a pre-planted symlink is followed and the target truncated.
Use tempfile in the destination directory and persist after inode verification instead of a predictable with_extension path.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="crates/export/src/loudness.rs">
<violation number="1" location="crates/export/src/loudness.rs:307">
<priority>P2</priority>
<title>Predictable loudness temp file lets a local attacker redirect the export rewrite through a symlink</title>
<evidence>normalize_file_loudness derives its scratch path as `path.with_extension("loudness.<ext>")` (lines 307-310), so the name is fully predictable from the chosen export filename. rewrite_audio then calls `format::output(temp_path)` on that path, which opens/truncates it through any pre-existing symlink, and the result is later renamed over the export. The export directory is normally selected by the user and can be a shared or world-writable location (for example /tmp or a shared volume). Another local user who can create entries there can pre-create `<name>.loudness.mp4` as a symlink to any file the exporting user can write; the loudness pass opens it with victim privileges and truncates/overwrites that target with encoded media. The same predictable name also silently clobbers any unrelated existing `<name>.loudness.<ext>` file. The existing export code already avoids this class by staging through tempfile's randomized names and verifying the staged inode was not replaced (crates/export/src/mp4.rs:1189-1215 and :1161-1177, present at the merge-base), but the new loudness scratch file does not use that mechanism.</evidence>
<recommendation>Create the scratch file with tempfile::Builder::new().tempfile_in(the export's parent directory) (or reuse the existing TemporaryExportOutput/prepare_new_output helper), keep the open file handle, write through it, and persist/replace the destination only after verifying the same inode was not swapped. Do not derive a fixed path with with_extension, and add a Unix regression test showing a pre-existing symlink at the old predictable location is not followed.</recommendation>
</violation>
</file>
|
Thanks for porting this to GPUI @YoavLevavi, the toggle and the default-off serde field look good and existing exports stay untouched. A few things before we can merge: 1) |
- The gain search encodes only the audio, into a private temp directory; the video is remuxed once at the end, interleaved by decode time. - The rewritten export is written to a tempfile::Builder file in the destination directory (exclusive, random name), verified to still be that same file, then persisted over the export. The old predictable <name>.loudness.<ext> path is gone. - Every pass and streaming loop checks the cancel flag; a cancelled run leaves the export untouched and the GPUI export ends as cancelled. - GPUI shows a "Normalizing audio" phase (cancellable) after rendering. - Tests: no scratch left and a file with the old name is untouched; cancel keeps the export byte-identical; a click stays within 1ms after normalization. An ignored test (CAP_LOUDNESS_AV_FILE) checks a real export's video timeline and audio offset. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks for the quick and thorough update @YoavLevavi, this addresses almost everything: the scratch file is now a random |
|
Thanks @richiemcilroy, all four are addressed in f2194c1:
|
- Merges origin/main, folding its latest-progress coalescing into the ExportProgress loop. - tempfile makes the scratch owner-only, so the original export's permissions are copied onto it before it replaces the export (asserted in the scratch-name test). - The A/V check keeps the source extension, so MOV exports are tested as MOV. Drops the doc comments on ExportProgress and ExportPhase::Normalizing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks @richiemcilroy, both are done in 74f255a (also merged main):
I also dropped the doc comments on @greptileai review |
| if shortfall.abs() < MIN_ADJUSTMENT_DB || (next - *gain).abs() < f32::EPSILON { | ||
| return Ok(true); | ||
| } | ||
| *gain = next; |
There was a problem hiding this comment.
Returned gain was never applied
When all three passes finish without reaching the target, render_at_target updates gain after the last render. normalize_file_loudness then returns that new value, although the saved audio used the previous one. This makes callers and manual test output report the wrong adjustment. Keep the last applied gain separate from the next proposed gain.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/export/src/loudness.rs
Line: 517
Comment:
**Returned gain was never applied**
When all three passes finish without reaching the target, `render_at_target` updates `gain` after the last render. `normalize_file_loudness` then returns that new value, although the saved audio used the previous one. This makes callers and manual test output report the wrong adjustment. Keep the last applied gain separate from the next proposed gain.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Fixed in c10f698: on the last pass @greptileai review |
GPUI port of #2379 (the Tauri version).
cap_export::loudness: measures integrated loudness and true peak, then renders the export's audio to the -14 LUFS social target (gain capped, peak-limited), replacing the file in place.AudioConfiguration::normalize_loudness(defaults off, old projects deserialize unchanged).run_exportruns the pass after MP4 and MOV exports. GIF and cursor-only exports skip it. A failed pass keeps the unnormalized export and logs a warning.Testing:
cargo test -p cap-export --lib loudness,cargo test -p cap-project --lib,cargo checkinapps/desktop-gpui.A/V sync check (
CAP_LOUDNESS_AV_FILE=<file> cargo test -p cap-export --lib normalizing_a_real_export_keeps_av_sync -- --ignored --nocapture). Each run normalizes a copy, compares every video packet's pts/dts (µs) and size, and cross-correlates the decoded audio before and after:The MOVs end 64 samples (1.3 ms) longer because of AAC end padding; the start is sample-exact.
🤖 Generated with Claude Code


Confidence Score: 5/5
The reviewed update appears safe to merge.Summary
Adds optional export audio normalization to -14 LUFS.
Reviews (2) · Last reviewed commit: "Report the gain the saved audio was rend..." · Reviewed by Greptile