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.
