[pulseaudio-discuss] [RFC PATCH 0/4] PA2.0 UCM integration

[Feng Wei]

David,
Thank your careful review. I will fix those immediately.
>
> Hi Feng,
>
> I've had a look at your patches. I went with the patches at git.linaro.org
> and diffed them against master, this was to avoid commenting on the things
> you already fixed.
>
> See comments below for details. In general I've mostly looked at the
> integration, rather than the alsa-ucm.c file. I think the integration is
> well done, and I think we should merge this sooner rather than later. The
> nitpicks below should not block merging (possibly with the exception of the
> public API question I have about proplist.h).
>
> My biggest question is therefore about the future. I just found out that the
> Linaro Multimedia working group is dismantling, and as none of our core team
> really know this stuff by heart, who will be around to maintain this code?
Multimedia working group is divided into several other working group,
and I'm transfered to Graphic and Audio Working Group. So the
maintainer is still ... me. :-)
In the worst case, I could still contribute as a community member.
> If we want to refactor things later on, who can test that we don't break UCM
> in some way?
>
>>
>> +static int setup_mixer_ucm(struct userdata *u, pa_bool_t ignore_dB) {
>> +    pa_assert(u);
>> +    pa_assert(u->sink);
>> +    pa_assert(u->ucm_context);
>> +
>> +    if (u->sink->active_port)
>> +        return pa_alsa_ucm_set_port(u->ucm_context, u->sink->active_port,
>> 1);
>> +
>> +    return 0;
>> +}
>
>
> Nitpick: I don't think we need setup_mixer_ucm. Just call
> sink_set_port_ucm_cb or pa_alsa_ucm_set_port instead.
Fine.
>
>>
>> -    if (setup_mixer(u, ignore_dB) < 0)
>> +    if (u->ucm_context) {
>> +        if (setup_mixer_ucm(u, ignore_dB) < 0)
>> +            goto fail;
>>
>
> Same as for sink (setup_mixer_ucm not needed)
>
>>
>> +    }
>> +    else if (setup_mixer(u, ignore_dB) < 0)
>>         goto fail;
>>
>> +    PA_HASHMAP_FOREACH(p, ps->profiles, state) {
>> +        /* change verb */
>> +        pa_log_info("ucm_probe_jacks: set ucm verb to %s", p->name);
>> +        if ((snd_use_case_set(ucm->ucm_mgr, "_verb", p->name)) < 0) {
>> +            pa_log("ucm_probe_jacks: failed to set verb %s", p->name);
>> +            p->supported = FALSE;
>
>
> If this is a function to probe jacks...why can it mark a profile as
> unsupported? Does it probe more than jacks?
Actually it also probes profile_set, setting verb is to activate
profile. I'll fix the name of the function to
ucm_probe_profile_set_and_jacks
>> diff --git a/src/modules/alsa/alsa-ucm.h b/src/modules/alsa/alsa-ucm.h
>> new file mode 100644
>> index 0000000..5ae9954
>> --- /dev/null
>> +++ b/src/modules/alsa/alsa-ucm.h
>> @@ -0,0 +1,132 @@
>> +#ifndef fooalsaucmhfoo
>> +#define fooalsaucmhfoo
>> +
>> +/***
>> +  This file is part of PulseAudio.
>> +
>> +  Copyright 2011 Wolfson Microelectronics PLC
>> +  Author Margarita Olaya <magi at slimlogic.co.uk>
>> +  Copyright 2012 Feng Wei <feng.wei at linaro.org>, Linaro
>> +
>> +  PulseAudio is free software; you can redistribute it and/or modify
>> +  it under the terms of the GNU Lesser General Public License as
>> published
>> +  by the Free Software Foundation; either version 2.1 of the License,
>> +  or (at your option) any later version.
>> +
>> +  PulseAudio is distributed in the hope that it will be useful, but
>> +  WITHOUT ANY WARRANTY; without even the implied warranty of
>> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> +  General Public License for more details.
>> +
>> +  You should have received a copy of the GNU Lesser General Public
>> License
>> +  along with PulseAudio; if not, write to the Free Software
>> +  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
>> +  USA.
>> +***/
>> +
>> +#include <asoundlib.h>
>> +#include <use-case.h>
>>
>
> Hmm, this is probably not your fault, but for being a public header file
> use-case.h is probably not the best name - either <asound/use-case.h> or
> <asound-use-case.h> would have been better, but I guess there's nothing to
> do about that. (But do you really need both for just the snd_use_case_mgr_t
> symbol?)
asoundlib.h is not needed, will be removed. About use-case.h, now I
can't fix that.
>
>>
>> +
>> +/* UCM modifier action direction */
>> +enum {
>> +    PA_ALSA_UCM_DIRECTION_NONE = 0,
>> +    PA_ALSA_UCM_DIRECTION_SINK,
>> +    PA_ALSA_UCM_DIRECTION_SOURCE
>> +};
>> +
>>
>
> We already have "enum pa_device_type", "enum pa_direction" and "enum
> pa_stream_direction" (see pulse/def.h), any chance we can use one of those
> instead of adding yet another enum meaning the same thing?
My favorite is pa_direction, but I was afraid pa_direction could be
dual direction before.
>
>>

>> +struct pa_alsa_port_data_ucm {
>> +    int dummy;
>> +};
>>
>
> You don't need this struct pa_alsa_port_data_ucm. Just specify 0 in the call
> to pa_device_port_new.
ok.
>
>> diff --git a/src/modules/alsa/module-alsa-card.c
>> b/src/modules/alsa/module-alsa-card.c
>> index b06394d..67b0a98 100644
>> --- a/src/modules/alsa/module-alsa-card.c
>> +++ b/src/modules/alsa/module-alsa-card.c
>> @@ -153,7 +180,14 @@ static void add_profiles(struct userdata *u,
>> pa_hashmap *h, pa_hashmap *ports) {
>>             cp->n_sources = pa_idxset_size(ap->input_mappings);
>>
>>             PA_IDXSET_FOREACH(m, ap->input_mappings, idx) {
>> -                pa_alsa_path_set_add_ports(m->input_path_set, cp, ports,
>> NULL, u->core);
>> +                if (u->use_ucm) {
>> +                    pdevices = pa_xnew(pa_alsa_ucm_device *,
>> pa_idxset_size(m->ucm_context.ucm_devices));
>> +                    pa_alsa_ucm_add_ports_combination(NULL,
>> &m->ucm_context,
>> +                            0, pdevices, 0, PA_IDXSET_INVALID, ports, cp,
>> u->core);
>> +                    pa_xfree(pdevices);
>
>
> pdevices is not used on any of the "root" calls to
> pa_alsa_ucm_add_ports_combination. It would be more elegant to add a simple
> wrapper around pa_alsa_ucm_add_ports_combination that would remove pdevices
> (and dev_num?) parameters from the call here.
Exactly.

>>
>> +static int card_query_ucm_profiles(struct userdata *u, int card_index)
>
>
> Any reason this function is not in alsa_ucm.h instead?
Caused by history.
>

>> +/** For devices: List of verbs, devices or modifiers available */
>> +#define PA_PROP_UCM_NAME                       "ucm.name"
>
>
> Is the device proplist accessible through the client API, and if so, how? If
> not, I don't think these names should be in the client API either.
I can't think about the client use cases now because ucm is absolutely
hidden behind. Again it's inherited from original source code, I will
move it inside alsa-ucm.h
>
>> +
>> +/** For devices: List of supported devices per verb*/
>> +#define PA_PROP_UCM_DESCRIPTION                "ucm.description"
>> +
>> +/** For devices: Playback device name e.g PlaybackPCM */
>> +#define PA_PROP_UCM_SINK                       "ucm.sink"
>> +
>> +/** For devices: Capture device name e.g CapturePCM*/
>> +#define PA_PROP_UCM_SOURCE                     "ucm.source"
>> +
>> +/** For devices: Playback roles */
>> +#define PA_PROP_UCM_PLAYBACK_ROLES             "ucm.playback.roles"
>> +
>> +/** For devices: Playback control volume ID string. e.g PlaybackVolume */
>> +#define PA_PROP_UCM_PLAYBACK_VOLUME            "ucm.playback.volume"
>> +
>> +/** For devices: Playback switch e.g PlaybackSwitch */
>> +#define PA_PROP_UCM_PLAYBACK_SWITCH            "ucm.playback.switch"
>> +
>> +/** For devices: Playback priority */
>> +#define PA_PROP_UCM_PLAYBACK_PRIORITY          "ucm.playback.priority"
>> +
>> +/** For devices: Playback channels */
>> +#define PA_PROP_UCM_PLAYBACK_CHANNELS          "ucm.playback.channels"
>> +
>> +/** For devices: Capture roles */
>> +#define PA_PROP_UCM_CAPTURE_ROLES              "ucm.capture.roles"
>> +
>> +/** For devices: Capture controls volume ID string. e.g CaptureVolume */
>> +#define PA_PROP_UCM_CAPTURE_VOLUME             "ucm.capture.volume"
>> +
>> +/** For devices: Capture switch e.g CaptureSwitch */
>> +#define PA_PROP_UCM_CAPTURE_SWITCH             "ucm.capture.switch"
>> +
>> +/** For devices: Capture priority */
>> +#define PA_PROP_UCM_CAPTURE_PRIORITY           "ucm.capture.priority"
>> +
>> +/** For devices: Capture channels */
>> +#define PA_PROP_UCM_CAPTURE_CHANNELS           "ucm.capture.channels"
>> +
>> +/** For devices: Quality of Service */
>> +#define PA_PROP_UCM_QOS                        "ucm.qos"
>> +
>>  /** For PCM formats: the sample format used as returned by
>> pa_sample_format_to_string() \since 1.0 */
>>  #define PA_PROP_FORMAT_SAMPLE_FORMAT           "format.sample_format"
>>
>
>
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic



-- 
Wei.Feng (irc wei_feng)
Linaro Multimedia Team
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog