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 <[email protected]>
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.
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
