[GH-ISSUE #472] softmixer: Volume changes lead to clicks #298

Closed
opened 2026-02-27 19:29:53 +03:00 by kerem · 7 comments
Owner

Originally created by @v1ne on GitHub (May 2, 2020).
Original GitHub issue: https://github.com/librespot-org/librespot/issues/472

Scenario:
– Start Spotify client on Linux/Windows
– Select your librespot instance as playback device
– Start playback
– Click the volume slider in the client at different places

Expected:
– Volume changes

Actual:
– An audible click is produced sometimes and the volume changes

The patch below solves the issue, but contains unsafe code because AudioFilter has no access to mutable state and I'm not a Rust programmer. 0:-) Because of that, I didn't feel comfortable to make a pull request out of it.

commit 1a060cc46af83eaa46aed5cdaee37286d95eaf21 (HEAD -> dev)
Author: v1ne <v1ne2go@gmail.com>
Date:   Sat May 2 11:39:46 2020 +0200

    softmixer: Prevent clicks when changing volume abruptly
    
    If the user changes the playback volume, create a ramp to smooth out any
    discontinuities. Such an abrupt volume change happens for example if the
    user clicks on the volume slider in the Spotify client. Otherwise, the
    audio output contains noticeble clicks.

diff --git a/playback/src/mixer/softmixer.rs b/playback/src/mixer/softmixer.rs
index 85f08c4..f2a9695 100644
--- a/playback/src/mixer/softmixer.rs
+++ b/playback/src/mixer/softmixer.rs
@@ -1,3 +1,4 @@
+use std::cmp::min;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::Arc;
 
@@ -25,20 +26,72 @@ impl Mixer for SoftMixer {
     }
     fn get_audio_filter(&self) -> Option<Box<dyn AudioFilter + Send>> {
         Some(Box::new(SoftVolumeApplier {
-            volume: self.volume.clone(),
+            target_volume: self.volume.clone(),
+            state: Box::new(SoftVolumeState {
+                current_target_volume: self.volume.load(Ordering::Relaxed) as i32,
+                current_volume_scaled: 0,
+                volume_stepsize_scaled: 0,
+                volume_numsteps: 0,
+            }),
         }))
     }
 }
 
