On 20.04.2017 16:35, 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> Still found a few issues, but I think the next version will be final. > --- > src/modules/module-filter-apply.c | 121 +++++++++++++++++++++++++++++++------- > 1 file changed, 101 insertions(+), 20 deletions(-) > > diff --git a/src/modules/module-filter-apply.c b/src/modules/module-filter-apply.c > index e91fb37..fbe41b8 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,66 @@ static const char* get_filter_parameters(pa_object *o, const char *want, bool is > return parameters; > } > > +static void set_filter_properties(pa_proplist *pl, struct filter *filter, bool set_properties, bool set_by_mfa) { You either call that function with ..., false, false) or with ..., true, true), so the "set_by_mfa" parameter is in fact not necessary. You can set/unset PA_PROP_FILTER_APPLY_SET_BY_MFA unconditionally. > + char *prop_parameters; > + > + if (set_properties) { > + 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) > + pa_proplist_sets(pl, PA_PROP_FILTER_APPLY_SET_BY_MFA, "1"); > + } else { > + const char *old_filter_name = NULL; > + > + 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); > + > + if (!set_by_mfa) > + pa_proplist_unset(pl, PA_PROP_FILTER_APPLY_SET_BY_MFA); > + } > +} > + > +static struct filter* get_filter_for_object(struct userdata *u, pa_object *o, bool is_sink_input) { > + 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"); > } > @@ -309,7 +370,7 @@ static int do_move(struct userdata *u, pa_object *obj, pa_object *parent, bool i > } > } > > -static void move_object_for_filter(struct userdata *u, pa_object *o, struct filter* filter, bool restore, bool is_sink_input) { > +static void move_object_for_filter(struct userdata *u, pa_object *o, struct filter *filter, bool restore, bool is_sink_input) { > pa_object *parent; > pa_proplist *pl; > const char *name; > @@ -343,7 +404,7 @@ static void move_object_for_filter(struct userdata *u, pa_object *o, struct filt > pa_proplist_unset(pl, PA_PROP_FILTER_APPLY_MOVING); > } > > -static void move_objects_for_filter(struct userdata *u, pa_object *o, struct filter* filter, bool restore, > +static void move_objects_for_filter(struct userdata *u, pa_object *o, struct filter *filter, bool restore, > bool is_sink_input) { > > if (!should_group_filter(filter)) > @@ -431,7 +492,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_property_change) { > const char *want; > const char *parameters; > bool done_something = false; > @@ -440,17 +501,16 @@ static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_sink_i > pa_module *module = NULL; > char *module_name = NULL; > struct filter *fltr = NULL, *filter = NULL; > + pa_proplist *pl; > > if (is_sink_input) { > - sink = PA_SINK_INPUT(o)->sink; > - > - if (sink) > + if ((sink = PA_SINK_INPUT(o)->sink)) > module = sink->module; > + pl = PA_SINK_INPUT(o)->proplist; > } else { > - source = PA_SOURCE_OUTPUT(o)->source; > - > - if (source) > + if ((source = PA_SOURCE_OUTPUT(o)->source)) > module = source->module; > + pl = PA_SOURCE_OUTPUT(o)->proplist; > } > > /* If there is no sink/source yet, we can't do much */ > @@ -471,6 +531,23 @@ static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_sink_i > goto done; > } > > + if (pa_proplist_gets(pl, PA_PROP_FILTER_APPLY_SET_BY_MFA)) { > + /* The stream has been manually moved away from the filter. > + * Keep sink/source as is. */ > + > + if ((filter = get_filter_for_object(u, o, is_sink_input))) { > + /* Change 'filter.apply'. */ > + set_filter_properties(pl, NULL, false, false); > + set_filter_properties(pl, filter, true, true); > + } else { > + set_filter_properties(pl, NULL, false, false); > + } You can move the "set_filter_properties(pl, NULL, false, false);" before the if() because you are doing it in both branches. > + > + trigger_housekeeping(u); > + return PA_HOOK_OK; /* goto done; */ You forget to free module_name. I would move the "done" label before the "if (done_something)" and do "done_something=true; goto done" here. > + } > + /* Else, normal case. The stream should be moved to a filter. */ > + > /* Some filter modules might require parameters by default. > * (e.g 'plugin', 'label', 'control' of module-ladspa-sink) */ > parameters = get_filter_parameters(o, want, is_sink_input); > @@ -515,15 +592,19 @@ static pa_hook_result_t process(struct userdata *u, pa_object *o, bool is_sink_i > done_something = true; > } > } else { > - void *state; > - > /* We do not want to filter... but are we already filtered? > * This can happen if an input's proplist changes */ > - PA_HASHMAP_FOREACH(filter, u->filters, state) { > - if ((is_sink_input && sink == filter->sink) || (!is_sink_input && source == filter->source)) { > + > + if ((filter = get_filter_for_object(u, o, is_sink_input))) { > + if (is_property_change) { > + /* 'filter.apply' has been manually unset. Do restore. */ > move_objects_for_filter(u, o, filter, true, is_sink_input); > + set_filter_properties(pl, filter, false, false); > done_something = true; > - break; > + } else { > + /* Stream has been manually moved to a filter sink/source > + * without 'filter.apply' set. Leave sink as it is. */ > + set_filter_properties(pl, filter, true, true); > } > } > } > @@ -542,7 +623,7 @@ static pa_hook_result_t sink_input_put_cb(pa_core *core, pa_sink_input *i, struc > pa_core_assert_ref(core); > pa_sink_input_assert_ref(i); > > - return process(u, PA_OBJECT(i), true); > + return process(u, PA_OBJECT(i), true, false); > } > > static pa_hook_result_t sink_input_move_finish_cb(pa_core *core, pa_sink_input *i, struct userdata *u) { > @@ -555,14 +636,14 @@ static pa_hook_result_t sink_input_move_finish_cb(pa_core *core, pa_sink_input * > /* If we're managing m-d-m.auto_filtered on this, remove and re-add if we're continuing to manage it */ > pa_hashmap_remove(u->mdm_ignored_inputs, i); > > - return process(u, PA_OBJECT(i), true); > + return process(u, PA_OBJECT(i), true, false); > } > > static pa_hook_result_t sink_input_proplist_cb(pa_core *core, pa_sink_input *i, struct userdata *u) { > pa_core_assert_ref(core); > pa_sink_input_assert_ref(i); Didn't you want to avoid the double move here? Or will that be another patch? > > - return process(u, PA_OBJECT(i), true); > + return process(u, PA_OBJECT(i), true, true); > } > > static pa_hook_result_t sink_input_unlink_cb(pa_core *core, pa_sink_input *i, struct userdata *u) { > @@ -619,7 +700,7 @@ static pa_hook_result_t source_output_put_cb(pa_core *core, pa_source_output *o, > pa_core_assert_ref(core); > pa_source_output_assert_ref(o); > > - return process(u, PA_OBJECT(o), false); > + return process(u, PA_OBJECT(o), false, false); > } > > static pa_hook_result_t source_output_move_finish_cb(pa_core *core, pa_source_output *o, struct userdata *u) { > @@ -632,14 +713,14 @@ static pa_hook_result_t source_output_move_finish_cb(pa_core *core, pa_source_ou > /* If we're managing m-d-m.auto_filtered on this, remove and re-add if we're continuing to manage it */ > pa_hashmap_remove(u->mdm_ignored_outputs, o); > > - return process(u, PA_OBJECT(o), false); > + return process(u, PA_OBJECT(o), false, false); > } > > static pa_hook_result_t source_output_proplist_cb(pa_core *core, pa_source_output *o, struct userdata *u) { > pa_core_assert_ref(core); > pa_source_output_assert_ref(o); Didn't you want to avoid the double move here? Or will that be another patch? > > - return process(u, PA_OBJECT(o), false); > + return process(u, PA_OBJECT(o), false, true); > } > > static pa_hook_result_t source_output_unlink_cb(pa_core *core, pa_source_output *o, struct userdata *u) {
