[pulseaudio-discuss] [PATCH v5] filter-apply: Fixed a stream moves to wrong sink or source.

[Georg Chini]

On 19.04.2017 11:03, KimJeongYeon wrote:
> For example, a normal stream tried to attach to filter sink or source, which
> filter loaded and managed by filter-apply. But, the stream goes to filter's
> ***master sink or source*** due to unexpected restoring operation.
> It should attached to filter sink or source properly.
>
> Also, includes further fix according to Georg's comment, [1]
>    If a stream that had filter.apply set initially is moved to another sink/source, then the filter
>    should be applied again (a new filter, since the master sink/source has changed).
>
>    If a stream that did not have filter.apply set initially is moved away, the property should
>    be removed and no new filter applied.
>
>    Also, a property list change might add or remove the filter.apply property. If it is added,
>    we want that the filter is applied. Your patch does nothing and assumes that the stream
>    is already filtered, even if the stream is not on a filter sink.
>
> [1] https://lists.freedesktop.org/archives/pulseaudio-discuss/2017-April/027980.html
>
> Signed-off-by: KimJeongYeon <jeongyeon.kim at samsung.com>

We are nearly there. Still some comments ...

> ---
>   src/modules/module-filter-apply.c | 107 +++++++++++++++++++++++++++++++-------
>   1 file changed, 89 insertions(+), 18 deletions(-)
>
> diff --git a/src/modules/module-filter-apply.c b/src/modules/module-filter-apply.c
> index e91fb37..562221a 100644
> --- a/src/modules/module-filter-apply.c
> +++ b/src/modules/module-filter-apply.c
> @@ -39,6 +39,7 @@
>   
>   #define PA_PROP_FILTER_APPLY_PARAMETERS PA_PROP_FILTER_APPLY".%s.parameters"
>   #define PA_PROP_FILTER_APPLY_MOVING     "filter.apply.moving"
> +#define PA_PROP_FILTER_APPLY_SET_BY_MFA "filter.apply.set_by_mfa"
>   #define PA_PROP_MDM_AUTO_FILTERED       "module-device-manager.auto_filtered"
>   
>   PA_MODULE_AUTHOR("Colin Guthrie");
> @@ -161,6 +162,53 @@ static const char* get_filter_parameters(pa_object *o, const char *want, bool is
>       return parameters;
>   }
>   

This function is very difficult to read. I'd rename "is_set" to 
"set_properties" and also
add another boolean "set_by_mfa" so that you set/unset all filter 
properties within
this function. More changes below.

> +static void set_filter_properties(pa_proplist *pl, struct filter* filter, bool is_set) {
> +    const char *apply;

Rename the variable "apply" to "old_filter_name", initialize it to NULL 
and move
the definition into the else clause.

> +    char *prop_parameters;
> +
> +    if (is_set) {
> +        pa_assert(filter);
> +
> +        pa_proplist_sets(pl, PA_PROP_FILTER_APPLY, filter->name);
> +        if (filter->parameters) {
> +            prop_parameters = pa_sprintf_malloc(PA_PROP_FILTER_APPLY_PARAMETERS, filter->name);
> +            pa_proplist_sets(pl, prop_parameters, filter->parameters);
> +            pa_xfree(prop_parameters);
> +        }

If "set_by_mfa" is true, you can set the filter.apply.set_by_mfa 
property here.

> +    } else {
> +        if (filter)
> +            prop_parameters = pa_sprintf_malloc(PA_PROP_FILTER_APPLY_PARAMETERS, filter->name);
> +        else {
> +            if (!(apply = pa_proplist_gets(pl, PA_PROP_FILTER_APPLY)))
> +                return;
> +            prop_parameters = pa_sprintf_malloc(PA_PROP_FILTER_APPLY_PARAMETERS, apply);
> +        }

To make it more easy to read, I would change the code above like this:

          if (filter)
              old_filter_name = filter->name;
          else
              old_filter_name = pa_proplist_gets(pl, PA_PROP_FILTER_APPLY);

          if (!old_filter_name)
             /* Filter name cannot be determined, nothing to do */
             return
          prop_parameters = 
pa_sprintf_malloc(PA_PROP_FILTER_APPLY_PARAMETERS, old_filter_name);

> +        pa_proplist_unset(pl, prop_parameters);
> +        pa_xfree(prop_parameters);
> +        pa_proplist_unset(pl, PA_PROP_FILTER_APPLY);

Here you can unconditionally unset the filter.apply.set_by_mfa property.

> +    }
> +}
> +
> +static struct filter* get_attached_filter(struct userdata *u, pa_object *o, bool is_sink_input) {

Maybe I would rename the function to get_filter_for_object(). But I 
don't mind if you
keep the name.

> +    pa_sink *sink = NULL;
> +    pa_source *source = NULL;
> +    struct filter *filter = NULL;
> +    void *state;
> +
> +    if (is_sink_input)
> +        sink = PA_SINK_INPUT(o)->sink;
> +    else
> +        source = PA_SOURCE_OUTPUT(o)->source;
> +
> +    PA_HASHMAP_FOREACH(filter, u->filters, state) {
> +        if ((is_sink_input && sink == filter->sink) || (!is_sink_input && source == filter->source)) {
> +            return filter;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>   static bool should_group_filter(struct filter *filter) {
>       return pa_streq(filter->name, "echo-cancel");
>   }
> @@ -431,7 +479,7 @@ static bool can_unload_module(struct userdata *u, uint32_t idx) {
>       return true;
>   }
>   
> -static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_sink_input) {
> +static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_sink_input, bool is_put_or_move) {

I would prefer "is_property_change" instead of "is_put_or_move". But again,
I don't mind if you keep it.


 From your other mail:

I think it is able to eliminate redundant moving by check
PA_PROP_FILTER_APPLY_MOVING at 'sink_input_proplist_cb'.
No more need additional 'skip_prop_change' flag.

code:
     if (pa_proplist_gets(i->proplist, PA_PROP_FILTER_APPLY_MOVING))
         return PA_HOOK_OK;

How do you think? Are you agree that would be fix at new patch?

Yes, that looks like a good idea.

I hope you don't mind if I change the commit message like I did for
your other patches and add a few more comments to the code.