attach/detach by mixer class implementation of alsa-lib mixer API

attach/detach by mixer class implementation of alsa-lib mixer API

[Takashi Sakamoto]

Hi Jaroslav,

Recent years I'm bothered about unexpected abort of pulseaudio and
pipewire processes when testing user-defined control element set.
They aborts at element removal event.

```
pulseaudio: mixer.c:149: hctl_elem_event_handler: Assertion `bag_empty(bag)' failed.
wireplumber: mixer.c:149: hctl_elem_event_handler: Assertion `bag_empty(bag)' failed.
```

Would I ask your opinion about the design of alsa-lib mixer API?

As long as I investigate, these programs seem to have careless coding as
alsa-lib mixer API application. Both of them attaches an instance of
snd_mixer_elem_t to an instance of snd_hctl_elem_t by calling
snd_mixer_elem_attach() when detecting element addition, but never detach
it even if detecting element removal.

In the case, the link list (=bag) of mixer API internal never becomes empty.
It has link entries as much as the number of registered mixer classes which
attaches snd_mixer_elem_t. Then it hits the assertion.

I think the design of alsa-lib mixer API demands mixer class implementation
to detach at element removal which attached at element addition. But I have
less conviction about it since enough unfamiliar.

I'm glad if receiving your opinion about it.

I wrote test program for the issued behaviour:
 - https://gist.github.com/takaswie/8378fe3bc04652d83428694cd7573bc0

For test, please use sample script in alsa-gobject project:
 - https://github.com/alsa-project/alsa-gobject/blob/master/samples/ctl

The patches for pulseaudio/pipewire are prepared:
 - https://gitlab.freedesktop.org/takaswie/pulseaudio/-/commit/topic/fix-wrong-handling-alsa-ctl-event
 - https://gitlab.freedesktop.org/takaswie/pipewire/-/commits/topic/fix-wrong-handling-alsa-ctl-event


Regards

Takashi Sakamoto

Re: attach/detach by mixer class implementation of alsa-lib mixer API

[Jaroslav Kysela]

On 25. 06. 22 13:28, Takashi Sakamoto wrote:
> Hi Jaroslav,
> 
> Recent years I'm bothered about unexpected abort of pulseaudio and
> pipewire processes when testing user-defined control element set.
> They aborts at element removal event.
> 
> ```
> pulseaudio: mixer.c:149: hctl_elem_event_handler: Assertion `bag_empty(bag)' failed.
> wireplumber: mixer.c:149: hctl_elem_event_handler: Assertion `bag_empty(bag)' failed.
> ```
> 
> Would I ask your opinion about the design of alsa-lib mixer API?
> 
> As long as I investigate, these programs seem to have careless coding as
> alsa-lib mixer API application. Both of them attaches an instance of
> snd_mixer_elem_t to an instance of snd_hctl_elem_t by calling
> snd_mixer_elem_attach() when detecting element addition, but never detach
> it even if detecting element removal.
> 
> In the case, the link list (=bag) of mixer API internal never becomes empty.
> It has link entries as much as the number of registered mixer classes which
> attaches snd_mixer_elem_t. Then it hits the assertion.
> 
> I think the design of alsa-lib mixer API demands mixer class implementation
> to detach at element removal which attached at element addition. But I have
> less conviction about it since enough unfamiliar.

Yes, if the REMOVE event is delivered to the mixer class, the reference to the 
associated hctl element should be removed. The assert does the check for this 
consistency.

> I'm glad if receiving your opinion about it.
> 
> I wrote test program for the issued behaviour:
>   - https://gist.github.com/takaswie/8378fe3bc04652d83428694cd7573bc0
> 
> For test, please use sample script in alsa-gobject project:
>   - https://github.com/alsa-project/alsa-gobject/blob/master/samples/ctl
> 
> The patches for pulseaudio/pipewire are prepared:
>   - https://gitlab.freedesktop.org/takaswie/pulseaudio/-/commit/topic/fix-wrong-handling-alsa-ctl-event
>   - https://gitlab.freedesktop.org/takaswie/pipewire/-/commits/topic/fix-wrong-handling-alsa-ctl-event

Your fixes seem correct. Please, create the PA/PW merge request with this 
code. Please add me (@perexg) to your merge message.

Thank you for your work on this.

						Jaroslav


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: attach/detach by mixer class implementation of alsa-lib mixer API

[Takashi Sakamoto]

Hi,

On Sun, Jun 26, 2022 at 07:23:23PM +0200, Jaroslav Kysela wrote:
> On 25. 06. 22 13:28, Takashi Sakamoto wrote:
> > Hi Jaroslav,
> > 
> > Recent years I'm bothered about unexpected abort of pulseaudio and
> > pipewire processes when testing user-defined control element set.
> > They aborts at element removal event.
> > 
> > ```
> > pulseaudio: mixer.c:149: hctl_elem_event_handler: Assertion `bag_empty(bag)' failed.
> > wireplumber: mixer.c:149: hctl_elem_event_handler: Assertion `bag_empty(bag)' failed.
> > ```
> > 
> > Would I ask your opinion about the design of alsa-lib mixer API?
> > 
> > As long as I investigate, these programs seem to have careless coding as
> > alsa-lib mixer API application. Both of them attaches an instance of
> > snd_mixer_elem_t to an instance of snd_hctl_elem_t by calling
> > snd_mixer_elem_attach() when detecting element addition, but never detach
> > it even if detecting element removal.
> > 
> > In the case, the link list (=bag) of mixer API internal never becomes empty.
> > It has link entries as much as the number of registered mixer classes which
> > attaches snd_mixer_elem_t. Then it hits the assertion.
> > 
> > I think the design of alsa-lib mixer API demands mixer class implementation
> > to detach at element removal which attached at element addition. But I have
> > less conviction about it since enough unfamiliar.
> 
> Yes, if the REMOVE event is delivered to the mixer class, the reference to
> the associated hctl element should be removed. The assert does the check for
> this consistency.
> 
> > I'm glad if receiving your opinion about it.
> > 
> > I wrote test program for the issued behaviour:
> >   - https://gist.github.com/takaswie/8378fe3bc04652d83428694cd7573bc0
> > 
> > For test, please use sample script in alsa-gobject project:
> >   - https://github.com/alsa-project/alsa-gobject/blob/master/samples/ctl
> > 
> > The patches for pulseaudio/pipewire are prepared:
> >   - https://gitlab.freedesktop.org/takaswie/pulseaudio/-/commit/topic/fix-wrong-handling-alsa-ctl-event
> >   - https://gitlab.freedesktop.org/takaswie/pipewire/-/commits/topic/fix-wrong-handling-alsa-ctl-event
> 
> Your fixes seem correct. Please, create the PA/PW merge request with this
> code. Please add me (@perexg) to your merge message.
> 
> Thank you for your work on this.
 
Thanks for your confirmation and reviewing.

In my opinion, it's better to have enough explanation about the
postcondition in alsa-lib documentation so that developer for
implementation of mixer class takes care of the postcondition. I filed a
request to alsa-lib repository.

 * https://github.com/alsa-project/alsa-lib/pull/244

Then I filed to each project with reference to your account:

 * https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/728
 * https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1296

> 						Jaroslav
> 
> 
> -- 
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.


Thanks

Takashi Sakamoto