[alsa-devel] PulseAudio and softvol

[Takashi Iwai]

At Wed, 15 May 2013 15:12:17 +0200,
Jaroslav Kysela wrote:
> 
> Date 15.5.2013 15:05, Takashi Iwai wrote:
> > At Wed, 15 May 2013 14:52:53 +0200,
> > Jaroslav Kysela wrote:
> >>
> >> Date 15.5.2013 14:47, David Henningsson wrote:
> >>> On 05/15/2013 02:42 PM, Takashi Iwai wrote:
> >>>> At Wed, 15 May 2013 13:22:03 +0200,
> >>>> Jaroslav Kysela wrote:
> >>>>>
> >>>>> Date 15.5.2013 13:03, David Henningsson wrote:
> >>>>>> On 05/15/2013 12:53 PM, Jaroslav Kysela wrote:
> >>>>>>> Date 15.5.2013 12:48, Takashi Iwai wrote:
> >>>>>>>> At Wed, 15 May 2013 12:26:51 +0200,
> >>>>>>>> Jaroslav Kysela wrote:
> >>>>>>>>>
> >>>>>>>>> Date 15.5.2013 11:55, Arun Raghavan wrote:
> >>>>>>>>>> Hello,
> >>>>>>>>>> A number of users have intermittently(?) been hitting a crash in
> >>>>>>>>>> alsa-lib 1.0.27 [1, 2] related to the softvol plugin. I'm not able to
> >>>>>>>>>> reproduce this reliably, so can't find an easy way to debug/fix.
> >>>>>>>>>
> >>>>>>>>> The problem is that the offsets are not in sync in this case [1]:
> >>>>>>>>>
> >>>>>>>>> src_offset = 38560
> >>>>>>>>> dst_offset = 38568
> >>>>>>>>> frames = 16374
> >>>>>>>>>
> >>>>>>>>> Could you reproduce this bug in any way? At least snd_pcm_dump() before
> >>>>>>>>> the failing snd_pcm_mmap_commit() call might help to determine what was
> >>>>>>>>> the status before the assert() was entered.
> >>>>>>>>
> >>>>>>>> Yep.  And this path is actually with volume 0dB, that is, a simply
> >>>>>>>> passthrough in softvol.  Thus the bug may hit essentially any
> >>>>>>>> plugins, not specifically softvol.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> However, this raises a tangential question - why do we need softvol to
> >>>>>>>>>> be plugged for 'front' at all? David explained to me that this is to
> >>>>>>>>>> guarantee the existence of a PCM control. Perhaps I don't fully
> >>>>>>>>>> understand this, because I'm unconvinced by the reason. Could someone
> >>>>>>>>>> explain/refute?
> >>>>>>>>>>
> >>>>>>>>>> This is especially bad for us, from PulseAudio's perspective, because we
> >>>>>>>>>> aren't getting a zero-copy path.
> >>>>>>>>>
> >>>>>>>>> If the softvol is set to 0dB (no attenuation or gain), then the ring
> >>>>>>>>> buffer pointers are moved without any sample processing, so the
> >>>>>>>>> zero-copy functionality is kept.
> >>>>>>>>
> >>>>>>>> Yeah, a sort of.  The mmap is cleared in the slave PCM, so actually
> >>>>>>>> there will be copy operations in underlying layers even though softvol
> >>>>>>>> itself does zero copy.
> >>>>>>>>
> >>>>>>>> Actually it makes no sense to keep softvol for PA, but the problem is
> >>>>>>>> always the regression.  There are certainly users without PA, which
> >>>>>>>> might still rely on the softvol for such hardware without the amp
> >>>>>>>> control.
> >>>>>>>>
> >>>>>>>> Maybe We can add some flag to indicate whether to handle softvol or
> >>>>>>>> not, e.g. defaults.pcm.skip_softvol, and let PA set this in its config
> >>>>>>>> space.  Setting a config item itself would break anything, so it'll
> >>>>>>>> still work with old alsa-lib (but with softvol).
> >>>>>>>
> >>>>>>> We have already SND_PCM_NO_SOFTVOL open mode for this purpose, so I
> >>>>>>> wonder, why PA does not use it..
> >>>>>>
> >>>>>> The problem is knowing whether PCM is a softvol or not. In some cases,
> >>>>>> we need to set PCM to control hardware volume.
> >>>>>>
> >>>>>> Maybe, if we could figure this out somehow, we could ignore the PCM
> >>>>>> mixer control (or possibly set it to zero) in case PCM is a softvol,
> >>>>>> and actually use it if PCM is not a softvol.
> >>>>>>
> >>>>>> It does not look like this is currently possible from the simple mixer
> >>>>>> interface, but I might be missing something?
> >>>>>
> >>>>> It is not possible. Perhaps, we may create a new dummy mixer control (in
> >>>>> an inactive state) which will identify the presence of the softvol
> >>>>> plugin, like:
> >>>>>
> >>>>> "Softvol PCM Playback Volume" - full name for the raw control API
> >>>>> "Softvol PCM" - simple mixer name
> >>>>
> >>>> Well, if changing in such a way, I'd rather drop softvol from
> >>>> HDA-Intel.conf.
> >>>>
> >>>> If we could give some flag in mixer API, we could add a code to filter
> >>>> out the user controls from the mixer's hctl.  But snd_mixer_attach()
> >>>> takes only the string, and the string modifier may lead to the
> >>>> incompatibility when used with an older version.  Hmm.
> >>>
> >>> That seems solvable to me, something like this:
> >>>
> >>> diff --git a/src/mixer/mixer.c b/src/mixer/mixer.c
> >>> index 56e023d..4afa979 100644
> >>> --- a/src/mixer/mixer.c
> >>> +++ b/src/mixer/mixer.c
> >>> @@ -65,13 +65,14 @@ static int snd_mixer_compare_default(const 
> >>> snd_mixer_elem_t *c1,
> >>>    * \param mode Open mode
> >>>    * \return 0 on success otherwise a negative error code
> >>>    */
> >>> -int snd_mixer_open(snd_mixer_t **mixerp, int mode ATTRIBUTE_UNUSED)
> >>> +int snd_mixer_open(snd_mixer_t **mixerp, int mode)
> >>
> >> Yes, it could be implemented in this way. A special TLV entry may be
> >> introduced to detect, if the control is created by softvol.
> > 
> > The additional TLV won't work if a control is restored by alsactl, for
> > example, unfortunately.
> 
> This looks like a bug, doesn't?
> Anyway, I see some TLV restore code in
> alsactl, but the support for all control types should be added not only
> for SND_CTL_ELEM_TYPE_INTEGER.

Well, alsactl would just restore what's saved.  So, if the saved data
already contains the softvol ctl element with the old TLV, it's simply
restored as is.

You may think of adding the code to softvol plugin to automatically
rewrite TLV of the existing ctl element if it contains no new TLV
type.  But, PA shall skip softvol.  Thus, it won't be touched.  And
yet, PA would like to skip the control elements that have been created
beforehand.

Certainly this is a corner case, but the requirement is incompatible
with old data.  If it's only about the change of the library code, it
would work by a simple update.  But if an additional metadata has to
be embedded, it's a different question...


Takashi