[pulseaudio-discuss] [PATCH 6/6] ucm: Add support for "JackHWMute"

[Tanu Kaskinen]

On Wed, 2015-03-18 at 12:30 +0530, Arun Raghavan wrote:
> On Thu, 2015-02-12 at 15:11 +0200, Tanu Kaskinen wrote:
> > "JackHWMute" is a UCM device value that, if set, indicates that the
> > jack of the device mutes some other device at the hardware or driver
> > level. A common example would be a headphone jack that mutes built-in
> > speakers. PulseAudio should show such auto-muted devices as
> > unavailable.
> 
> Wouldn't the "right" way for this to happen be to have the corresponding
> mute control of the device be set to mute by the driver? Are there
> circumstances where this might not be possible, or are we basically
> providing a shortcut to describe hardware behaviour via configuration
> rather than via the driver (i.e. exposing and updating kcontrols)?

Just muting the speaker control isn't enough to convey that trying to
unmute the control will be futile. At least on my laptop the speaker
mute control does nothing when headphones are plugged in. I don't know
if that's a driver bug or a genuine hardware limitation.

> > diff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h
> > index ec39fab..dad1ea2 100644
> > --- a/src/modules/alsa/alsa-mixer.h
> > +++ b/src/modules/alsa/alsa-mixer.h
> > @@ -168,8 +168,22 @@ struct pa_alsa_jack {
> >      pa_alsa_required_t required;
> >      pa_alsa_required_t required_any;
> >      pa_alsa_required_t required_absent;
> > +
> > +    /* UCM specific: this is the "owner" device of this jack. When the jack is
> > +     * plugged in, the device referenced here gets marked as available, and
> > +     * when the jack is unplugged, the device gets marked as unavailable. */
> > +    pa_alsa_ucm_device *device;
> > +
> > +    /* UCM specific: if this is non-NULL, then the referenced device gets muted
> > +     * by the hardware (or driver) when this jack is plugged in. The muting can't
> > +     * be overridden by PulseAudio, so the device gets marked as
> > +     * unavailable (so if a device is referenced by this field, the effect is
> > +     * inverse to being referenced by the "device" field). */
> > +    pa_alsa_ucm_device *hw_mute;
> >  };
> 
> Might make sense to prefix those two with "ucm_" to make the meaning
> clearer wherever they're used.

Yes, that seems sensible.

> > diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
> > index ef6adcd..061b8f7 100644
> > --- a/src/modules/alsa/alsa-ucm.c
> > +++ b/src/modules/alsa/alsa-ucm.c
> > @@ -66,6 +66,17 @@
> >  
> >  #ifdef HAVE_ALSA_UCM
> >  
> > +static void device_set_mute_jack(pa_alsa_ucm_device *device, pa_alsa_jack *jack);
> > +
> > +struct port {
> 
> I'd prefer ucm_port or pa_alsa_ucm_port -- the namespacing means that
> it's easier to look up stuff with ctags/cscope/whatever shiny thing
> comes in the future (maybe some day we'll have a way of distinguishing
> the several struct userdatas that we have too).

I'd be strongly against pa_alsa_ucm_port. It's a private struct, and the
convention is to not prefix those with pa_, and I think that's a good
convention. I suppose ucm_port would be ok, but I feel that it's
redundant, since the file name already implies that everything is ucm
related (I also have a todo item for removing the ucm_ prefix from some
of the function names in that file). I'm not familiar with
ctags/cscope/whatever, so I'm pretty clueless as to how useful the extra
prefixing would be when using those things.

> > @@ -394,9 +406,12 @@ static int ucm_get_devices(pa_alsa_ucm_verb *verb, snd_use_case_mgr_t *uc_mgr) {
> >      for (i = 0; i < num_dev; i += 2) {
> >          pa_alsa_ucm_device *d = pa_xnew0(pa_alsa_ucm_device, 1);
> >  
> > +        d->verb = verb;
> >          d->proplist = pa_proplist_new();
> >          pa_proplist_sets(d->proplist, PA_ALSA_PROP_UCM_NAME, pa_strnull(dev_list[i]));
> >          pa_proplist_sets(d->proplist, PA_ALSA_PROP_UCM_DESCRIPTION, pa_strna(dev_list[i + 1]));
> > +        d->ports = pa_dynarray_new(NULL);
> > +        d->available = true;
> 
> Shouldn't the default availability be unknown, with actual availability
> set if we have a corresponding jack?

No, I think all ucm devices should be marked as available, unless
otherwise indicated by the jacks. The difference to the "normal" alsa
mixer stuff is that with UCM the configuration is tailored for the
hardware, while the "normal" alsa mixer stuff always tries to use all
paths.

...or that's at least what I told myself when I wrote this code.
Thinking about this again, that logic doesn't seem to hold water. In the
normal alsa mixer stuff ports are marked with unknown availability when
there's no jack control associated with a path, and I suppose there's no
guarantee with UCM either that there would be a jack control associated
always with e.g. headphones. I suppose you're right, I should set the
device availability to unknown if there's no information from jacks.

> > @@ -1591,6 +1820,15 @@ void pa_alsa_ucm_free(pa_alsa_ucm_config *ucm) {
> >      pa_alsa_ucm_verb *vi, *vn;
> >      pa_alsa_jack *ji, *jn;
> >  
> > +    if (ucm->ports) {
> > +        struct port *port;
> > +
> > +        while ((port = pa_dynarray_last(ucm->ports)))
> > +            port_free(port);
> 
> Should just pass port_free() as the free function in pa_dynarray_new().

Well, that depends... With the current code port_free() is responsible
for removing itself from the dynarray, which makes sense, since
port_new() is responsible for adding the port to the dynarray. I'd
really like to keep that symmetry.

> > @@ -142,10 +155,33 @@ struct pa_alsa_ucm_device {
> >      pa_idxset *conflicting_devices;
> >      pa_idxset *supported_devices;
> >  
> > +    pa_dynarray *ports; /* pa_alsa_ucm_port */
> > +
> > +    /* These jacks are owned by this device. When these jacks are plugged in,
> > +     * the device is marked as available, and when these jacks are unplugged,
> > +     * the device is marked as unavailable. Not all devices have jacks
> > +     * associated with them; those devices are always marked as available.
> > +     *
> > +     * XXX: It's probably pointless to have separate jacks for input and
> > +     * output. Both jack objects will anyway reference the same alsa
> > +     * kcontrol, so they will always have the same state. */
> >      pa_alsa_jack *input_jack;
> >      pa_alsa_jack *output_jack;
> 
> I don't know if this actually ever does happen this way, but I can
> imagine that you might have separate headphone and mic jack kcontrols
> for a combined physical headset port. If that might be the case, the XXX
> comment can be removed.

There's no support for such setup. The UCM "spec" doesn't allow separate
controls for input and output. Perhaps it should, though. I'll raise
this issue on alsa-devel.

> > @@ -80,10 +78,14 @@ void pa_device_port_set_available(pa_device_port *p, pa_available_t status) {
> >         status == PA_AVAILABLE_NO ? "no" : "unknown");
> >  
> >      /* Post subscriptions to the card which owns us */
> > -    pa_assert_se(core = p->core);
> > -    pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, p->card->index);
> > -
> > -    pa_hook_fire(&core->hooks[PA_CORE_HOOK_PORT_AVAILABLE_CHANGED], p);
> > +    /* XXX: We need to check p->card, because this function may be called
> > +     * before the card object has been created. The card object should probably
> > +     * be created before port objects, and then p->card could be non-NULL for
> > +     * the whole lifecycle of pa_device_port. */
> 
> Mario was working on implementing David's comments from
> https://bugs.freedesktop.org/show_bug.cgi?id=87002#c18 -- which might
> make this comment obsolete (but the condition correct). 

I don't have a clear picture of what David proposes in that comment, but
it sounds like ports would still be created before cards. If so, I don't
know how that would make the comment obsolete, unless you're only
referring to the "card object should probably be created before port
objects" part. In that case it's a question of whose "should" we take as
the truth... In my thinking the "bigger" (the container) object should
always be created before the "smaller" (the contained) object, because
that way the small->big pointer can be constant and never-NULL, which
helps with robustness and readability.

-- 
Tanu