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>
