Conversation
There was a problem hiding this comment.
Pull request overview
Adds a “Lossless Audio” recording option to the visualiser’s recording settings, intended to control how audio is encoded when muxing audio + video via FFmpeg.
Changes:
- Introduces a new
losslessAudioboolean setting and UI toggle in recording settings. - Updates live recording mux command to use codec args derived from recording settings.
- Adjusts container extension selection (MOV for H.264/H.265/ProRes; MP4 for VP9) and disables Lossless Audio when VP9 is selected.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Source/visualiser/VisualiserComponent.cpp | Uses recording settings to decide FFmpeg audio codec arguments during mux. |
| Source/visualiser/RecordingSettings.h | Adds losslessAudio param + XML load hook; adds audio codec argument helper; adjusts file extension mapping. |
| Source/visualiser/RecordingSettings.cpp | Adds Lossless Audio toggle to UI and disables it based on Record Audio + codec constraints. |
| Source/visualiser/OfflineAudioToVideoRenderer.cpp | Leaves offline mux path on AAC with a TODO about lossless audio support. |
| if (auto* losslessAudioXml = settingsXml->getChildByName("losslessAudio")) { | ||
| losslessAudio.load(losslessAudioXml); | ||
| } |
There was a problem hiding this comment.
losslessAudio is loaded from the project XML, but it is never saved in RecordingParameters::save(), so the setting won't persist across project reloads. Add a losslessAudio.save(...) entry alongside the other recording settings.
| if (parameters.losslessAudio.getBoolValue()) { | ||
| return "-c:a pcm_s16le"; | ||
| } else { | ||
| return "-c:a aac -b:a 384k"; | ||
| } |
There was a problem hiding this comment.
getAudioCodecSettings() uses -c:a pcm_s16le for “Lossless Audio”. This downconverts from the current 32-bit WAV recording (see AudioRecorder using 32-bit WAV) and may clip/lose precision, so it’s not truly lossless vs the source. Consider using a 32-bit PCM format (or matching whatever bit-depth the recorder writes), and also guard against codecs/containers that can’t mux PCM (e.g., VP9/mp4) by forcing AAC in those cases even if the flag is set via XML/automation.
| if (parameters.losslessAudio.getBoolValue()) { | |
| return "-c:a pcm_s16le"; | |
| } else { | |
| return "-c:a aac -b:a 384k"; | |
| } | |
| const auto codec = parameters.videoCodec; | |
| const bool wantsLossless = parameters.losslessAudio.getBoolValue(); | |
| // Only treat audio as "lossless" when using a container/codec combination | |
| // that can safely mux PCM. For others (e.g. VP9/mp4), fall back to AAC. | |
| const bool containerSupportsPcm = | |
| #if JUCE_MAC | |
| (codec == VideoCodec::ProRes) || | |
| #endif | |
| (codec == VideoCodec::H264 || codec == VideoCodec::H265); | |
| if (wantsLossless && containerSupportsPcm) | |
| { | |
| // Use 32-bit float PCM to avoid down-converting from the 32-bit WAV source. | |
| return "-c:a pcm_f32le"; | |
| } | |
| // Default: AAC with a high bitrate for broad compatibility. | |
| return "-c:a aac -b:a 384k"; |
| args.addArray({"-c:a", "aac"}); // TODO incorporate lossless audio changes | ||
| args.addArray({"-b:a", "384k"}); |
There was a problem hiding this comment.
Offline muxing still hard-codes AAC (-c:a aac -b:a 384k), so the new Lossless Audio setting won’t apply to offline renders and will behave inconsistently with the live recorder mux. Consider plumbing RecordingSettings into this path (e.g., selecting PCM vs AAC based on the same setting, and respecting container/codec compatibility).
|
Added as part of a separate commit here 1770460 after addressing the comments, with you as a co-author so you have full credit! Thanks so much again for adding this! |
Add Lossless Audio toggle
Testing done:
Did not do: