[FFmpeg-devel] [PATCH] avcodec/internal: improve min_size documentation for ff_alloc_packet2()

[wm4]

On Mon, 3 Aug 2015 22:59:48 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Mon, Aug 03, 2015 at 10:05:23PM +0200, wm4 wrote:
> > On Mon,  3 Aug 2015 19:17:16 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > From: Michael Niedermayer <michael at niedermayer.cc>
> > > 
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/internal.h |    9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > > index 202f82d..17c86a3 100644
> > > --- a/libavcodec/internal.h
> > > +++ b/libavcodec/internal.h
> > > @@ -217,9 +217,14 @@ int avpriv_unlock_avformat(void);
> > >   *                avpkt->size is set to the specified size.
> > >   *                All other AVPacket fields will be reset with av_init_packet().
> > >   * @param size    the minimum required packet size
> > > - * @param min_size the smallest the packet might be down sized to, can be set to
> > > + * @param min_size This is a hint to the allocation algorithm, which indicates
> > > + *                to what minimal size the caller might later shrink the packet
> > > + *                to. Encoders often allocate packets which are larger than the
> > > + *                amount of data that is written into them as the exact amount is
> > > + *                not known at the time of allocation. min_size represents the
> > > + *                size a packet might be schrunk to by the caller. Can be set to
> > 
> > shrunk
> > 
> > >   *                0, setting this roughly correctly allows the allocation code
> > 
> > Using . instead of , sounds better.
> > 
> > > - *                to choose between several allocation stragies to improve
> > > + *                to choose between several allocation strategies to improve
> > >   *                speed slightly.
> > >   * @return        non negative on success, negative error code on failure
> > >   */
> > 
> > Better, but I still don't understand why this function exists. So the
> 
> ok, applied with these changes, to get it docuemnted at least. Will
> try to resolve all other issues too
> 
> 
> > parameter is a heuristic which determines whether to use an internal
> > semi-static internal buffer? But what if the resulting AVPacket gets put
> > into a packet queue? Then the packet needs to be copied anyway, because
> > refcounting doesn't work with this internal buffer.
> 
> IIRC i always assumed that a extra copy would be needed with the
> static buffer and that still was faster in testing IIRC
> 
> 
> > 
> > I see this line of code, which is also the only use of min_size in the
> > implementation of ff_alloc_packet2:
> > 
> >     if (avctx && 2*min_size < size) { // FIXME The factor needs to be finetuned
> > 
> > Why does it need to be fine-tuned? The comment doesn't really imply the
> > author of this code had much faith in the heuristic.
> 
> absolutely, yes
> I tried to implement the difference by using ff_alloc_packet2()
> and ff_alloc_packet() this was rejected
> i implemented it by using ff_alloc_packet2() and the context, this
> too was rejected, it was suggested to use a flag, this too was not
> popular so i added the min_size field.
> i didnt optimize the heuristic because i wanted to wait for the
> community first to accept the system before i finetuned it

I only remember a recent patch, where I wondered why you changed a call
from ff_alloc_packet2() to ff_alloc_packet(). I complained that the
function with "2" implies that it's a replacement for the "old"
function, and that changing the code to use an "old" function made no
sense.

So yes, these functions had terrible, terrible naming.

Now the thing is, you just pushed the modified ff_alloc_packet2()
directly to git. I can't see any patch for this on the mailing list. So
it looks like you didn't ask the community.

Now, maybe this single function isn't all so important, and we can just
live with the min_size parameter. But still, it shows that reviews are
important before such questionable things spread all over the codebase.

> 
> > 
> > Are there any concrete benchmarks which prove that this optimization
> > is worth doing?
> 
> yes, i benchmarked it when i wrote it, there even was a bug report
> due to the speedloss from having the wrong variant being used and the
> resulting speed loss.
> As well as benchmark results in some commits like
> 4302a92835313000d4bf8030baae32c2b130d8a1

    avcodec/pcm: Better min_size for ff_alloc_packet2()
    
    33318 -> 30601 decicycles

That doesn't look convincing at all. So you save ~3000 decicycles for
a task that isn't CPU bound, but which might blocks entire milliseconds
on I/O.

> 
> i do intend to fine tune this for more codecs but i spend more time
> in such disussions than working on the code and having people tell me
> to do it differently each time i take a step kind of slows it down
> and turns it into a mess

We also must take care of not turning the codebase into a mess.

> 
> > 
> > Actually looking at the codebase, only encoders use it. Most encoders
> > use 0 for min_size, which makes them always use the internal buffer.
> > Some recent changes of switching encoders to ff_alloc_packet2() use
> > min_size==size, always disabling the use of the internal buffer.
> > 
> > I haven't found a single case where min_size is something other than 0
> > or size! Maybe I overlooked some other cases, but they can't be many.
> > 
> > So effectively, does this only switch between using internal buffer,
> > and an internal buffer? Why not make them separate functions? It would
> > also make the code more readable.
> 
> Thats what i did initially :)
> (ff_alloc_packet() and ff_alloc_packet2()
> but it was rejected
> i also asked what API to use  ...

No, ff_alloc_packet2() implies that it's a strict replacement of
ff_alloc_packet(). If these functions just do different things, their
names shouldn't just be different in their "version number" (because
that's how such trailing numbers are used in this codebase).

> 
> 
> [...]
> 
> > 
> > So there are several things wrong with this:
> > - the heuristic has a comment that it needs work
> > - but effectively there's no heuristic; there are only 2 possible cases
> 
> > - it's not even clear when which case is preferable, and it doesn't
> >   seem to depend on packet sizes (but this is what the function
> >   implies; what else would the function of min_size be)
> 
> the time taken to allocate, resize or copy a block of memory does
> depend on the size of the block
> 
> 
> > - we'll probably see a flood of commits changing random encoders to
> >   this new function, for no reason (I'm looking forward to be proven
> >   wrong)
> 
> almost all encoders already use the new function,
> i intended to go over them and correct the min_size field as this
> results in some quite significant speed gain for them.
> as well as more extensively testing things
> 
> i dont think you oppose me making them faster ...

Not if it's significant.