Add --progressive-volume option. Increase volume slowly at low level,…

[thekr1s]

… faster at higher level

I use a Raspberry pi with a hifiberry. I installed run OSMC (includes Kodi) on it and added librespot. The volume control of librespot is very dfferent from Kodi. In the first 5 percent of the volume level in librespot, the volume increases very fast to about the same level Kodi has at 50%. That is what I try to solve here.
I'm not a mathematic, so I implemented some 'by trial' algorithm.

Note: I did not know how to test the handle_volume_up() and handle_volume_down() functions in spirc.rs.

[alin23]

Hi @ComlOnline @plietar 😇
I hope you don't mind me stepping up here. I've been following this project for a while and using it daily. I just thought you could use some help on the reviews.

@thekr1s
You have some formatting issues, try using rustfmt so that your code style matches the project's code style.

Also I don't really understand the idea here and its usefulness, maybe try to explain it with some examples and why you think this is better than the current implementation.

I like the idea of revising the volume control and adding some features so it fits better with the system volume, I just don't quite grasp your ideas here.

[alin23]

I think it would be a good idea to move these values (3, 42) into properly named constants.
Maybe something like:

const PROGRESSIVE_VOLUME_STEP_LOW: u32 = 3
const PROGRESSIVE_VOLUME_STEP_HIGH: u32 = 42

[alin23]

Typo: vulume should be volume

[thekr1s]

Thanks @alin23, I'll process this comments. Note I have updated my initial comment on this pull request. Hope it clears up you questions

[cortegedusage]

Hi all, I was also thinking of this, not being a programmer myself but using it at a daily basis. And following the project for quite some time now. This pull request seems to try to emulate some sort of logarithmic volume scale. I think it would be better to replace the linear scale with a logarithmic (natural) scale. I remember some discussion on this some time ago, but can't find it right now. 2018-01-26 9:05 GMT+01:00 Alin Panaitiu <notifications@github.com>:

…

***@***.**** commented on this pull request. Hi @ComlOnline <https://github.com/comlonline> @plietar <https://github.com/plietar> 😇 I hope you don't mind me stepping up here. I've been following this project for a while and using it daily. I just thought you could use some help on the reviews. @thekr1s <https://github.com/thekr1s> You have some formatting issues, try using rustfmt so that your code style matches the project's code style. Also I don't really understand the idea here and its usefulness, maybe try to explain it with some examples and why you think this is better than the current implementation. I like the idea of revising the volume control and adding some features so it fits better with the system volume, I just don't quite grasp your ideas here. ------------------------------ In src/spirc.rs <#10 (comment)> : > @@ -122,6 +124,33 @@ fn initial_device_state(config: ConnectConfig, volume: u16) -> DeviceState { }) } +fn volume_to_mixer(volume: u16, progressive: bool) -> u16 { + if progressive { + // Some by trail determined volume calculation algorithm. Start increasing slowly, + // then after 50% increase in bigger steps. + let d = volume / (std::u16::MAX / 100); + let mut v:u32 = 0; + let mut incr:u32 = 0; + for i in 0..d { + v += incr; + incr +=3; I think it would be a good idea to move these values (3, 42) into properly named constants. Maybe something like: const PROGRESSIVE_VOLUME_STEP_LOW: u32 = 3const PROGRESSIVE_VOLUME_STEP_HIGH: u32 = 42 ------------------------------ In src/spirc.rs <#10 (comment)> : > + if progressive { + // Some by trail determined volume calculation algorithm. Start increasing slowly, + // then after 50% increase in bigger steps. + let d = volume / (std::u16::MAX / 100); + let mut v:u32 = 0; + let mut incr:u32 = 0; + for i in 0..d { + v += incr; + incr +=3; + if i > 50 { + // Increase faster to reach max volume + incr += 42; + } + } + + // Clip the vulume to the max Typo: *vulume* should be *volume* — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#10 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AV69TaudmdFBqJ24mAIh08g83ANf9XWJks5tOYc5gaJpZM4RtmsH> .

[alin23]

@thekr1s I see your point now.
I don't think this is the way to go here. Your problem is a specific case tied to your environment, there's nothing wrong with librespot's way of controlling the volume as far as I can tell.
So hardcoding some values to make the volume go the way you need isn't going to help the other users.

I don't decide anything here though. I just think you should find the root cause for the problem and if it is really because of librespot, implement it as a universal fix, not a hardcoded one that works only for RPi with HiFiBerry and OSMC.

[thekr1s]

I am not the only one with this problem as can be concluded form this thread:
plietar/librespot#150 (comment).
Point three in the first message refers to the same issue. I also mentioned my fix in this thread and got some positive responses, I assume from people with the same issue.
I agree that additional investigation is necessary. In the mean time this workaround could be useful.

