[Ffmpeg-devel] [PATCH 8/9] Doxygenize av_fast_realloc comments

[Michael Niedermayer]

Hi

On Fri, Feb 23, 2007 at 01:55:57PM +0100, Panagiotis Issaris wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi,
> 
> Michael Niedermayer schreef:
> > On Thu, Feb 22, 2007 at 10:32:31PM +0100, Panagiotis Issaris wrote:
> >> Panagiotis Issaris schreef:
> >>> Michael Niedermayer schreef:
> >>>> On Thu, Feb 22, 2007 at 08:47:58PM +0100, Panagiotis Issaris wrote:
> >> [...]
> >>>>>> additionally, this function is a internal function of avcodec
> >>>>>> and should not be used outside, it shouldnt have a av_ prefix  but ff_
> >>>>>> and it should be droped if normal av_realloc() is as fast (benchmark a
> >>>>>> large number of reallocs)
> >>>>>>
> >>>>>>      
> >>>>> I?ll first prepare a patch for moving it and doing the s/av/ff/ 
> >>>>> thing. Afterwards I'll post
> >>>>> some benchmarking code so that others correct it and run the 
> >>>>> benchmark too.
> >>>>>    
> >>>> hmm wouldnt it be better to benchmark it first before doing a possibly
> >>>> redundant move/rename patch?
> >>>>  
> >>> Yes, but I thought it might take a while till consensus is reached on 
> >>> whether it is faster or not, and
> >>> that it might also take awhile before it gets benchmarked on enough 
> >>> different architectures/CPUs.
> >> A first attempt of some benchmarking code for av_fast_realloc() 
> >> hideously embedded within FFmpeg's main function.
> >>
> >> I do not know if I'm benchmarking the correct type of usage of 
> >> av_fast_realloc() vs av_realloc() nor if I am using
> >> START_TIMER|STOP_TIMER in the way you had intended it...
> >>
> >> For each test of av_fast_realloc() I tested two runs, one with 
> >> size<min_size and one with size>min_size, to test both
> >> cases. (Besides the obvious av_realloc() run ofcourse.)
> >>
> >> _Obviously_ not intended for Subversion.
> > 
> > you change the internal size variable for av_fast_realloc() so the result
> > is meaningless at least as far as i can see, the size variable is never
> > ever chnanged by anything except av_fast_realloc() itself
> 
> Ah! You are right, I didn't really know what the min_size parameter was
> about, which is also why I didn't enhance the doxygen documentation.
> 
> If I understand it correctly now, the idea is that you do an:
> av_fast_realloc(ptr, &size, req_size);
> Where the first time both size and req_size have the same value.

first time size is 0


> av_fast_realloc() increases the value of size a bit to get better
> performance (I guess). If the requested size in subsequent calls to
> av_fast_realloc() is smaller than the previous requested sizes, it will
> just return the pointer and not reallocate it (not realloc a smaller
> buffer). So, you might "waste" some memory, right?

yes, the realloc to smaller generally doesnt happen in the code AFAIK


> 
> int size = 40000;
> int req_size = 40000;
> ptr = av_fast_realloc(ptr, &size, req_size);
> 
> And the following code will be faster because no reallocation occurs:
> req_size = 10;
> ptr = av_fast_realloc(ptr, &size, req_size);
> 
> And the code below should be a lot faster with av_fast_realloc()
> then with a plain av_realloc() because no reallocation occurs
> because size is still > 40000, right?

yes


> req_size = 40000;
> ptr = av_fast_realloc(ptr, &size, req_size);
> 
> *size= FFMAX(17*min_size/16 + 32, min_size);
> Why is the maximum of both taken? Isn't the first part always bigger
> than the second?

you forgetting the possibility of overflows

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070223/86b1d477/attachment.pgp>