Re: bin/56552: mixerctl(1) does not increment volume with ++ or +=1

[RVP <rvp%SDF.ORG@localhost>]

The following reply was made to PR bin/56552; it has been noted by GNATS.

From: RVP <rvp%SDF.ORG@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: Michael van Elst <mlelstv%serpens.de@localhost>
Subject: Re: bin/56552: mixerctl(1) does not increment volume with ++ or
 +=1
Date: Thu, 16 Dec 2021 07:38:17 +0000 (UTC)

 On Wed, 15 Dec 2021, Michael van Elst wrote:
 
 > HD-Audio computes the delta value as:
 >
 >     mx[index].mx_di.un.v.delta = 256 / (masterctl->ctl_step + 1);
 >
 > [...]
 >
 > When changing a value it computes the values like:
 >
 >     if (ctl->ctl_step == 0)
 >         divisor = 128; /* ??? - just avoid div by 0 */
 >     else
 >         divisor = 255 / ctl->ctl_step;
 >
 >     hdafg_control_amp_set(ctl, HDAUDIO_AMP_MUTE_NONE,
 >       mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] / divisor,
 >       mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] / divisor);
 >
 > [...]
 >
 > Reading back the value is done like:
 >
 >     if (ctl->ctl_step == 0)
 >         factor = 128; /* ??? - just avoid div by 0 */
 >     else
 >         factor = 255 / ctl->ctl_step;
 >
 >     mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = ctl->ctl_left * factor;
 >     mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] = ctl->ctl_right * factor;
 >
 
 After that concise explanation it was pretty clear what had to be done:
 
 ---START---
 --- sys/dev/hdaudio/hdafg.c.orig	2020-06-11 02:39:30.000000000 +0000
 +++ sys/dev/hdaudio/hdafg.c	2021-12-16 07:04:54.341631494 +0000
 @@ -2887,7 +2887,7 @@
   		mx[index].mx_di.prev = mx[index].mx_di.next = AUDIO_MIXER_LAST;
   		mx[index].mx_di.un.v.num_channels = 2;	/* XXX */
   		mx[index].mx_di.mixer_class = HDAUDIO_MIXER_CLASS_OUTPUTS;
 -		mx[index].mx_di.un.v.delta = 256 / (masterctl->ctl_step + 1);
 +		mx[index].mx_di.un.v.delta = 256 / (masterctl->ctl_step ? masterctl->ctl_step : 1);
   		strcpy(mx[index].mx_di.label.name, AudioNmaster);
   		strcpy(mx[index].mx_di.un.v.units.name, AudioNvolume);
   		hda_trace(sc, "  adding outputs.%s\n",
 @@ -2922,7 +2922,7 @@
   		mx[index].mx_di.type = AUDIO_MIXER_VALUE;
   		mx[index].mx_di.prev = mx[index].mx_di.next = AUDIO_MIXER_LAST;
   		mx[index].mx_di.un.v.num_channels = 2;	/* XXX */
 -		mx[index].mx_di.un.v.delta = 256 / (ctl->ctl_step + 1);
 +		mx[index].mx_di.un.v.delta = 256 / (ctl->ctl_step ? ctl->ctl_step : 1);
   		if (ctrlcnt[audiodev] > 0)
   			snprintf(mx[index].mx_di.label.name,
   			    sizeof(mx[index].mx_di.label.name),
 ---END---
 
 Now the mixerctl.c patch becomes:
 
 ---START---
 diff -urN a/mixerctl.c b/mixerctl.c
 --- a/mixerctl.c	2017-02-23 14:09:11.000000000 +0000
 +++ b/mixerctl.c	2021-12-16 07:20:30.533031095 +0000
 @@ -139,6 +139,16 @@
   }
 
   static int
 +clamp(int vol)
 +{
 +	if (vol < AUDIO_MIN_GAIN)
 +		vol = AUDIO_MIN_GAIN;
 +	if (vol > AUDIO_MAX_GAIN)
 +		vol = AUDIO_MAX_GAIN;
 +	return vol;
 +}
 +
 +static int
   rdfield(struct field *p, char *q)
   {
   	mixer_ctrl_t *m;
 @@ -181,17 +191,17 @@
   	case AUDIO_MIXER_VALUE:
   		if (m->un.value.num_channels == 1) {
   			if (sscanf(q, "%d", &v) == 1) {
 -				m->un.value.level[0] = v;
 +				m->un.value.level[0] = clamp(v);
   			} else {
   				warnx("Bad number %s", q);
   				return 0;
   			}
   		} else {
   			if (sscanf(q, "%d,%d", &v0, &v1) == 2) {
 -				m->un.value.level[0] = v0;
 -				m->un.value.level[1] = v1;
 +				m->un.value.level[0] = clamp(v0);
 +				m->un.value.level[1] = clamp(v1);
   			} else if (sscanf(q, "%d", &v) == 1) {
 -				m->un.value.level[0] = m->un.value.level[1] = v;
 +				m->un.value.level[0] = m->un.value.level[1] = clamp(v);
   			} else {
   				warnx("Bad numbers %s", q);
   				return 0;
 @@ -234,11 +244,7 @@
   		for (i = 0; i < m->un.value.num_channels; i++) {
   			v = m->un.value.level[i];
   			v += inc;
 -			if (v < AUDIO_MIN_GAIN)
 -				v = AUDIO_MIN_GAIN;
 -			if (v > AUDIO_MAX_GAIN)
 -				v = AUDIO_MAX_GAIN;
 -			m->un.value.level[i] = v;
 +			m->un.value.level[i] = clamp(v);
   		}
   		break;
   	default:
 ---END---
 
 I'm clamping the volume everywhere so there's no unwanted wraparound now:
 
 $ ./mixerctl.new -w outputs.master++
 outputs.master: 168,168 -> 174,174
 $ ./mixerctl.new -w outputs.master+=1
 outputs.master: 174,174 -> 180,180
 $ ./mixerctl.new -w outputs.master=11111111
 outputs.master: 180,180 -> 252,252
 $ ./mixerctl.new -w outputs.master=-11111111
 outputs.master: 252,252 -> 0,0
 $
 
 Thanks!
 -RVP