[pulseaudio-discuss] [PATCH v2 3/3] filter-apply: Supports ladspa-sink and virtual-surround-sink properly

[KimJeongYeon]

2017-04-12 20:34 GMT+09:00 Georg Chini <georg at chini.tk>:

> The first two patches of the series look good to me (apart from
> spelling/grammar).
> A few comments to this patch below.

Ok, I did submit patch v3 again according to your review.

>>   @@ -140,6 +144,26 @@ static const char* should_filter(pa_object *o, bool
>> is_sink_input) {
>>       return NULL;
>>   }
>>   +static const char* should_filter_parameters(pa_object *o, const char
>> *want, bool is_sink_input) {
>
>
> I don't like the name of the function. Can't you call it
> get_filter_parameters() and maybe also
> rename should_filter() to get_filter_name()? I would expect a function
> starting with "should"
> to return a boolean.

You are right. Fixed,

>> +    const char *parameters;
>> +    char *prop_parameters;
>> +    pa_proplist *pl;
>> +
>> +    if (is_sink_input)
>> +        pl = PA_SINK_INPUT(o)->proplist;
>> +    else
>> +        pl = PA_SOURCE_OUTPUT(o)->proplist;
>> +
>> +    prop_parameters = pa_sprintf_malloc(PA_PROP_FILTER_APPLY_PARAMETERS,
>> want);
>> +    if ((parameters = pa_proplist_gets(pl, prop_parameters)) &&
>> !pa_streq(parameters, "")) {
>
>
> Why do you check for the empty string here? If this is not needed you could
> leave out
> the if () condition completely and just return parameters. As far as I can
> tell an empty
> string would not do any harm.

Fixed as you suggested.

>> @@ -517,7 +547,11 @@ static pa_hook_result_t process(struct userdata *u,
>> pa_object *o, bool is_put_or
>>     done:
>>       pa_xfree(module_name);
>> -    pa_xfree(fltr);
>> +    if (fltr) {
>> +        pa_xfree(fltr->name);
>> +        pa_xfree(fltr->parameters);
>> +        pa_xfree(fltr);
>> +    }
>>
>
>
> This is (partly) redundant with your "Fix memory leak" patch. As already
> said, I would use
> filter_free() here after modification.

Ok, it fixed,

Thanks for reviewing. :-)

Regards,
KimJeongYeon