+struct SoftVolumeState {
+    current_target_volume: i32,
+    current_volume_scaled: i32,
+    volume_stepsize_scaled: i32,
+    volume_numsteps: usize,
+}
+
 struct SoftVolumeApplier {
-    volume: Arc<AtomicUsize>,
+    target_volume: Arc<AtomicUsize>,
+    state: Box<SoftVolumeState>,
 }
 
 impl AudioFilter for SoftVolumeApplier {
     fn modify_stream(&self, data: &mut [i16]) {
-        let volume = self.volume.load(Ordering::Relaxed) as u16;
-        for x in data.iter_mut() {
-            *x = (*x as i32 * volume as i32 / 0xFFFF) as i16;
+        let new_volume = min(
+            self.target_volume.load(Ordering::Relaxed) as i32,
+            0xFFFF as i32,
+        );
+
+        // create a ramp so that abrupt volume changes don't click:
+        let (ramped_samples, start_volume_scaled, target_volume, stepsize_scaled) = unsafe {
+            let mut_state =
+                &mut *((*((self as *const SoftVolumeApplier) as *mut SoftVolumeApplier)).state);
+
+            if new_volume != mut_state.current_target_volume {
+                mut_state.current_target_volume = new_volume;
+                mut_state.volume_numsteps = 44100 / 100; // 10 ms ramp duration @ 44100 S/s
+                mut_state.volume_stepsize_scaled = ((new_volume << 15)
+                    - mut_state.current_volume_scaled)
+                    / (mut_state.volume_numsteps as i32);
+            }
+
+            let retval = (
+                mut_state.volume_numsteps,
+                mut_state.current_volume_scaled,
+                mut_state.current_target_volume,
+                mut_state.volume_stepsize_scaled,
+            );
+
+            let num_steps_this_buffer = min(data.len(), mut_state.volume_numsteps);
+            mut_state.current_volume_scaled +=
+                (num_steps_this_buffer as i32) * mut_state.volume_stepsize_scaled;
+            mut_state.volume_numsteps -= num_steps_this_buffer;
+
+            retval
+        };
+
+        let mut volume_scaled = start_volume_scaled;
+        for x in data.iter_mut().take(ramped_samples) {
+            *x = ((*x as i32 * (volume_scaled >> 15)) / 0xFFFF) as i16;
+            volume_scaled += stepsize_scaled;
+        }
+
+        for x in data.iter_mut().skip(ramped_samples) {
+            *x = ((*x as i32 * target_volume) / 0xFFFF) as i16;
         }
     }
 }
Originally created by @v1ne on GitHub (May 2, 2020). Original GitHub issue: https://github.com/librespot-org/librespot/issues/472 Scenario: – Start Spotify client on Linux/Windows – Select your `librespot` instance as playback device – Start playback – Click the volume slider in the client at different places Expected: – Volume changes Actual: – An audible click is produced sometimes and the volume changes The patch below solves the issue, but contains unsafe code because `AudioFilter` has no access to mutable state and I'm not a Rust programmer. 0:-) Because of that, I didn't feel comfortable to make a pull request out of it. ```rust commit 1a060cc46af83eaa46aed5cdaee37286d95eaf21 (HEAD -> dev) Author: v1ne <v1ne2go@gmail.com> Date: Sat May 2 11:39:46 2020 +0200 softmixer: Prevent clicks when changing volume abruptly If the user changes the playback volume, create a ramp to smooth out any discontinuities. Such an abrupt volume change happens for example if the user clicks on the volume slider in the Spotify client. Otherwise, the audio output contains noticeble clicks. diff --git a/playback/src/mixer/softmixer.rs b/playback/src/mixer/softmixer.rs index 85f08c4..f2a9695 100644 --- a/playback/src/mixer/softmixer.rs +++ b/playback/src/mixer/softmixer.rs @@ -1,3 +1,4 @@ +use std::cmp::min; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; @@ -25,20 +26,72 @@ impl Mixer for SoftMixer { } fn get_audio_filter(&self) -> Option<Box<dyn AudioFilter + Send>> { Some(Box::new(SoftVolumeApplier { - volume: self.volume.clone(), + target_volume: self.volume.clone(), + state: Box::new(SoftVolumeState { + current_target_volume: self.volume.load(Ordering::Relaxed) as i32, + current_volume_scaled: 0, + volume_stepsize_scaled: 0, + volume_numsteps: 0, + }), })) } } +struct SoftVolumeState { + current_target_volume: i32, + current_volume_scaled: i32, + volume_stepsize_scaled: i32, + volume_numsteps: usize, +} + struct SoftVolumeApplier { - volume: Arc<AtomicUsize>, + target_volume: Arc<AtomicUsize>, + state: Box<SoftVolumeState>, } impl AudioFilter for SoftVolumeApplier { fn modify_stream(&self, data: &mut [i16]) { - let volume = self.volume.load(Ordering::Relaxed) as u16; - for x in data.iter_mut() { - *x = (*x as i32 * volume as i32 / 0xFFFF) as i16; + let new_volume = min( + self.target_volume.load(Ordering::Relaxed) as i32, + 0xFFFF as i32, + ); + + // create a ramp so that abrupt volume changes don't click: + let (ramped_samples, start_volume_scaled, target_volume, stepsize_scaled) = unsafe { + let mut_state = + &mut *((*((self as *const SoftVolumeApplier) as *mut SoftVolumeApplier)).state); + + if new_volume != mut_state.current_target_volume { + mut_state.current_target_volume = new_volume; + mut_state.volume_numsteps = 44100 / 100; // 10 ms ramp duration @ 44100 S/s + mut_state.volume_stepsize_scaled = ((new_volume << 15) + - mut_state.current_volume_scaled) + / (mut_state.volume_numsteps as i32); + } + + let retval = ( + mut_state.volume_numsteps, + mut_state.current_volume_scaled, + mut_state.current_target_volume, + mut_state.volume_stepsize_scaled, + ); + + let num_steps_this_buffer = min(data.len(), mut_state.volume_numsteps); + mut_state.current_volume_scaled += + (num_steps_this_buffer as i32) * mut_state.volume_stepsize_scaled; + mut_state.volume_numsteps -= num_steps_this_buffer; + + retval + }; + + let mut volume_scaled = start_volume_scaled; + for x in data.iter_mut().take(ramped_samples) { + *x = ((*x as i32 * (volume_scaled >> 15)) / 0xFFFF) as i16; + volume_scaled += stepsize_scaled; + } + + for x in data.iter_mut().skip(ramped_samples) { + *x = ((*x as i32 * target_volume) / 0xFFFF) as i16; } } } ```
kerem 2026-02-27 19:29:53 +03:00
Author
Owner

@v1ne commented on GitHub (May 2, 2020):

@ashthespy: Maybe that's also something for Vollibrespot.

<!-- gh-comment-id:622929818 --> @v1ne commented on GitHub (May 2, 2020): @ashthespy: Maybe that's also something for Vollibrespot.
Author
Owner

@ashthespy commented on GitHub (May 9, 2020):

@v1ne Thanks, will have a look :-)

<!-- gh-comment-id:626142972 --> @ashthespy commented on GitHub (May 9, 2020): @v1ne Thanks, will have a look :-)
Author
Owner

@roderickvd commented on GitHub (Mar 12, 2021):

I cannot reproduce this on my setup. One is a RPi 3B+ with Alsa backend feeding into an external DAC over I2S, the other Rodio on macOS. What does your setup look like?

<!-- gh-comment-id:797804574 --> @roderickvd commented on GitHub (Mar 12, 2021): I cannot reproduce this on my setup. One is a RPi 3B+ with Alsa backend feeding into an external DAC over I2S, the other Rodio on macOS. What does your setup look like?
Author
Owner

@v1ne commented on GitHub (Mar 12, 2021):

