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

[KimJeongYeon]

2017. 4. 19. 오전 5:10에 "Georg Chini" <georg at chini.tk>님이 작성:

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/20
>> 17-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


Ok. I'll do.

should do. Also skip_prop_change seems unnecessary.
>

About 'skip_prop_change':
Move operation happens twice when call 'move_objects_for_filter'. Because,
unexpected proplist-hook comes at 'do_move'.
But, it doesn't harm moving operation.
It would be remove 'skip_prop_change'.

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
>



Your logic seems fine for me.
I'll submit patch soon.

Thanks,
KimJeongYeon
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20170419/b9a55c02/attachment.html>