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

[Paul B Mahol]

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.

Allocating something and than returning immediately error and then saying
look your code sucks is very unfriendly.