[pulseaudio-discuss] [PATCH v6] filter-apply: Fixed a stream moves to wrong sink or source.
Georg Chini
georg at chini.tk
Thu Apr 20 18:27:59 UTC 2017
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) {
More information about the pulseaudio-discuss
mailing list