Re: [alsa-devel] snd_mixer_selem_get_playback_volume return value outside allowed range
2010/5/25 Clemens Ladisch clemens@ladisch.de
James Courtier-Dutton wrote:
On 25 May 2010 09:38, Clemens Ladisch clemens@ladisch.de wrote:
All the others seem to be mistakes:
soc/codecs/stac9766.c:... soc/codecs/wm8350.c:... soc/codecs/wm8400.c:... soc/codecs/wm8990.c:...
Could this maybe explain why pulseaudio was having such problems with alsa with regards to gain controls????
Only if it happens on embedded hardware that uses these particular codecs.
Regards, Clemens
http://git.alsa-project.org/?p=alsa-utils.git;a=commit;h=c9b86f49a8a1a8c337b...
your patch did not fix the buggy driver (e.g. alsa-pulse plugin) which return volume outside the alllowed range
amixer -D pulse Simple mixer control 'Master',0 Capabilities: pvolume pswitch pswitch-joined Playback channels: Front Left - Front Right Limits: Playback 0 - 65536 Mono: Front Left: Playback 98304 [150%] [on] Front Right: Playback 98304 [150%] [on]
Should snd_mixer_selem_get_playback_volume() perform range check ?
tatic int get_volume_ops(snd_mixer_elem_t *elem, int dir, snd_mixer_selem_channel_id_t channel, long *value) { selem_none_t *s = snd_mixer_elem_get_private(elem); if (s->selem.caps & SM_CAP_GVOLUME) dir = SM_PLAY; if ((unsigned int) channel >= s->str[dir].channels) return -EINVAL; *value = s->str[dir].vol[channel]; + if (*value > s->str[dir].max) + return -EINVAL; + if (*value < s->str[dir].min) + return -EINVAL; return 0;
At Thu, 17 Jun 2010 08:35:54 +0800, Raymond Yau wrote:
2010/5/25 Clemens Ladisch clemens@ladisch.de
James Courtier-Dutton wrote:
On 25 May 2010 09:38, Clemens Ladisch clemens@ladisch.de wrote:
All the others seem to be mistakes:
soc/codecs/stac9766.c:... soc/codecs/wm8350.c:... soc/codecs/wm8400.c:... soc/codecs/wm8990.c:...
Could this maybe explain why pulseaudio was having such problems with alsa with regards to gain controls????
Only if it happens on embedded hardware that uses these particular codecs.
Regards, Clemens
http://git.alsa-project.org/?p=alsa-utils.git;a=commit;h=c9b86f49a8a1a8c337b...
your patch did not fix the buggy driver (e.g. alsa-pulse plugin) which return volume outside the alllowed range
amixer -D pulse Simple mixer control 'Master',0 Capabilities: pvolume pswitch pswitch-joined Playback channels: Front Left - Front Right Limits: Playback 0 - 65536 Mono: Front Left: Playback 98304 [150%] [on] Front Right: Playback 98304 [150%] [on]
Should snd_mixer_selem_get_playback_volume() perform range check ?
tatic int get_volume_ops(snd_mixer_elem_t *elem, int dir, snd_mixer_selem_channel_id_t channel, long *value) { selem_none_t *s = snd_mixer_elem_get_private(elem); if (s->selem.caps & SM_CAP_GVOLUME) dir = SM_PLAY; if ((unsigned int) channel >= s->str[dir].channels) return -EINVAL; *value = s->str[dir].vol[channel];
if (*value > s->str[dir].max)return -EINVAL;if (*value < s->str[dir].min)return -EINVAL;
Some sanity check would be nice, but returning -EINVAL here isn't correct, at least. -EINVAL should be returned when the caller gives a wrong value (e.g. a wrong channel value).
Takashi
2010/6/17 Takashi Iwai tiwai@suse.de
At Thu, 17 Jun 2010 08:35:54 +0800, Raymond Yau wrote:
2010/5/25 Clemens Ladisch clemens@ladisch.de
James Courtier-Dutton wrote:
Could this maybe explain why pulseaudio was having such problems with alsa with regards to gain controls????
Only if it happens on embedded hardware that uses these particular codecs.
Regards, Clemens
http://git.alsa-project.org/?p=alsa-utils.git;a=commit;h=c9b86f49a8a1a8c337b...
your patch did not fix the buggy driver (e.g. alsa-pulse plugin) which return volume outside the alllowed range
amixer -D pulse Simple mixer control 'Master',0 Capabilities: pvolume pswitch pswitch-joined Playback channels: Front Left - Front Right Limits: Playback 0 - 65536 Mono: Front Left: Playback 98304 [150%] [on] Front Right: Playback 98304 [150%] [on]
Should snd_mixer_selem_get_playback_volume() perform range check ?
tatic int get_volume_ops(snd_mixer_elem_t *elem, int dir, snd_mixer_selem_channel_id_t channel, long *value) { selem_none_t *s = snd_mixer_elem_get_private(elem); if (s->selem.caps & SM_CAP_GVOLUME) dir = SM_PLAY; if ((unsigned int) channel >= s->str[dir].channels) return -EINVAL; *value = s->str[dir].vol[channel];
if (*value > s->str[dir].max)return -EINVAL;if (*value < s->str[dir].min)return -EINVAL;Some sanity check would be nice, but returning -EINVAL here isn't correct, at least. -EINVAL should be returned when the caller gives a wrong value (e.g. a wrong channel value).
Takashi
Clemens's patch just clamp the invalid volume to max is not not enough for alsamixer behave as normal
PA 's server still keep the 9xxxx value , it take a lot of time to press the arrow key for alsamixer to change from 100% to 99&
2010/6/17 Takashi Iwai tiwai@suse.de
At Thu, 17 Jun 2010 08:35:54 +0800, Raymond Yau wrote:
2010/5/25 Clemens Ladisch clemens@ladisch.de
James Courtier-Dutton wrote:
On 25 May 2010 09:38, Clemens Ladisch clemens@ladisch.de wrote:
All the others seem to be mistakes:
soc/codecs/stac9766.c:... soc/codecs/wm8350.c:... soc/codecs/wm8400.c:... soc/codecs/wm8990.c:...
Could this maybe explain why pulseaudio was having such problems with alsa with regards to gain controls????
Only if it happens on embedded hardware that uses these particular codecs.
Regards, Clemens
http://git.alsa-project.org/?p=alsa-utils.git;a=commit;h=c9b86f49a8a1a8c337b...
your patch did not fix the buggy driver (e.g. alsa-pulse plugin) which return volume outside the alllowed range
amixer -D pulse Simple mixer control 'Master',0 Capabilities: pvolume pswitch pswitch-joined Playback channels: Front Left - Front Right Limits: Playback 0 - 65536 Mono: Front Left: Playback 98304 [150%] [on] Front Right: Playback 98304 [150%] [on]
Should snd_mixer_selem_get_playback_volume() perform range check ?
tatic int get_volume_ops(snd_mixer_elem_t *elem, int dir, snd_mixer_selem_channel_id_t channel, long *value) { selem_none_t *s = snd_mixer_elem_get_private(elem); if (s->selem.caps & SM_CAP_GVOLUME) dir = SM_PLAY; if ((unsigned int) channel >= s->str[dir].channels) return -EINVAL; *value = s->str[dir].vol[channel];
if (*value > s->str[dir].max)return -EINVAL;if (*value < s->str[dir].min)return -EINVAL;Some sanity check would be nice, but returning -EINVAL here isn't correct, at least. -EINVAL should be returned when the caller gives a wrong value (e.g. a wrong channel value).
-ERANGE "result too large" seem to be the correct error code when value > max
amixer like the other mixer application assume snd_mixer_selem_get_playback_volume() won't fail so it is still cannot solve the problem
since the bug is PA server return a value which is outside the range declared by ctl_pulse.c
2010/6/17 Takashi Iwai tiwai@suse.de
At Thu, 17 Jun 2010 08:35:54 +0800, Raymond Yau wrote:
http://git.alsa-project.org/?p=alsa-utils.git;a=commit;h=c9b86f49a8a1a8c337b...
your patch did not fix the buggy driver (e.g. alsa-pulse plugin) which return volume outside the alllowed range
amixer -D pulse Simple mixer control 'Master',0 Capabilities: pvolume pswitch pswitch-joined Playback channels: Front Left - Front Right Limits: Playback 0 - 65536 Mono: Front Left: Playback 98304 [150%] [on] Front Right: Playback 98304 [150%] [on]
Should snd_mixer_selem_get_playback_volume() perform range check ?
tatic int get_volume_ops(snd_mixer_elem_t *elem, int dir, snd_mixer_selem_channel_id_t channel, long *value) { selem_none_t *s = snd_mixer_elem_get_private(elem); if (s->selem.caps & SM_CAP_GVOLUME) dir = SM_PLAY; if ((unsigned int) channel >= s->str[dir].channels) return -EINVAL; *value = s->str[dir].vol[channel];
if (*value > s->str[dir].max)return -EINVAL;if (*value < s->str[dir].min)return -EINVAL;Some sanity check would be nice, but returning -EINVAL here isn't correct, at least. -EINVAL should be returned when the caller gives a wrong value (e.g. a wrong channel value).
Takashi
But the application can use snd_mixer_selem_set_playback_volume_range() to set min and max of the control ,
is there any reason allow the application to set this range ?
it seem that snd_mixer_selem_set_playback_volume did not performe any range check
correct me if I am wrong, I seem able to use snd_mixer_selem_set_playback_volume to -1 for the pulse device and there is no error return , but I get another value when use snd_mixer_selem_get_playback_volume
BTW, amixer has -nocheck option to bypass the range check but this option seem unable to set the value of "Master playback volume" toabove max
Raymond Yau wrote:
Should snd_mixer_selem_get_playback_volume() perform range check ?
No, it's the responsibility of the control implementation to return valid values (and to check that values that are being set are valid).
Regards, Clemens
2010/6/17 Clemens Ladisch clemens@ladisch.de
Raymond Yau wrote:
Should snd_mixer_selem_get_playback_volume() perform range check ?
No, it's the responsibility of the control implementation to return valid values (and to check that values that are being set are valid).
Regards, Clemens
If it is the responsibility of the driver/external plugin to keep the value inside the allowed range
your patch actually help those buggy driver/external plugin hide the bug only
2010/6/17 Clemens Ladisch clemens@ladisch.de
Raymond Yau wrote:
Should snd_mixer_selem_get_playback_volume() perform range check ?
No, it's the responsibility of the control implementation to return valid values (and to check that values that are being set are valid).
Regards, Clemens _
The root cause of the bug is gnome-volume-control allow user to set PA_VOLUME to 150% and PA return the value outside the allowed range of the "master" volume control of the pulse device
http://git.gnome.org/browse/gnome-media/commit/?id=cfe5cf8d6826f2eacf0d2b109...
'Twas brillig, and Raymond Yau at 19/06/10 03:39 did gyre and gimble:
2010/6/17 Clemens Ladisch clemens@ladisch.de
Raymond Yau wrote:
Should snd_mixer_selem_get_playback_volume() perform range check ?
No, it's the responsibility of the control implementation to return valid values (and to check that values that are being set are valid).
Regards, Clemens _
The root cause of the bug is gnome-volume-control allow user to set PA_VOLUME to 150% and PA return the value outside the allowed range of the "master" volume control of the pulse device
http://git.gnome.org/browse/gnome-media/commit/?id=cfe5cf8d6826f2eacf0d2b109...
This is not the root cause of the bug Raymond. I have explained this several times now.
This is perfectly acceptable behaviour in PA.
I have stated the two possible solutions quite clearly (twice!) in one of the three threads you've managed to create on this topic.
Please stop going on about the issue and please reply just once to each email, it's incredibly annoying trying to follow any discussion you comment on due to this deluge of responses.
Col
2010/6/19 Colin Guthrie gmane@colin.guthr.ie
'Twas brillig, and Raymond Yau at 19/06/10 03:39 did gyre and gimble:
2010/6/17 Clemens Ladisch clemens@ladisch.de
Raymond Yau wrote:
Should snd_mixer_selem_get_playback_volume() perform range check ?
No, it's the responsibility of the control implementation to return valid values (and to check that values that are being set are valid).
Regards, Clemens _
The root cause of the bug is gnome-volume-control allow user to set PA_VOLUME to 150% and PA return the value outside the allowed range of
the
"master" volume control of the pulse device
http://git.gnome.org/browse/gnome-media/commit/?id=cfe5cf8d6826f2eacf0d2b109...
This is not the root cause of the bug Raymond. I have explained this several times now.
This is perfectly acceptable behaviour in PA.
I have stated the two possible solutions quite clearly (twice!) in one of the three threads you've managed to create on this topic.
Please stop going on about the issue and please reply just once to each email, it's incredibly annoying trying to follow any discussion you comment on due to this deluge of responses.
Col
I am discussing with Clemen 's patch which clamp the display volume within the allowed range is not correct. since the display percentage jump from 150% to 100% immediately but the actually value is still 9xxxx which is much larger than 65536, the user has to pressed the down arrow key for a long time (alsamixer seem freezing ) for the value to down below the allowed range (100% to 99%) when the buggy driver did not return value within allowed range of control
I don't think this behaviour of alsamixer is normal
http://git.alsa-project.org/?p=alsa-utils.git;a=commitdiff;h=c9b86f49a8a1a8c...
2010/6/19 Colin Guthrie gmane@colin.guthr.ie
'Twas brillig, and Raymond Yau at 19/06/10 03:39 did gyre and gimble:
2010/6/17 Clemens Ladisch clemens@ladisch.de
Raymond Yau wrote:
Should snd_mixer_selem_get_playback_volume() perform range check ?
No, it's the responsibility of the control implementation to return valid values (and to check that values that are being set are valid).
Regards, Clemens _
It is OK to clamp the volume so that the volume bar would not distort the layout
but the display percentage under the volume bar is clamp to 100% and this will mislead the user that alsamixer hangs since pressing up and down arrow cannot change 100% to 101% or 100% to 90% ( displayed percentage remain at 100% for a very long time)
value = ((volumes[0] - min) * 100 + (max - min) / 2) / (max - min); if (!(control->flags & HAS_VOLUME_1)) { sprintf(buf, "%d", value); display_string_in_field(values_y, frame_left - 2, buf, 8, ALIGN_CENTER); } else { mvwprintw(mixer_widget.window, values_y, frame_left - 2, "%3d", value); if (control->flags & IS_ACTIVE) wattrset(mixer_widget.window, attr_ctl_frame); waddstr(mixer_widget.window, "<>"); if (control->flags & IS_ACTIVE) wattrset(mixer_widget.window, attr_mixer_active); value = ((volumes[1] - min) * 100 + (max - min) / 2) / (max - min); wprintw(mixer_widget.window, "%-3d", value); }
participants (4)
-
Clemens Ladisch -
Colin Guthrie -
Raymond Yau -
Takashi Iwai