Repository navigation
Add --alsa-mixer: hardware volume through an ALSA mixer control - #96
TermeHansen wants to merge 8 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.
| this->volume_.store(volume > 100 ? 100 : volume); | ||
| // Hardware first: a mixer that takes the value leaves the PCM scaling at unity. | ||
| if (!this->mixer_spec_.empty() && set_mixer_volume_(this->mixer_spec_, volume)) { | ||
| this->target_multiplier_.store(Q32_ONE, std::memory_order_relaxed); |
There was a problem hiding this comment.
Blocking (spec MUST). The spec says: "volume and muted are independent: a volume change ... MUST NOT clear the mute state."
When the control has no playback switch, set_mixer_mute_ returns false and set_muted(true) falls back to update_target_multiplier_(), so the target is 0. The next set_volume succeeds on the mixer, and this line stores Q32_ONE. Audio is audible again while the player still reports muted: true.
| this->target_multiplier_.store(Q32_ONE, std::memory_order_relaxed); | |
| this->target_multiplier_.store(this->muted_.load() ? 0 : Q32_ONE, std::memory_order_relaxed); |
| 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.
| static uint8_t volume_for_mixer_dB_(long min, long max, long dB); | ||
| /// @return true when the control took the volume; false leaves the caller on software gain. | ||
| static bool set_mixer_volume_(const std::string& mixer_spec, uint8_t volume); | ||
| /// @return true when the control took the mute, or has no switch for the caller to fall back. |
There was a problem hiding this comment.
This doc says set_mixer_mute_ returns true when the control has no switch, but the code returns false (has_playback_switch && ...). The software fallback in set_muted relies on false, so I'd fix the comment. Either way, please make them agree.
| if (elem == nullptr) { | ||
| return -1; | ||
| } | ||
| muted = !playback_switch_(elem); |
There was a problem hiding this comment.
Related: for a control without a switch, playback_switch_ returns false, so this sets muted = true. The header says "false without one". A player on a switchless control then starts muted and reports muted: true. Combined with the issue in set_volume, it then unmutes on the first volume command. Something like muted = snd_mixer_selem_has_playback_switch(elem) && !playback_switch_(elem); would fix it.
| // hardware value is what a controller should show until the server sends its own. | ||
| bool mixer_muted = false; | ||
| if (!opts.alsa_mixer.empty()) { | ||
| const int mixer_volume = AlsaAudioSink::read_mixer_volume(opts.alsa_mixer, mixer_muted); |
There was a problem hiding this comment.
Blocking (spec MUST). The spec says: "State updates MUST be sent whenever a field in the player state object changes, including when the volume was changed through a server/command or via device controls."
This is the only time we read the mixer. A change from amixer, alsamixer, another app or a hardware knob afterwards never reaches the player state, so controllers show a stale volume and mute.
We need to watch the mixer. Keep a handle open and add its snd_mixer_poll_descriptors to a loop that calls snd_mixer_handle_events, with an elem callback (a periodic re-read would also work). On a change, map dB back to 0-100 and push the volume and mute through the player so client/state goes out. Skip echoes of our own writes.
| if (volume == 0) { | ||
| return min; | ||
| } | ||
| // alsamixer's curve: linear in perceived loudness between the control's endpoints, not in |
There was a problem hiding this comment.
Suggestion (spec SHOULD, non-blocking). The spec curve is amplitude = (v/100)^1.5, which is 30·log10(v/100) dB. The alsamixer/Snapcast mapping here works out to 60·log10(v/100) dB, twice the slope. At volume 50 that is −18.1 dB, against −9.0 dB in the spec. A --alsa-mixer player would be 9 dB quieter than a software-volume player in the same group, and the level jumps when we fall back to software gain, since pcm_volume.h uses ^1.5.
The cubic curve is the Linux norm (alsamixer, PulseAudio, PipeWire), so we won't block on it. If you're open to it, the spec curve in dB is max + 3000·log10(v/100) in 0.01 dB units, clamped to min, with the inverse for read-back. Also, alsamixer itself uses a linear-in-dB scale for controls with a range of 24 dB or less, and this mapping doesn't.
| if (volume >= 100) { | ||
| return max; | ||
| } | ||
| if (volume == 0) { |
There was a problem hiding this comment.
Suggestion (non-blocking). Volume 0 maps to the control's dB floor. That is only silent when the floor is SND_CTL_TLV_DB_GAIN_MUTE. Many controls bottom out at something like −60 dB, which is still audible. The spec maps volume 0 to amplitude 0, where "0 is silent". Consider also turning the switch off, or using software gain 0, at volume 0.
…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>
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.