[ComlOnline]

I think if this bug still exists in this IOS app (gonna try and get my hands on an iPhone) it does need to be addressed. However I do agree with @cortegedusage if this were to be implemented a proper logarithmic volume control should be used as an option.

I also think @alin23 is correct in saying we need to find the root cause I can understand wanting a temporary fix however I don't think it should be on the master branch for now. If an alternate branch is made with these changes called for example "progressive volume" would that work for you in the interim?

Out of interest when librespot is at full volume is it too loud? (As in clipping and distorting). I have a pi3 on my desk I'll install OSMC over the weekend and have a play as well.

[alin23]

@thekr1s ok, that makes sense now. I remember having the same problem some time ago while changing the volume from the iPhone.

How about changing the volume_to_mixer method with an interpolation function that can be configurable?

Idea

Instead of a --progressive-volume flag we could have a --volume-scale-factor option that would take a float and interpolate the volume using the following formula:

normalized_volume = volume / std::u16::MAX  // To get a value between 0 and 1
volume = normalized_volume.powf(volume_scale_factor) * std::u16::MAX

Some Examples

--volume-scale-factor 2.0
--volume-scale-factor 4.5 (this fits your case)

Tips for the future (even if you don't like math)

What you have implemented inside volume_to_mixer is roughly a very inefficient Gauss sum.
You're doing something like:

• volume = 3 + 6 + 9 + ... + 150 if volume ≤ 50
• volume = 195 + 240 + 285 + ... + 2400 if volume > 50

...and you are using more than 50 cycles for something that can be done in a single line.

You could have narrowed that whole for loop to something like:

volume = if volume <= 50 {
    n = volume
    3 * ((n * (n + 1)) / 2)
} else {
    n = volume - 50
    (45 * n + 195) * ((n * (n + 1)) / 2)
}

WolframAlpha gives you really nice formulas for things like that: 195 + 240 + 285 + ... + 2400

[maxx]

This patch, in whatever final form it takes, is very important to me too. I mainly control with the iOS app and pipe out to snapcast via pulseaudio. We also use a lot of white noise type tracks on spotify for sleeping, and the control on the low end of the volume spectrum is way too choppy. The default scale is awkward.

@alin23 's approach seems reasonable too.

[thekr1s]

@ComlOnline The maximum volume is OK. It is about the same as the maximum volume of Kodi.
I don't mind having the volume modification in a separate branch.

@alin23 I prototyped your --volume-scale-factor solution in a spreadsheet. Looks like it could work althoug the volume increases very slowly in the first 40%. I'll test it and see how it behaves.
Your code optimization could work, but I don't see it. I prefer readable code above optimized code, as long as performance is no issue. But what's readable is a personal matter....

I'll do some experiments and get back to it.

[ComlOnline]

@thekr1s That does sound like a volume issue then.

It sounds like the lack of volume in the first 40% may be an issue with the scale factor. If you have a look at this graph you can play around with the scale factor (slider on the right) and see what looks good. From what I understood @alin23 was meaning to pass this as a variable rather than hard code it in, so it would just be a matter of finding what works best for you.

[alin23]

@ComlOnline Yes, I meant to pass the scale factor as a variable from the command line. There are all kinds of sound cards out there so letting the users fiddle with the volume scaling should make the problem solvable for everyone.
Nice website for checking the plot 👍 I didn't know about it.

@thekr1s
You're right, the scale factor gives much lower values than your approach for volume < 40%.

About the optimization, applying a single formula instead of doing 100 additions in a for loop is at least 1 order of magnitude faster. Usually people don't just set volume to a number in one go. They raise the volume fast from a lower volume and that volume_to_mixer operation gets repeated a pretty big number of times.

I am all for readability usually but what you are doing there is like writing a multiplication (2*50) as a repeated addition (2+2+2+...+2)

Anyway, seems like the scale factor implementation isn't too fast either (probably because of floating point operations), so applying a gauss sum might be the best choice.

You can see here a benchmark: Gauss volume control benchmark

And the results after cargo bench:

running 3 tests
test tests::raise_volume_with_for_loop     ... bench:       1,883 ns/iter (+/- 742)
test tests::raise_volume_with_formula      ... bench:         186 ns/iter (+/- 38)
test tests::raise_volume_with_scale_factor ... bench:       1,499 ns/iter (+/- 257)

For more flexibility, I think we can implement both ideas:

• --linear-volume-scale-factor factor with a default factor=1.0
• --gaussian-volume-scale-factors low_increment,high_increment,threshold with defaults
  • low_increment = 3
  • high_increment = 45
  • threshold = 50

[LeonCB]

I'm not sure if this will be helpful, but it's worth a read anyway:
https://www.dr-lex.be/info-stuff/volumecontrols.html

The core message you should take home is: volume must be exponential, or at least look like it!

[alin23]

@LeonCB thanks for the quality material! Well that's what I was advocating for with the linear-volume-scale-factor option. The formula is exactly the one explained in the article.
Although, now that I think about it, it should probably be named exponential-volume-scale-factor. What a mouthful!

But now I find that the gaussian method should be available too because some systems have very weird volume configuration.

We should also make the exponential curve default with a sensible factor. The 6.908 value found in the article is wild. Most of us don't listen to Spotify at 90dB to need that range. Also as you can see in this benchmark, using powf is pretty slow compared to powi:

running 2 tests
test tests::raise_volume_with_integer_scale_factor ... bench:         304 ns/iter (+/- 62)
test tests::raise_volume_with_scale_factor         ... bench:       1,513 ns/iter (+/- 373)

So I think we should go with a factor of 4 as default.

@thekr1s Let me know if you still want to finish this. If you want, I can take it from here and implement the ideas in a new PR.

[thekr1s]

I'm happy to implement it. I'll work on it this week.
I'll add the following options to make them less verbose (they fit in the table in the readme.md)
--exp-volume-scale [factor]
--gaus-volume-scale [low_increment,high_increment,threshold]

If none of the above command line options are given, the exponential volume scale will be used with factor 1.0 (= lineair isn't it?)
if --exp-volume-scale with no argument, factor 4.0 is used.
if --gaus-volume-scale is given with no arguments, the following defaults will be used:

• low_increment = 3
• high_increment = 45
• threshold = 50

[alin23]

@thekr1s sounds good! Only one thing, we should name --gaus-volume-scale as --gauss-volume-scale because the formula used is named after Carl Friedrich Gauss and it would be a shame to misspell his name ^_^

[ComlOnline]

Just so you know I moved the options table to a new Wiki you can add the option here when you're ready:
https://github.com/librespot-org/librespot/wiki/Options

[thekr1s]

I took a look in the XBMC source code. I made a volume calculation based on the code in XBMS and ends-up like below. I tested it, and sounds indeed quite like XBMC volume control.
@alin23 How does this mathematically relate to the other options?

fn volume_to_mixer_xbmc(volume: u16) -> u16 {

// Convert a volume percentage (as a proportion) to a dB gain
// We assume a dB range of 60dB, i.e. assume that 0% volume corresponds
// to a reduction of 60dB.
// value the volume from 0..0xffff
let db_range: f64 = 60.0;
let normalized_volume = volume as f64 / std::u16::MAX as f64;  // To get a value between 0 and 1
// Calculate the corresponding gain in dB from -60dB .. 0dB.
let gain = (normalized_volume - 1.0) * db_range;

// Convert gain to a scale factor for audio manipulation
let mut val = 0.0; 
// we need to make sure that our lowest db returns plain zero
if  gain > -60.0 {
    // Inverts gain = 20 log_10(scale)
    let ten: f64 = 10.0;
    val = ten.powf(gain/20.0); 
}

// in order to not introduce computing overhead for nearly zero
// values of dB e.g. -0.01 or -0.001 we clamp to top
if val >= 0.99 {
  val = 1.0;
}

val *= std::u16::MAX as f64;
info!("volume_to_mixer_xbmc {} {}", volume, val);	

// return the scale factor (0..0xffff) (equivalent to a voltage multiplier).
val as u16

}

[alin23]

@thekr1s nice find! 🥇

As we can see, XBMC implements exactly the algorithm for the ideal curve explained in the article posted by @LeonCB

We now know two points of our y = a·exp(b·x) curve, namely: (0, 30dB(A)) and (1, 90dB(A)). If we move to relative units, this translates to either (0, −60dB) and (1, 0dB) when working with the usual convention of attenuation levels. If we offset this by 60 dB we get (0, 0dB) and (1, 60dB), making our calculations somewhat more intuitive. Given that we work with amplitudes, 60 dB is 1060/20 = 1000 times the amplitude of 0 dB. Hence 1000 = exp(b·1) and b = ln(1000) = 6.908. The value of a is simply 1/1000.

But if we already know our range (60dB) we can precompute some things and reduce the algorithm implementation to the following:

const IDEAL_FACTOR: f64 = 6.908;

fn exponential_volume(volume: u16) -> u16 {
    let normalized_volume = volume as f64 / std::u16::MAX as f64; // To get a value between 0 and 1
    let new_volume = (normalized_volume * IDEAL_FACTOR).exp() / 1000.0;

    (new_volume * std::u16::MAX as f64) as u16
}

Which as you can see here, is pretty fast:

running 5 tests
test tests::raise_volume_with_for_loop             ... bench:       1,837 ns/iter (+/- 296)
test tests::raise_volume_with_formula              ... bench:         185 ns/iter (+/- 32)
––> test tests::raise_volume_with_ideal_factor     ... bench:         718 ns/iter (+/- 104)
test tests::raise_volume_with_integer_scale_factor ... bench:         298 ns/iter (+/- 105)
test tests::raise_volume_with_scale_factor         ... bench:       1,410 ns/iter (+/- 176)

If everyone agrees, we can chop down on the complexity of having 2 more obscure command line args, and make this implementation the default for everyone.

@ComlOnline @thekr1s what would you vote for?

1. Exponential volume with factor 6.908 as default
2. Linear volume as default with the addition of --exp-volume-scale and --gauss-volume-scale

I vote for 1

[ComlOnline]

I like it! I would just get a bunch of people to test it to make sure it sounds OK before we merge it but I see no reason not to do that. Could people vote and +1 for option 1 and -1 for option 2 on @alin23 post.

@sashahilton00 @cortegedusage @LeonCB @maxx Could you spare a second and look at the above?

Thanks

[thekr1s]

Votes from Les Pays-Bas
+1 for option 1
-1 for option 2

[sashahilton00]

I'd vote for 1 given the choice, but given that we've been discussing making an 'easy to use' librespot daemon in a new repo, I wouldn't be against adding another command line argument for the library, as there will undoubtedly be those that prefer one over another for whatever reason. However, exponential is clearly better for most people, hence I'd vote for exponential by default, with a --linear-volume runtime flag for those that want linear scaling.

[LeonCB]

My vote is option 1.

Exponential should be default. Maybe with an option to change the factor if 6.908 is to steep for some people.

[cortegedusage]

+1 for 1

[sashahilton00]

So, conclusion? I seem to be the only one interested in maintaining backward compatibility for linear volume, and I'm not planning to use that myself, so why don't we just dump it. Are we happy with exponential volume as @alin23 proposes we implement it, with a runtime flag --volume-factor for those that want a factor other than 6.908?

[ComlOnline]

I think your correct in that it would be good to maintain it but I don't see a single situation where it could be useful. So just drop it and do as you and @alin23 have suggested with a default of 6.908 and have the run time flag.

[sashahilton00]

Fair enough. @alin23 are you ok to turn it into a PR?

[thekr1s]

OK, I'll implement option 1.
I have no problem implementing the --volume-factor option, but I prefer not to do. I have learned not to implement thing that we think that someone maybe might want to use in the future. In most cases it's never used but will sit there forever...

[sashahilton00]

@thekr1s fair enough, leave it out. we can add it later if we need to.

[thekr1s]

as agreed..

In the end only one file modified, little impact.

[sashahilton00]

LGTM, If anyone has any thoughts, please weigh in, otherwise I'll merge this tomorrow.

[alin23]

Looks good to me too ^_^

[Gongreg]

Hey, @sashahilton00, @thekr1s .
I know this PR is quite old but I just migrated from original project to this fork.
I am using Raspberry pi 3 and controlling the volume through either mac or windows spotify player. Now the first 90% of volume almost doesnt differ and the last 10% are responsible for the volume. It makes it look quite silly when you try to control the volume and a single percent has su a huge impact. At least adding back the flag to revert the behaviour would be welcome. Also @thekr1s, relevant xkcd https://xkcd.com/1172/ . If you change something without a way to keep using old flow you will always break it for somebody.

[kingosticks]

What do you mean? This new functionality is behind a flag...

Edit: sorry, got my issue crossed! Your point stands.

[thekr1s]

I was afraid of that. I will add an option to get the old volume control back.

[Gongreg]

@thekr1s, It is all good. Things sometime happen. I understand that for some people this PR was necessary. :) Thank you in advance.

[sashahilton00]

Knew there was someone out there using linear volume still ;) Are you ok to create the PR or do you want me to?

[Gongreg]

Never wrote a single line of rust :)
If you have some free time it would be faster for you. If not I will try to find some time in the next few days :)

[sashahilton00]

I actually meant @thekr1s :) @thekr1s if you are creating the PR can you move the volume level logic into a function something like get_volume_level which returns the volume level depending on whether the user wants linear or logarithmic, so we can just have one check for the runtime flag in that function, and not a bunch of them wherever there is a volume_to_mixer call.

[thekr1s]

I'll create a PR and look into this

[thekr1s]

I have created issue #187 for this