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

[Ganesh Ajjanagadde]

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.

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