Re: PulseAudio and softvol

[David Henningsson <david.henningsson@canonical.com>]

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)
  {
         snd_mixer_t *mixer;
         assert(mixerp);
         mixer = calloc(1, sizeof(*mixer));
         if (mixer == NULL)
                 return -ENOMEM;
+       mixer->attach_mode = mode;
         INIT_LIST_HEAD(&mixer->slaves);
         INIT_LIST_HEAD(&mixer->classes);
         INIT_LIST_HEAD(&mixer->elems);
@@ -200,7 +201,7 @@ int snd_mixer_attach(snd_mixer_t *mixer, const char 
*name)
         snd_hctl_t *hctl;
         int err;

-       err = snd_hctl_open(&hctl, name, 0);
+       err = snd_hctl_open(&hctl, name, mixer->attach_mode);
         if (err < 0)
                 return err;
         err = snd_mixer_attach_hctl(mixer, hctl);
diff --git a/src/mixer/mixer_local.h b/src/mixer/mixer_local.h
index 27b4a3b..2d1866e 100644
--- a/src/mixer/mixer_local.h
+++ b/src/mixer/mixer_local.h
@@ -71,6 +71,7 @@ struct _snd_mixer {
         unsigned int count;
         unsigned int alloc;
         unsigned int events;
+       int attach_mode;
         snd_mixer_callback_t callback;
         void *callback_private;
         snd_mixer_compare_t compare;


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic