[FFmpeg-devel] [PATCH 2/2] avfilter: add sidechaingate filter

Sat Dec 5 14:33:03 CET 2015

On Sat, Dec 5, 2015 at 1:20 AM, Paul B Mahol <onemda at gmail.com> wrote:
> On 12/4/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> On Fri, Dec 4, 2015 at 3:49 PM, Paul B Mahol <onemda at gmail.com> wrote:
>>> On 12/4/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>>> On Fri, Dec 4, 2015 at 11:36 AM, Paul B Mahol <onemda at gmail.com> wrote:
>>>>> On 12/4/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>>>>> On Wed, Dec 2, 2015 at 1:42 AM, Paul B Mahol <onemda at gmail.com> wrote:
>>>>>>> On 12/2/15, Paul B Mahol <onemda at gmail.com> wrote:
>>>>>>>> On 12/2/15, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>>>>>>>> On Tue, Dec 1, 2015 at 2:14 PM, Paul B Mahol <onemda at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>> On 12/1/15, Nicolas George <george at nsup.org> wrote:
>>>>>>>>>>> Le primidi 11 frimaire, an CCXXIV, Paul B Mahol a ecrit :
>>>>>>>>>>>> Similar how its freed when no longer used.
>>>>>>>>>>>
>>>>>>>>>>> Please elaborate. I know the API, I do not see what you suggest.
>>>>>>>>>>>
>>>>>>>>>>> (Thanks for trimming.)
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>
>>>>>>>>>> After carefully looking at this functon, I see no leaking at all.
>>>>>>>>>
>>>>>>>>> Care to elaborate on this? My question is: what happens when e.g
>>>>>>>>> layouts get set and allocated/ref'ed correctly, but while trying to
>>>>>>>>> allocate formats, ENOMEM occurs? Or in other words, where does the
>>>>>>>>> formats get deallocated in such a case? And why does Coverity flag
>>>>>>>>> these things?
>>>>>>>>
>>>>>>>> Maybe it is for test program: libavfilter/filtfmts.c
>>>>>>>>
>>>>>>>
>>>>>>> Also I changed return value of query_formats to always be -1 and run
>>>>>>> program under 'valgrind --leak-check=full --show-leak-kinds=all'
>>>>>>> I see unrelated leaks from af_aformat and others, and no leaks from
>>>>>>> query_formats.
>>>>>>
>>>>>> You are being ambiguous here, and your testing was likely not thorough
>>>>>> enough.
>>>>>>
>>>>>> Please try the following:
>>>>>> diff --git a/libavfilter/af_agate.c b/libavfilter/af_agate.c
>>>>>> index 291e803..416b671 100644
>>>>>> --- a/libavfilter/af_agate.c
>>>>>> +++ b/libavfilter/af_agate.c
>>>>>> @@ -188,6 +188,7 @@ static int query_formats(AVFilterContext *ctx)
>>>>>>      layouts = ff_all_channel_counts();
>>>>>>      if (!layouts)
>>>>>>          return AVERROR(ENOMEM);
>>>>>> +    return AVERROR(ENOMEM);
>>>>>
>>>>> This is nonsense.
>>>>
>>>> You have a very bad habit of calling something "nonsense". I can reply
>>>> in kind saying that your original "carefully looking at this function"
>>>> was nonsense, since the leak is demonstrated below. Your test was with
>>>> a return -1, which is also nonsensical.
>>>>
>>>> I moreover did say it is not a proper illustration, and gave a better
>>>> one with instructions to reproduce below. Please stop your habit of
>>>> selectively replying and thus creating biased perspectives for a
>>>> casual observer of what I point out. Your code was broken wrt
>>>> memleaks, stop defending it, test the example I gave, and work towards
>>>> creating a constructive solution.
>>>
>>> First provide actual sane patch that demonstrate that leak actually does
>>> happen.
>>
>> If you actually bothered to read the message I posted fully instead of
>> posting comments like "nonsense", you would have seen such a patch.
>>
>>>
>>> Allocating something and than returning immediately error and then saying
>>> look your code sucks is very unfriendly.
>>
>> No. What is unfriendly is the following:
>> 1. You not addressing a reviewer at the time when a patch is posted
>> and pushing to master, and the associated hypocrisy wrt the dev policy
>> since you joined the fracas regarding "pushing without review"
>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/182511.html)
>> which was not even true while doing so freely yourself for a far more
>> sizable and thus impactful change.
>> 2. Your terse replies that need repeated queries to extract something
>> out of them. This was not for just a newbie like me, but even Nicolas
>> needed clarification.
>> 3. Your repeated refusal to acknowledge that the leak can happen, and
>> when someone (in this case me) spends effort demonstrating said leak,
>> your blunt dismissal of it via a selective reply.
>> 4. Your carefree attitude, falling to things like "it is all low
>> hanging fruit" and claiming that it was "wasted time". This is
>> insulting to the reviewer.
>
> I do not like reviewes from vertical space indent department.

It is very insulting to me to claim that I am from the "vertical space
indent department", and again this goes back to the unfriendliness.
Please refrain from this. I won't actually address this with explicit
links as proof, since it is obviously false from the commit history
which speaks for itself. The reviews also speak for themselves from
the ffmpeg-devel archive.

I personally don't mind at all to stop reviewing filters posted by
you: I only have a finite amount of energy to deal with this kind of
stuff, and I have zero interest in them personally. The reason I do it
is for the benefit of the project, and because I know that avfilter
often is lacking in this regard due to reduced manpower and user
interest here. Furthermore, on a personal note, it was where I
encountered a bug first with FFmpeg:
https://trac.ffmpeg.org/ticket/4547.

>
> I'm still looking for valid proof that leak happens in (almost) all of
> audio filters.

That is irrelevant to the actual comments I have made. It happens for
this one and other filters that you pushed recently despite me
pointing it out in review (at least 3 in number). It also happens for
the anoisesrc filter posted by Kyle - I may send him a private mail
some time. It likely happens for the others. I think the patch I gave
(the second one obviously) gives such valid proof across filters. But
again going back to the "finite energy" remark: maintainers should
test it out for filters they maintain.

>
> Yes, it was and still is wasted time, to find out that there is no
> leaking at all.

Thankfully Clement has started backing me up here, so you can keep
ranting about it all you want. Just please keep it off ffmpeg-devel,
it is now becoming ludicruous. You can't "make it not happen" by
simply asserting it over and over again expecting things to change.
This is the last time I will state this: proof lies in the patch I
gave (the second one obviously). Each filter can now be tested in a
matter of < 1 minute by applying the patch and running valgrind under
full leak checking.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel