[PATCH v2 3/3] filter-apply: Supports ladspa-sink and virtual-surround-sink properly

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10.04.2017 15:59, KimJeongYeon wrote:
> filter-apply able to load ladspa-sink, virtual-surround-sink using new
> 'filter.apply.{MODULE_NAME}.parameter' property.
> e.g) paplay file.wav --property=filter.apply=ladspa-sink \
>                       --property=filter.apply.ladspa-sink.parameters="plugin=ladspa label=ladspa_stereo control=0"
>
> Signed-off-by: KimJeongYeon <jeongyeon.kim at samsung.com>

The first two patches of the series look good to me (apart from 
spelling/grammar).
A few comments to this patch below.

> ---
>   src/modules/module-filter-apply.c | 56 +++++++++++++++++++++++++++++++--------
>   1 file changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/src/modules/module-filter-apply.c b/src/modules/module-filter-apply.c
> index c5fd9de..a7eab40 100644
> --- a/src/modules/module-filter-apply.c
> +++ b/src/modules/module-filter-apply.c
> @@ -37,8 +37,9 @@
>   
>   #include "module-filter-apply-symdef.h"
>   
> -#define PA_PROP_FILTER_APPLY_MOVING "filter.apply.moving"
> -#define PA_PROP_MDM_AUTO_FILTERED   "module-device-manager.auto_filtered"
> +#define PA_PROP_FILTER_APPLY_PARAMETERS PA_PROP_FILTER_APPLY".%s.parameters"
> +#define PA_PROP_FILTER_APPLY_MOVING     "filter.apply.moving"
> +#define PA_PROP_MDM_AUTO_FILTERED       "module-device-manager.auto_filtered"
>   
>   PA_MODULE_AUTHOR("Colin Guthrie");
>   PA_MODULE_DESCRIPTION("Load filter sinks automatically when needed");
> @@ -56,6 +57,7 @@ static const char* const valid_modargs[] = {
>   
>   struct filter {
>       char *name;
> +    char *parameters;
>       uint32_t module_index;
>       pa_sink *sink;
>       pa_sink *sink_master;
> @@ -97,13 +99,14 @@ static int filter_compare(const void *a, const void *b) {
>       return 0;
>   }
>   
> -static struct filter *filter_new(const char *name, pa_sink *sink, pa_source *source) {
> +static struct filter *filter_new(const char *name, const char *parameters, pa_sink *sink, pa_source *source) {
>       struct filter *f;
>   
>       pa_assert(sink || source);
>   
>       f = pa_xnew(struct filter, 1);
>       f->name = pa_xstrdup(name);
> +    f->parameters = pa_xstrdup(parameters);
>       f->sink_master = sink;
>       f->source_master = source;
>       f->module_index = PA_INVALID_INDEX;
> @@ -117,6 +120,7 @@ static void filter_free(struct filter *f) {
>       pa_assert(f);
>   
>       pa_xfree(f->name);
> +    pa_xfree(f->parameters);
>       pa_xfree(f);
>   }
>   
> @@ -140,6 +144,26 @@ static const char* should_filter(pa_object *o, bool is_sink_input) {
>       return NULL;
>   }
>   
> +static const char* should_filter_parameters(pa_object *o, const char *want, bool is_sink_input) {

I don't like the name of the function. Can't you call it 
get_filter_parameters() and maybe also
rename should_filter() to get_filter_name()? I would expect a function 
starting with "should"
to return a boolean.

> +    const char *parameters;
> +    char *prop_parameters;
> +    pa_proplist *pl;
> +
> +    if (is_sink_input)
> +        pl = PA_SINK_INPUT(o)->proplist;
> +    else
> +        pl = PA_SOURCE_OUTPUT(o)->proplist;
> +
> +    prop_parameters = pa_sprintf_malloc(PA_PROP_FILTER_APPLY_PARAMETERS, want);
> +    if ((parameters = pa_proplist_gets(pl, prop_parameters)) && !pa_streq(parameters, "")) {

Why do you check for the empty string here? If this is not needed you 
could leave out
the if () condition completely and just return parameters. As far as I 
can tell an empty
string would not do any harm.

> +        pa_xfree(prop_parameters);
> +        return parameters;
> +    }
> +
> +    pa_xfree(prop_parameters);
> +    return NULL;
> +}
> +
>   static bool should_group_filter(struct filter *filter) {
>       return pa_streq(filter->name, "echo-cancel");
>   }
> @@ -359,7 +383,7 @@ static void move_objects_for_filter(struct userdata *u, pa_object *o, struct fil
>   
>   /* Note that we assume a filter will provide at most one sink and at most one
>    * source (and at least one of either). */
> -static void find_filters_for_module(struct userdata *u, pa_module *m, const char *name) {
> +static void find_filters_for_module(struct userdata *u, pa_module *m, const char *name, const char *parameters) {
>       uint32_t idx;
>       pa_sink *sink;
>       pa_source *source;
> @@ -369,7 +393,7 @@ static void find_filters_for_module(struct userdata *u, pa_module *m, const char
>           if (sink->module == m) {
>               pa_assert(pa_sink_is_filter(sink));
>   
> -            fltr = filter_new(name, sink->input_to_master->sink, NULL);
> +            fltr = filter_new(name, parameters, sink->input_to_master->sink, NULL);
>               fltr->module_index = m->index;
>               fltr->sink = sink;
>   
> @@ -382,7 +406,7 @@ static void find_filters_for_module(struct userdata *u, pa_module *m, const char
>               pa_assert(pa_source_is_filter(source));
>   
>               if (!fltr) {
> -                fltr = filter_new(name, NULL, source->output_from_master->source);
> +                fltr = filter_new(name, parameters, NULL, source->output_from_master->source);
>                   fltr->module_index = m->index;
>                   fltr->source = source;
>               } else {
> @@ -412,6 +436,7 @@ static bool can_unload_module(struct userdata *u, uint32_t idx) {
>   
>   static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_put_or_move, bool is_sink_input) {
>       const char *want;
> +    const char *parameters;
>       bool done_something = false;
>       pa_sink *sink = NULL;
>       pa_source *source = NULL;
> @@ -449,7 +474,11 @@ static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_put_or
>               goto done;
>           }
>   
> -        fltr = filter_new(want, sink, source);
> +        /* Some filter modules might require parameters by default.
> +         * (e.g 'plugin', 'label', 'control' of module-ladspa-sink) */
> +        parameters = should_filter_parameters(o, want, is_sink_input);
> +
> +        fltr = filter_new(want, parameters, sink, source);
>   
>           if (should_group_filter(fltr) && !find_paired_master(u, fltr, o, is_sink_input)) {
>               pa_log_debug("Want group filtering but don't have enough streams.");
> @@ -460,16 +489,17 @@ static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_put_or
>               char *args;
>               pa_module *m;
>   
> -            args = pa_sprintf_malloc("autoloaded=1 %s%s %s%s",
> +            args = pa_sprintf_malloc("autoloaded=1 %s%s %s%s %s",
>                       fltr->sink_master ? "sink_master=" : "",
>                       fltr->sink_master ? fltr->sink_master->name : "",
>                       fltr->source_master ? "source_master=" : "",
> -                    fltr->source_master ? fltr->source_master->name : "");
> +                    fltr->source_master ? fltr->source_master->name : "",
> +                    fltr->parameters ? fltr->parameters : "");
>   
>               pa_log_debug("Loading %s with arguments '%s'", module_name, args);
>   
>               if ((m = pa_module_load(u->core, module_name, args))) {
> -                find_filters_for_module(u, m, want);
> +                find_filters_for_module(u, m, want, parameters);
>                   filter = pa_hashmap_get(u->filters, fltr);
>                   done_something = true;
>               }
> @@ -517,7 +547,11 @@ static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_put_or
>   
>   done:
>       pa_xfree(module_name);
> -    pa_xfree(fltr);
> +    if (fltr) {
> +        pa_xfree(fltr->name);
> +        pa_xfree(fltr->parameters);
> +        pa_xfree(fltr);
> +    }
>   

This is (partly) redundant with your "Fix memory leak" patch. As already 
said, I would use
filter_free() here after modification.

>       return PA_HOOK_OK;
>   }




[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux