Repository navigation
Add --alsa-mixer: hardware volume through an ALSA mixer control - #96
TermeHansen wants to merge 9 commits into
Conversation
--alsa-mixer <card:control> drives a hardware mixer for volume and mute instead of scaling the stream in software, mapped over the control's dB range the way alsamixer lays its scale out. The level the mixer is at at startup seeds the player and is shown in status until a server sends its own. A mixer that cannot be opened or set falls back to software gain; the flag is refused off the ALSA backend and alongside --fixed-volume. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
chrisuthe
left a comment
chrisuthe
left a comment
There was a problem hiding this comment.
Thanks for this. Hardware volume is a feature we want. I checked it against the player section of the spec. Two spec MUSTs are broken, so I'm requesting changes. The rest are suggestions.
Blocking
- A volume change can clear mute. When the control has no playback switch, mute falls back to software gain 0. The next server volume command then resets the multiplier to unity, and audio plays while we still report
muted: true. Details inline. - External mixer changes are never reported. We read the mixer once at startup. After that, a change from
amixer, alsamixer, another app or a hardware knob never reachesclient/state. Details inline.
Non-blocking suggestions
- Curve. The cubic curve is twice the spec's dB slope: −18.1 dB at 50 against −9.0 dB. Inline.
- Ramp. The spec says volume changes SHOULD ramp: "To avoid audible clicks, clients SHOULD apply volume changes over a short ramp." Mixer writes jump in one step, while the software path ramps through
current_multiplier_. A short stepped write would cover it, but we're fine either way. - Volume 0 is not silent. Inline.
Checked and conformant: supported_commands is unchanged. The value seeded from the mixer at startup goes out in the first client/state. Preferring the mixer level over the persisted one is fine, since persistence is only RECOMMENDED.
| void AlsaAudioSink::set_muted(bool muted) { | ||
| this->muted_.store(muted); | ||
| if (!this->mixer_spec_.empty() && set_mixer_mute_(this->mixer_spec_, muted)) { | ||
| this->target_multiplier_.store(Q32_ONE, std::memory_order_relaxed); |
There was a problem hiding this comment.
Same issue on the mute path. Once the switch has muted, unity is right here. Using the same muted_ ? 0 : Q32_ONE form in both places keeps the two paths from drifting apart.
…r watch Two spec MUSTs were broken. A volume change could clear a mute that fell back to software gain, because set_volume forced the PCM multiplier to unity regardless of muted_; the software gain is now derived from both fields, so mute survives it. And the mixer was read once at startup, so a change from amixer, alsamixer or a hardware knob never reached client/state; the sink now keeps the control open, watches its poll descriptors, and publishes a change through the player role. Also from the review: the volume-to-dB curve now follows the spec's 30*log10(v/100) instead of alsamixer's twice-as-steep slope (-9.0 dB at 50, not -18.1), volume 0 silences the stream even when the control's floor is not silent, mixer writes step over a short ramp, set_mixer_mute_'s doc matches its return, and a switchless control no longer reports muted. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
Resolve conflicts between --alsa-mixer and the Windows port plus --release-device that landed upstream. Both sides added independent enumerators, options, config keys, long options, help text, and tests, so every conflict is additive and keeps both features. - src/cli.h, src/cli.cpp: keep Opt::AlsaMixer and Opt::ReleaseDevice, both SettableOption rows, both config setters, both long options, both usage blocks, and both DAEMON_ONLY entries. - src/player_listener.h: keep on_sink_volume_changed() and release_device_between_streams(). - docs/wiki/Configuration.md: keep both option-table rows. - tests/cli_test.cpp, tests/config_file_test.cpp: keep both test groups. Also correct the --alsa-mixer help text, which still described the old alsamixer-shaped curve; the review commit moved it to the spec's curve. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
The flag shipped without a README row; add one next to --fixed-volume, matching the wiki's Configuration table. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
The hardware-mixer volume curve was hardcoded to the spec's 30*log10(v/100). --alsa-mixer-curve now picks between that and the steeper cubic curve (Snapcast's), whose inverse 100*10^(dB/60) is alsamixer's slider position, so the two track each other on a control with a wide range. Default stays spec; the flag needs --alsa-mixer, is settable from a config file, and rejects anything but spec or cubic. A control that mutes at its lowest step reports that step as SND_CTL_TLV_DB_GAIN_MUTE (-9999999), so its dB range came back as -99999.99 dB .. 0 dB. As the curve floor that collapsed every volume but 100% onto it and inflated the ramp step to the whole sentinel range. playback_dB_range_ now rebuilds the floor from the raw range and the per-step dB, which is where alsamixer gets the same numbers; a range without the sentinel is untouched. Verified on an IQaudIO DAC (card 1, Digital): the floor resolves to -103.50 dB instead of the sentinel, and the read-back matches (raw 153 -> -27.00 dB -> volume 12 spec / 35 cubic). Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
|
Two additions on top of the review feedback, both pushed to
|
Four issues on the mixer path, each reachable with a real control. A refused mixer write left the software gain at unity, so a control that rejected writes (or a switch write it would not take) kept playing at the old level. Track whether the control actually carries the level and the mute in mixer_carries_volume_/mixer_carries_mute_, cleared by a refused write and restored by one that succeeds; set from write_mixer_dB_, so a failure partway through a ramp is caught too. A newer volume request that took the immediate-write branch left any ramp in flight running, and the next poll resumed writing toward the older target. The same happened when an outside change was accepted. Cancel the pending ramp in both places. On a control with no playback switch, an outside dB change read muted as false and assigned it to muted_, audibly unmuting a stream the user had muted. Only a control with a switch can report a mute change, so leave muted_ alone when there is none, and report the preserved state. A mute-at-minimum control leaves mixer_reported_dB_ at the mute sentinel, so the next ramp started near -100,000 dB: every intermediate write sat at the mute step and then jumped. Clamp the ramp's start to the repaired range, leaving the reported value itself alone since it recognizes echoes. Verified on the IQaudIO DAC: the ramp from the mute step walks 0 -> 50 -> 101 -> 151 -> 201 instead of pinning at the mute step and jumping, and an outside change on the switchless Analogue control no longer clears the mute. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
report_mixer_change_() cancelled the volume ramp on any event, including a mute-only change. Muting mid-ramp stopped the mixer at an intermediate level, and unmuting then adopted that level as volume_ and reported it to the server, losing the level that was actually asked for. Cancel the ramp and adopt the level only for an outside volume change; a mute-only change leaves the ramp to finish at its target. A ramp step that failed cleared the carry flag but left the software multiplier at the unity a running ramp uses, so the level stayed wherever the control last accepted a write while software scaled nothing. Refresh the multiplier when a ramp is abandoned. open_mixer_() closed the handle on every failure path but left the caller's pointer dangling. The persistent mixer_ member then held freed memory and the destructor's mixer_close_() freed it again, so a typo in --alsa-mixer crashed the process on exit instead of falling back to software volume. Clear the pointer wherever the handle is closed. Verified on the IQaudIO DAC: an external mute mid-ramp leaves the ramp running and adopts the mute, a failed ramp step hands the level to the software gain (multiplier 3667107365, not unity), and an unopenable control exits cleanly instead of segfaulting in snd_mixer_close. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
README resolves in upstream's favour: the option table it replaced with wiki links is gone, and the Volume section points at --alsa-mixer in the Configuration wiki. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
|
@chrisuthe will you take look at this PR again? |
Summary
--alsa-mixer <card:control>drives a hardware mixer for volume and mute instead of scaling the stream in software, mapped over the control's dB range the way alsamixer lays its scale out.--fixed-volume.Testing
tests/cli_test.cppandtests/config_file_test.cppcover the flag parsing and config-file handling.