On Sat, Dec 5, 2015 at 1:20 AM, Paul B Mahol <one...@gmail.com> wrote: > On 12/4/15, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >> On Fri, Dec 4, 2015 at 3:49 PM, Paul B Mahol <one...@gmail.com> wrote: >>> On 12/4/15, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >>>> On Fri, Dec 4, 2015 at 11:36 AM, Paul B Mahol <one...@gmail.com> wrote: >>>>> On 12/4/15, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >>>>>> On Wed, Dec 2, 2015 at 1:42 AM, Paul B Mahol <one...@gmail.com> wrote: >>>>>>> On 12/2/15, Paul B Mahol <one...@gmail.com> wrote: >>>>>>>> On 12/2/15, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: >>>>>>>>> On Tue, Dec 1, 2015 at 2:14 PM, Paul B Mahol <one...@gmail.com> >>>>>>>>> wrote: >>>>>>>>>> On 12/1/15, Nicolas George <geo...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
