On 10/07/2013 05:39 PM, Arun Raghavan wrote:
> This adds mechanism to use a custom UCM value on verbs and modifiers to
> allow providing sink/source module arguments that are
> PulseAudio-speicific, such as mmap/tsched mode and ignore_dB.
> ---
> src/modules/alsa/alsa-mixer.c | 9 +++++++
> src/modules/alsa/alsa-mixer.h | 11 +++++++++
> src/modules/alsa/alsa-ucm.c | 41 +++++++++++++++++++++++++++----
> src/modules/alsa/alsa-ucm.h | 4 +++
> src/modules/alsa/module-alsa-card.c | 49 +++++++++++++++++++++++++++++++------
> 5 files changed, 101 insertions(+), 13 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
> index 71dfa79..b51f8ea 100644
> --- a/src/modules/alsa/alsa-mixer.c
> +++ b/src/modules/alsa/alsa-mixer.c
> @@ -3310,6 +3310,15 @@ static void mapping_free(pa_alsa_mapping *m) {
> pa_assert(!m->input_pcm);
> pa_assert(!m->output_pcm);
>
> + if (m->sink_arguments) {
> + pa_xfree(m->sink_arguments);
> + pa_modargs_free(m->sink_modargs);
Would be more resilient to future refactorings to do:
pa_xfree(m->sink_arguments);
if (m->sink_modargs) {
pa_modargs_free(m->sink_modargs);
> + }
> + if (m->source_arguments) {
> + pa_xfree(m->source_arguments);
> + pa_modargs_free(m->source_modargs);
> + }
> +
> pa_alsa_ucm_mapping_context_free(&m->ucm_context);
>
> pa_xfree(m);
> diff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h
> index 995a34b..79516db 100644
> --- a/src/modules/alsa/alsa-mixer.h
> +++ b/src/modules/alsa/alsa-mixer.h
> @@ -31,6 +31,7 @@
> #include <pulse/volume.h>
>
> #include <pulsecore/llist.h>
> +#include <pulsecore/modargs.h>
> #include <pulsecore/rtpoll.h>
>
> typedef struct pa_alsa_fdlist pa_alsa_fdlist;
> @@ -272,6 +273,16 @@ struct pa_alsa_mapping {
> pa_sink *sink;
> pa_source *source;
>
> + /* Module arguments from config, overriden by config/command line module
s/overriden/overridden
> + * arguments */
> + char *sink_arguments;
> + char *source_arguments;
> +
> + /* Parsed modargs, merged with the card module arguments. If null,
> + * signifies that card modargs should directly be used. */
> + pa_modargs *sink_modargs;
> + pa_modargs *source_modargs;
> +
> /* ucm device context*/
> pa_alsa_ucm_mapping_context ucm_context;
> };
> diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
> index a451c88..212942e 100644
> --- a/src/modules/alsa/alsa-ucm.c
> +++ b/src/modules/alsa/alsa-ucm.c
> @@ -92,6 +92,8 @@ static struct ucm_items item[] = {
> {"CaptureRate", PA_ALSA_PROP_UCM_CAPTURE_RATE},
> {"CaptureChannels", PA_ALSA_PROP_UCM_CAPTURE_CHANNELS},
> {"TQ", PA_ALSA_PROP_UCM_QOS},
> + {"PulseAudioSinkModargs", PA_ALSA_PROP_UCM_SINK_MODARGS},
> + {"PulseAudioSourceModargs", PA_ALSA_PROP_UCM_SOURCE_MODARGS},
Hmm, we should probably document this behaviour (including the name of
this key) somewhere...
Also since this is called alsa.ucm.source_arguments maybe
PulseAudioSourceArguments would be a more consistent name here.
> {NULL, NULL},
> };
>
> @@ -1244,6 +1246,7 @@ static int ucm_create_mapping_direction(
> pa_alsa_profile_set *ps,
> pa_alsa_profile *p,
> pa_alsa_ucm_device *device,
> + pa_alsa_ucm_verb *verb,
> const char *verb_name,
> const char *device_name,
> const char *device_str,
> @@ -1251,6 +1254,7 @@ static int ucm_create_mapping_direction(
>
> pa_alsa_mapping *m;
> char *mapping_name;
> + const char *value;
> unsigned priority, rate, channels;
>
> mapping_name = pa_sprintf_malloc("Mapping %s: %s: %s", verb_name, device_str, is_sink ? "sink" : "source");
> @@ -1281,6 +1285,16 @@ static int ucm_create_mapping_direction(
> if (rate)
> m->sample_spec.rate = rate;
> pa_channel_map_init_extend(&m->channel_map, channels, PA_CHANNEL_MAP_ALSA);
> +
> + if (is_sink) {
> + value = pa_proplist_gets(verb->proplist, PA_ALSA_PROP_UCM_SINK_MODARGS);
> + if (value)
> + m->sink_arguments = pa_xstrdup(value);
> + } else {
> + value = pa_proplist_gets(verb->proplist, PA_ALSA_PROP_UCM_SOURCE_MODARGS);
> + if (value)
> + m->source_arguments = pa_xstrdup(value);
> + }
> }
>
> /* mapping priority is the highest one of ucm devices */
> @@ -1301,6 +1315,7 @@ static int ucm_create_mapping_for_modifier(
> pa_alsa_profile_set *ps,
> pa_alsa_profile *p,
> pa_alsa_ucm_modifier *modifier,
> + pa_alsa_ucm_verb *verb,
> const char *verb_name,
> const char *mod_name,
> const char *device_str,
> @@ -1308,6 +1323,7 @@ static int ucm_create_mapping_for_modifier(
>
> pa_alsa_mapping *m;
> char *mapping_name;
> + const char *value;
>
> mapping_name = pa_sprintf_malloc("Mapping %s: %s: %s", verb_name, device_str, is_sink ? "sink" : "source");
>
> @@ -1333,6 +1349,20 @@ static int ucm_create_mapping_for_modifier(
> m->priority = 0;
>
> ucm_add_mapping(p, m);
> +
> + if (is_sink) {
> + value = pa_proplist_gets(modifier->proplist, PA_ALSA_PROP_UCM_SINK_MODARGS);
> + if (!value)
> + value = pa_proplist_gets(verb->proplist, PA_ALSA_PROP_UCM_SINK_MODARGS);
> + if (value)
> + m->sink_arguments = pa_xstrdup(value);
> + } else {
> + value = pa_proplist_gets(modifier->proplist, PA_ALSA_PROP_UCM_SOURCE_MODARGS);
> + if (!value)
> + value = pa_proplist_gets(verb->proplist, PA_ALSA_PROP_UCM_SOURCE_MODARGS);
> + if (value)
> + m->source_arguments = pa_xstrdup(value);
> + }
> } else if (!m->ucm_context.ucm_modifiers) /* share pcm with device */
> m->ucm_context.ucm_modifiers = pa_idxset_new(pa_idxset_trivial_hash_func, pa_idxset_trivial_compare_func);
>
> @@ -1346,6 +1376,7 @@ static int ucm_create_mapping(
> pa_alsa_profile_set *ps,
> pa_alsa_profile *p,
> pa_alsa_ucm_device *device,
> + pa_alsa_ucm_verb *verb,
> const char *verb_name,
> const char *device_name,
> const char *sink,
> @@ -1359,9 +1390,9 @@ static int ucm_create_mapping(
> }
>
> if (sink)
> - ret = ucm_create_mapping_direction(ucm, ps, p, device, verb_name, device_name, sink, true);
> + ret = ucm_create_mapping_direction(ucm, ps, p, device, verb, verb_name, device_name, sink, true);
Hmm, maybe we should consider having the verb_name inside the
pa_alsa_ucm_verb struct?
> if (ret == 0 && source)
> - ret = ucm_create_mapping_direction(ucm, ps, p, device, verb_name, device_name, source, false);
> + ret = ucm_create_mapping_direction(ucm, ps, p, device, verb, verb_name, device_name, source, false);
>
> return ret;
> }
> @@ -1444,7 +1475,7 @@ static int ucm_create_profile(
> sink = pa_proplist_gets(dev->proplist, PA_ALSA_PROP_UCM_SINK);
> source = pa_proplist_gets(dev->proplist, PA_ALSA_PROP_UCM_SOURCE);
>
> - ucm_create_mapping(ucm, ps, p, dev, verb_name, name, sink, source);
> + ucm_create_mapping(ucm, ps, p, dev, verb, verb_name, name, sink, source);
>
> if (sink)
> dev->output_jack = ucm_get_jack(ucm, name, PA_UCM_PRE_TAG_OUTPUT);
> @@ -1461,9 +1492,9 @@ static int ucm_create_profile(
> source = pa_proplist_gets(mod->proplist, PA_ALSA_PROP_UCM_SOURCE);
>
> if (sink)
> - ucm_create_mapping_for_modifier(ucm, ps, p, mod, verb_name, name, sink, true);
> + ucm_create_mapping_for_modifier(ucm, ps, p, mod, verb, verb_name, name, sink, true);
> else if (source)
> - ucm_create_mapping_for_modifier(ucm, ps, p, mod, verb_name, name, source, false);
> + ucm_create_mapping_for_modifier(ucm, ps, p, mod, verb, verb_name, name, source, false);
> }
>
> pa_alsa_profile_dump(p);
> diff --git a/src/modules/alsa/alsa-ucm.h b/src/modules/alsa/alsa-ucm.h
> index bae6b72..00adc56 100644
> --- a/src/modules/alsa/alsa-ucm.h
> +++ b/src/modules/alsa/alsa-ucm.h
> @@ -83,6 +83,10 @@ typedef void snd_use_case_mgr_t;
> /** For devices: Quality of Service */
> #define PA_ALSA_PROP_UCM_QOS "alsa.ucm.qos"
>
> +/** For verbs/devices/modifiers: Passthrough module arguments for the corresponding Playback/CapturePCM */
> +#define PA_ALSA_PROP_UCM_SINK_MODARGS "alsa.ucm.sink_arguments"
> +#define PA_ALSA_PROP_UCM_SOURCE_MODARGS "alsa.ucm.source_arguments"
> +
> /** For devices: The modifier (if any) that this device corresponds to */
> #define PA_ALSA_PROP_UCM_MODIFIER "alsa.ucm.modifier"
>
> diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
> index 970344a..a14c94d 100644
> --- a/src/modules/alsa/module-alsa-card.c
> +++ b/src/modules/alsa/module-alsa-card.c
> @@ -253,7 +253,7 @@ static int card_set_profile(pa_card *c, pa_card_profile *new_profile) {
> PA_IDXSET_FOREACH(am, nd->profile->output_mappings, idx) {
>
> if (!am->sink)
> - am->sink = pa_alsa_sink_new(c->module, u->modargs, __FILE__, c, am);
> + am->sink = pa_alsa_sink_new(c->module, am->sink_modargs ? am->sink_modargs : u->modargs, __FILE__, c, am);
>
> if (sink_inputs && am->sink) {
> pa_sink_move_all_finish(am->sink, sink_inputs, false);
> @@ -265,7 +265,7 @@ static int card_set_profile(pa_card *c, pa_card_profile *new_profile) {
> PA_IDXSET_FOREACH(am, nd->profile->input_mappings, idx) {
>
> if (!am->source)
> - am->source = pa_alsa_source_new(c->module, u->modargs, __FILE__, c, am);
> + am->source = pa_alsa_source_new(c->module, am->source_modargs ? am->source_modargs : u->modargs, __FILE__, c, am);
>
> if (source_outputs && am->source) {
> pa_source_move_all_finish(am->source, source_outputs, false);
> @@ -282,11 +282,26 @@ static int card_set_profile(pa_card *c, pa_card_profile *new_profile) {
> return 0;
> }
>
> +static pa_modargs* prepare_modargs(const char *module, const char *config) {
Maybe this could be a utility function inside modargs instead - make it
more generic like
pa_modargs_from_two(arg1, arg2, valid_keys);
> + pa_modargs *ma;
> +
> + /* It's easier to re-parse the module arguments than to do a deep modargs copy */
> + ma = pa_modargs_new(module, valid_modargs);
> + if (!ma)
> + return NULL;
> +
> + if (pa_modargs_append(ma, config, valid_modargs) < 0)
> + pa_log_error("Error parsing modargs from configuration");
> +
> + return ma;
> +}
> +
> static void init_profile(struct userdata *u) {
> uint32_t idx;
> pa_alsa_mapping *am;
> struct profile_data *d;
> pa_alsa_ucm_config *ucm = &u->ucm;
> + pa_modargs *modargs;
>
> pa_assert(u);
>
> @@ -300,13 +315,31 @@ static void init_profile(struct userdata *u) {
> }
> }
>
> - if (d->profile && d->profile->output_mappings)
> - PA_IDXSET_FOREACH(am, d->profile->output_mappings, idx)
> - am->sink = pa_alsa_sink_new(u->module, u->modargs, __FILE__, u->card, am);
> + modargs = u->modargs;
> +
> + if (d->profile && d->profile->output_mappings) {
> + PA_IDXSET_FOREACH(am, d->profile->output_mappings, idx) {
> + if (am->sink_arguments) {
> + am->sink_modargs = prepare_modargs(u->module->argument, am->sink_arguments);
Unless I'm missing something, this looks like the only place
am->sink_modargs is assigned. And this initializes only one of the
profiles. What about the other profiles?
> + pa_assert(am->sink_modargs);
> + modargs = am->sink_modargs;
> + }
> +
> + am->sink = pa_alsa_sink_new(u->module, modargs, __FILE__, u->card, am);
> + }
> + }
> +
> + if (d->profile && d->profile->input_mappings) {
> + PA_IDXSET_FOREACH(am, d->profile->input_mappings, idx) {
> + if (am->source_arguments) {
> + am->source_modargs = prepare_modargs(u->module->argument, am->source_arguments);
> + pa_assert(am->source_modargs);
> + modargs = am->source_modargs;
> + }
>
> - if (d->profile && d->profile->input_mappings)
> - PA_IDXSET_FOREACH(am, d->profile->input_mappings, idx)
> - am->source = pa_alsa_source_new(u->module, u->modargs, __FILE__, u->card, am);
> + am->source = pa_alsa_source_new(u->module, modargs, __FILE__, u->card, am);
> + }
> + }
> }
>
> static void report_port_state(pa_device_port *p, struct userdata *u) {
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
