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

[Georg Chini]

On 18.04.2017 21:45, Georg Chini wrote:
> On 18.04.2017 14:36, 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>
> Hi JeongYeon,
>
> sorry, but I still don't agree with your patch. As already said you do 
> not need the
> enumeration, a simple boolean should do. Also skip_prop_change seems 
> unnecessary.
> I would name the new property filter.apply.mfa_loaded because the 
> other properties
> have a filter.apply prefix (although I suggested the other name myself).
> I think the following logic is much simpler than your approach and 
> still covers all cases
> (I always wrote sink but naturally the same applies for the source). 
> Please correct
> me if I have overlooked anything.
>
> static pa_hook_result_t process(struct userdata *u, pa_object *o, bool 
> is_sink_input) {
>
>    ... leave the first bit unchanged ...
>
>     /* Check if filter.apply property is set and get filter name. */
>     if ((want = get_filter_name(o, is_sink_input))) {
>
>         /* We end up here, when filter.apply is set */
>
>         /* If module is not set, we don't know which type of sink we 
> are on,
>          * therefore do not try to do anything. (Not sure if this is 
> right. The
>          * filter sinks should always have module set, so we could 
> conclude
>          * that unset means that we are not on a filter sink and can 
> just skip
>          * the next check. But let's leave it like it is for the 
> moment.) */
>         if (!module)
>             goto done;
>
>         /* If we are already on the right filter sink, we don't need 
> to do anything */
>         module_name = pa_sprintf_malloc("module-%s", want);
>         if (pa_streq(module->name, module_name)) {
>             pa_log_debug("Stream appears to be playing on an 
> appropriate sink already. Ignoring.");
>             goto done;
>         }
>
>         /* Now filter apply is set and the sink is not the right 
> filter sink.
>          * We have two possible cases left: */
>
>         /* 1) The stream has filter.apply.mfa_loaded set. This means 
> the stream has been
>          *      manually moved away from the filter. */
>
>         if (property filter.apply.mfa_loaded) {

I have forgotten one possible case here: The stream may have moved from 
one filter sink to another.
In this case, filter.apply should be changed to the new filter and 
filter.apply.mfa_loaded should not be
removed. So it should be:

                 if (sink is filter sink) {
                     change filter.apply
                 } else {
                     remove filter.apply
                     remove filter.apply.mfa_loaded
                 }
                 keep sink as is

instead of

>             Remove filter.apply
>             Remove filter.apply.mfa_loaded
>             keep sink as is