Added log scale to AlsaMixerPlugin

[TermeHansen]

2 new keywords to mpd.conf:

---

-enable log scale instead of linear scale:
mixer_scale "log"
(everything else will use linear scale)

-extra log scale tuning parameter (used to make the scale fit alsamixer)
mixer_log_param "1.164"
(default is 1.0, 1.164 is found to make the scal fit with alsamixer)

** equations for the conversion between prc to log scale **
prc in log scale to volume level:
level = scale_log_param*(volume_max-volume_min)*(log(vol)/log(100.)-1. + volume_max+0.5

level to prc in log scale:
exp( log(100)(level-volume_max)/(scale_log_param(volume_max-volume_min)) + log(100))

---

[MaxKellermann]

Why?
Is this only relevant for ALSA, or might it be useful to make it a core feature?
Make scale an enum. Change string comparison to switch/case.

[TermeHansen]

I don't know if it only is relevant for Alsa, but I use mpd on an raspberry with an IQaudio DAC and needed this change to make it work more correctly with the volume in mpd.

So this is just a suggestion, that can be merged in if others could use it.

[MaxKellermann]

I have no idea if it's useful, but from what you just wrote, it sounds like a bug in the DAC device driver, and should be fixed there.

[TermeHansen]

Using amixer (libasound I guess) I get and set in the linear scale, which means that almost nothing is happening up until 50%. alsamixer however uses a scale that seem to be logarithmic, which makes more sense when changing the volume, so I wanted to make mpd behave like alsamixer on the volume scale.

>> amixer sget Digital
Simple mixer control 'Digital',0
  Capabilities: pvolume pswitch
  Playback channels: Front Left - Front Right
  Limits: Playback 0 - 207
  Mono:
  Front Left: Playback 123 [59%] [-42.00dB] [on]
  Front Right: Playback 123 [59%] [-42.00dB] [on]

[MaxKellermann]

If you want it to behave like alsamixer, then why does your code need explicit configuration, but alsamixer just works?

[TermeHansen]

I don't know why alsamixer "just works", as I (also) thought that alsamixer just was a nicer interface to libasound/amixer. But as my output from amixer and alsamixer shows above that is not the case.

You are probably right that the correct way would be to find the root of that difference and update libasound, but I think that is more difficult and a longer way for me than this change to the mpd mixer control.

[MaxKellermann]

Why don't you try to find out what alsamixer does differently? It would be desirable to improve MPD to "just work" with your hardware as well, and maybe there's an obvious trick we're missing.

[TermeHansen]

found this thread at runeaudio (http://www.runeaudio.com/forum/volume-control-linear-or-logarithmic-t254.html) and the -M flag of amixer does actually make it behave like alsamixer.

>> amixer set Digital 20%
  Front Left: Playback 42 [20%] [-82.50dB] [on]
  Front Right: Playback 42 [20%] [-82.50dB] [on]
>> mpc volume
volume:  4%
>> amixer set Digital 20% -M
  Front Left: Playback 123 [20%] [-42.00dB] [on]
  Front Right: Playback 123 [20%] [-42.00dB] [on]
>> mpc volume
volume: 20%

So do you have an idea of how I/we can make mpd interact with libasound by setting the prc (and hopefully also with something like the -M flag of amixer), and not by final level values as it does now where mpd calculates the linear level value from the user prc input.

[MaxKellermann]

This looks like MPD already does exactly what you want, doesn't it? It shows the same value as amixer -M.

[TermeHansen]

yes in my updated code with the log scale ;)

with the default linear calculation of level in mpd it would be:

>> amixer set Digital 20%
  Front Left: Playback 42 [20%] [-82.50dB] [on]
  Front Right: Playback 42 [20%] [-82.50dB] [on]
>> mpc volume
volume:  20%
>> amixer set Digital 20% -M
  Front Left: Playback 123 [20%] [-42.00dB] [on]
  Front Right: Playback 123 [20%] [-42.00dB] [on]
>> mpc volume
volume: 59%

[TermeHansen]

It seems like amixer with the set_playback_mapped_volume makes a log calculation to dB and then set sound based on dB

value = lrint_dir(6000.0 * log10(volume), dir) + max;
return set_dB[ctl_dir](elem, channel, value, dir);

http://git.alsa-project.org/?p=alsa-utils.git;a=blob_plain;f=amixer/amixer.c;hb=HEAD
http://git.alsa-project.org/?p=alsa-utils.git;a=blob;f=alsamixer/volume_mapping.c;hb=HEAD

[MaxKellermann]

That looks like something which could be useful for MPD.

[TermeHansen]

That's a more direct way than I did. I am starting to implement that instead in my fork...

[MaxKellermann]

Usually, I'd favor a general approach like the one you suggested. But in this case, a general approach is not needed, when the ALSA project itself has code for a general solution which does not need any configuration.

We could easily copy volume_mapping.c into MPD and use that in our ALSA mixer plugin.

[TermeHansen]

I have now implemented the volume_mapping from alsa-utils/alsamixer (with a few changes) and rewritten AlsaMixerPlugin to use those functions to get and set volume - so much simpler now...

Now mpd behaves as alsamixer on my setup.

Can you have a look at my changes and comment?

[MaxKellermann]

It's very hard to review your commits, because some of them add new code, the another commit replaces that new code with other code, then another one fixes a problem ... please fold those patches in a meaningful way.

[TermeHansen]

Branched my local development and committed to master only:
-Adding of the original volume_mapping files
-final rewrite of AlsaMixerPlugin and changes to volume_mapping and Makefile.am

I hope this makes it more clear what the final suggested changes are.

[iqaudio]

This would be great and allow MPD based apps (Volumio / RuneAudio / MoodeAudio to name a few) to standardise on the way they resolve this. It would also reduce customer emails :-) Thanks for driving and supporting this change.

[TermeHansen]

If you compile mpd from my fork you can test it already now, to see if it act as expected before merging it in to main master.

[MaxKellermann]

src/mixer/plugins/volume_mapping.cxx:34:0: error: "_GNU_SOURCE" redefined [-Werror]
 #define _GNU_SOURCE /* exp10() */

[TermeHansen]

Yep i got that as well, but it still compiled and worked, so didn't get to look further into it... Do you know why they have that line in volume_mapping?

[MaxKellermann]

Yes, because MPD defines _GNU_SOURCE globally - but ALSA doesn't. It compiles, but only without -Werror. I always build with -Werror, therefore warnings are not acceptable.

[TermeHansen]

so we just remove the line, end of story no?

small issue:
For some reason chosen volume and reported volume are not always completely the same (rounding issue). Adding the +0.5 to the get_volume at least makes it exactly the same as amixer reports. Still when setting a volume in the 60s gives a volume +-1 to the one set.
I will investigate a bit more...

[MaxKellermann]

Always round properly when you convert floating point to integer. Don't add 0.5 - that's a kludge. That's what lrint() is for.

[MaxKellermann]

Why define that one again?

[TermeHansen]

what do you mean again? Where is it defined? Doing this fixed my issue, as min on my card actually is -9999999

[MaxKellermann]

/usr/include/alsa/control.h:#define SND_CTL_TLV_DB_GAIN_MUTE -9999999

[TermeHansen]

hmm... it seems you are right (again...). Don't know why I decided that I saw a rounding issue earlier with this setting. I'll reset commits back and remove the _GNU_SOURCE line from the first commit.

[MaxKellermann]

Ordering of commits is wrong. With this ordering, we have a commit that we know fails to build - which breaks git bisect. Better amend the original commit.

[TermeHansen]

I don't get what you mean by amending the original commit. Do you suggest I "reset --hard" back to that commit and re commit without the line?

[MaxKellermann]

Maybe. I don't know about your tools, but I use stgit, and stgit allows me to amend older commits easily. This can be done with plain git, and may involve the "reset" command, but I don't know. It's more complicated with plain git.

[TermeHansen]

I have only used plain git (and also managed to get it to work here, but I will have a look on stgit, that I didn't know about. Thanks!

[MaxKellermann]

Something went wrong. Why is there a merge commit?

[MaxKellermann]

And while you're fixing your branch: it's a good idea to rebase your branch on the official master branch before (re)submitting. That is, git pull --rebase git://git.musicpd.org/master/mpd.git master

[TermeHansen]

got it... slowly learning the tricks of git. didn't know about the -amend trick (nice). Had to --force because it then was considered before remote/head.

what is the difference to your git here at github and the one at musicpd.org?

[MaxKellermann]

Both mirror the same contents. You can use either.

[MaxKellermann]

Merged with a bunch of fixups.