I'm using a Raspberry Pi 3B Rev. 1.2 with a HiFiBerry Digi (rewired to work on RPi 3). That is connected via an optical cable to my Nubert NuPro X-4000 speakers.
At this point, I used the ALSA backend. But because of other issues (stream stopping/starting without a grace period in between songs, leading to other discontinuities, if I remember correctly), I had to switch to the PulseAudio backend.

<!-- gh-comment-id:797809771 --> @v1ne commented on GitHub (Mar 12, 2021): I'm using a Raspberry Pi 3B Rev. 1.2 with a HiFiBerry Digi (rewired to work on RPi 3). That is connected via an optical cable to my Nubert NuPro X-4000 speakers. At this point, I used the ALSA backend. But because of other issues (stream stopping/starting without a grace period in between songs, leading to other discontinuities, if I remember correctly), I had to switch to the PulseAudio backend.
Author
Owner

@roderickvd commented on GitHub (May 24, 2021):

If you use amixer sset '{name}' {x}% on the command line, back and forth between various x volumes, do you experience the same issue? (I think you should, there is nothing else going on in librespot softvol than what Alsa does.

You can find out the name of your mixer control with amixer scontrols. I'm not sure if the HifiBerry Digi has hardware volume control. If not, you'll need to set up Alsa Softvol for this card to perform this experiment.

As much as I appreciate your idea of ramping up and down, this is not normal for a volume control to do. So in expectation you'll experience the same issue with amixer, I'm closing this for now. Feel free to re-open if the issue persists.

<!-- gh-comment-id:847337091 --> @roderickvd commented on GitHub (May 24, 2021): If you use `amixer sset '{name}' {x}%` on the command line, back and forth between various `x` volumes, do you experience the same issue? (I think you should, there is nothing else going on in `librespot` `softvol` than what Alsa does. You can find out the name of your mixer control with `amixer scontrols`. I'm not sure if the HifiBerry Digi has hardware volume control. If not, you'll need to set up Alsa `Softvol` for this card to perform this experiment. As much as I appreciate your idea of ramping up and down, this is not normal for a volume control to do. So in expectation you'll experience the same issue with `amixer`, I'm closing this for now. Feel free to re-open if the issue persists.
Author
Owner

@v1ne commented on GitHub (May 24, 2021):

Hi. I just tried this with the Spotify client on Linux, remote-controlling Spotify on Windows 10 20H2 with a ZOOM UAC-2 audio interface: No audible clicks at all.
Changing the volume in large steps in Windows: no clicks either.
Changing the volume in large steps on my Android smartphone: no clicks either.

As a DAW developer, I assure you that audible clicks are not expected if the user changes the gain. This can be fine in some internal node, but at least when you expose such a parameter to the user, you have to take precautions to avoid clicks. The same way it is unacceptable from a UX perspective to produce clicks when switching tracks, starting playback, stopping playback, seeking, …

<!-- gh-comment-id:847360648 --> @v1ne commented on GitHub (May 24, 2021): Hi. I just tried this with the Spotify client on Linux, remote-controlling Spotify on Windows 10 20H2 with a ZOOM UAC-2 audio interface: No audible clicks at all. Changing the volume in large steps in Windows: no clicks either. Changing the volume in large steps on my Android smartphone: no clicks either. As a DAW developer, I assure you that audible clicks are not expected if the user changes the gain. This can be fine in some internal node, but at least when you expose such a parameter to the user, you have to take precautions to avoid clicks. The same way it is unacceptable from a UX perspective to produce clicks when switching tracks, starting playback, stopping playback, seeking, …
Author
Owner

@roderickvd commented on GitHub (May 25, 2021):

I know, and if that was due to librespot then yes that would be broken.

Did you try my debugging steps above?
Also have you tried running librespot --mixer alsa?

I'm not saying it's not important, I'm saying:

  1. no one else so far can reproduce it even with some rigorous testing in the upcoming #685
  2. there's no ramping in Alsa Softvol either

w.r.t. point 1, if this was a wide-spread issue then rest assured there would be a lot of complaints and issue reports.

Hardware this has been tested on:

  • HiFiBerry DAC+ Pro
  • HiFiBerry DAC+ Zero
  • Soekris dam1121 over I2S
  • JDS Labs OLDAC
  • iMac (2017)

So there's strong evidence it's not due to librespot, but if your debugging finds it is then we can look into it again.

<!-- gh-comment-id:847600417 --> @roderickvd commented on GitHub (May 25, 2021): I know, and if that was due to `librespot` then yes that would be broken. Did you try my debugging steps above? Also have you tried running `librespot --mixer alsa`? I'm not saying it's not important, I'm saying: 1. no one else so far can reproduce it even with some rigorous testing in the upcoming #685 2. there's no ramping in [Alsa Softvol](https://github.com/alsa-project/alsa-lib/blob/master/src/pcm/pcm_softvol.c) either w.r.t. point 1, if this was a wide-spread issue then rest assured there would be *a lot* of complaints and issue reports. Hardware this has been tested on: - HiFiBerry DAC+ Pro - HiFiBerry DAC+ Zero - Soekris dam1121 over I2S - JDS Labs OLDAC - iMac (2017) So there's strong evidence it's not due to `librespot`, but if your debugging finds it is then we can look into it again.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/librespot#298
No description provided.