fix(audio): remove Independent Volume instead of writing system volume - #867
fix(audio): remove Independent Volume instead of writing system volume#867postoso wants to merge 1 commit into
Conversation
Greptile SummaryThe PR removes Independent Volume and the CoreAudio system-volume mutation path, leaving transcription cues controlled solely through
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40dd7c5e56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The cap is what keeps the audible result at or below the current | ||
| // output: `output * min(1, desired / output)` is `min(output, desired)`. | ||
| return .play(playerVolume: min(1, desired / output)) |
There was a problem hiding this comment.
On output devices whose CoreAudio scalar-to-decibel mapping is nonlinear, dividing desired by the hardware volume scalar does not produce the gain needed to reach the desired audible level: the hardware scalar and AVAudioPlayer.volume are different gain domains. For example, an output scalar of 0.8 and desired scalar of 0.4 need not correspond to a player gain of 0.5, so Independent Volume can play substantially above or below the selected level. Convert the device scalars to decibels using its CoreAudio conversion property, then derive the player gain from the dB difference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The scalar ratio is not valid across those gain domains. I'm removing the Independent
Volume path instead, so this compensation code and the exact-level claim go away.
| /// Independent Volume compensates the player toward the selected level instead | ||
| /// of moving the Mac output volume, so the current output stays the ceiling and | ||
| /// other audio is never touched. |
There was a problem hiding this comment.
When Independent Volume is enabled and system output is below the selected cue level, this new ceiling deliberately makes the cue quieter, but SettingsView.swift:375-377 still promises that the sound stays constant regardless of system volume and warns that playback temporarily changes system volume. Users therefore see a description that is now false in both directions; update that copy alongside this behavioral change.
Useful? React with 👍 / 👎.
|
CI note: the Build FluidVoice failure is |
|
Follow-up with a concrete main-branch comparison: the same failure predates this PR and appears unrelated to its changes. I could not find an open issue under the test name or |
|
This pull request has been marked stale because it has had no activity for 5 days. It will be closed in 2 days if there is no further activity. Add the |
|
Thanks for the PR @postoso . I still wonder if this is worth doing actually. Should we just remove the option by itself? having the sound cue louder than the audio itself seems a little off :/ No? |
|
I think removing it is the better call. Once the system-volume write is gone, Independent Volume Removing it still fixes #522, since the system-volume write only ran on that path. I'll revise this |
Independent Volume was the only code path that wrote the macOS output volume. It saved the current level, set the output to the selected cue level for the length of the cue, then restored it, which is what let a transcription cue change other applications' audio (altic-dev#522). Compensating app-side instead does not preserve what the option promised. The CoreAudio volume scalar and AVAudioPlayer.volume are different gain domains, so a player gain derived from their ratio does not land on the selected level, and it caps at full gain once the selected level is above the current output. Removing the option leaves one cue-volume slider, and nothing writes the system volume any more. It also removes four failure modes that lived in the save/restore path: a failed CoreAudio read was saved as 1.0 and restored as maximum, the restore re-resolved the default output so switching outputs mid-cue wrote one device's baseline onto another, the delayed restore had no termination handler, and overlapping cues shared one saved-volume slot. The Independent Volume settings copy is removed with the toggle. It described the old behaviour ("stays constant regardless of system volume", "temporarily changes system volume during playback") and was wrong in both directions either way.
40dd7c5 to
e19f498
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e19f498c6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let transcriptionStartSound: SettingsStore.TranscriptionStartSound | ||
| let transcriptionSoundVolume: Float | ||
| let transcriptionSoundIndependentVolume: Bool | ||
| let autoUpdateCheckEnabled: Bool |
There was a problem hiding this comment.
When a user exports a backup from this build and then restores it with the immediately preceding app version (for example, after downgrading), decoding fails because that version's synthesized SettingsBackupPayload decoder still requires transcriptionSoundIndependentVolume. The backup schema remains 1.0, so these files appear compatible but are reported as invalid JSON; retain the field in the encoded payload with a harmless fixed value, or introduce explicit schema migration/version handling.
Useful? React with 👍 / 👎.
postoso
changed the title
|
Pushed the revision, and updated the title and body to match. The Independent Volume toggle and the save/set/restore code are gone, so nothing in the app writes the Mac output volume any more, which is the part #522 was about. What is left is the single cue-volume slider: the cue plays at that level through The setting copy went with the toggle. It promised a level that stays constant regardless of system volume and warned that playback temporarily changes system volume, and both were wrong once the write is gone. I dropped the gain compensation and its tests rather than keep a reduced version of the option, since the compensation could not hold that promise anyway: the CoreAudio volume scalar and I rebased on current main while I was in there. The diff is four files, +6/-132, and CI is green including the test run, which also clears the |
2 participants
Description
You asked whether the option should just be removed rather than kept with app-side compensation. It should, so this PR now does that instead.
Independent Volume was the only code path that wrote the macOS output volume. It read the current level, set the output to the selected cue level for the length of the cue, then restored it on a timer. Writing the output volume is globally audible, and that is what #522 reports.
Compensating app-side, which is what this PR did before, does not preserve what the option promised. The CoreAudio volume scalar and
AVAudioPlayer.volumeare different gain domains, so a player gain derived from their ratio does not land on the selected level, and it caps at full gain once the selected level is above the current output. An option that only half keeps its promise is worse than no option.What is left is the single cue-volume slider. The cue plays at that level through
AVAudioPlayer, so it scales with system output like any other sound, and nothing in the app writes the system volume.Four latent bugs go with the removal, all of them in the save and restore path.
getSystemVolumereturned1.0on both failure paths, so a failed read was saved as the baseline and the restore drove the output to maximum. The restore re-resolved the default output device, so switching outputs mid-cue wrote one device's baseline onto another. The delayed restore had no termination handler, so quitting mid-cue left the device at the cue level. Overlapping cues shared onesavedSystemVolumeslot.The Independent Volume setting copy is removed with the toggle. It promised a level that stays constant regardless of system volume and warned that playback temporarily changes system volume, and both were wrong.
The tradeoff, stated plainly: if you kept system audio low and relied on an independently louder cue, the cue is now quieter. That capability cannot exist without moving everyone else's volume, which was the bug.
Type of Change
Related Issue or Discussion
Closes #522.
Testing
swiftlint --strict --config .swiftlint.ymlxcodebuild test -project Fluid.xcodeproj -scheme Fluid -destination 'platform=macOS,arch=arm64'swiftformat --config .swiftformat Sourcesswiftlint --strictover the whole repository reports 7 violations, alllegacy_swiftui_aspect_ratio, all in files this PR does not touch. A clean checkout ofmainreports the identical 7 on the same SwiftLint build, so they are not from this change.Locally I ran
xcodebuild build-for-testingrather than the full suite, so the test target compiles. CI ran the suite and it is green, which is also the first green Build on this branch: theDirectAudioReliabilityTestsfailure I flagged earlier is not present on this head.This PR removes the eight gain-policy tests it had added earlier, because the policy they covered no longer exists, and
DictationE2ETests.swiftis back to its state onmain. There is no pure seam left to test here and I would rather not add a test that asserts nothing.Rebased on current
main.Screenshots / Video
Settings sound section on current main; this PR removes the Independent Volume row.