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

[Michael Niedermayer]

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


> 
> 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

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


> 
> 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  ...


[...]

> 
> 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 ...


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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150803/dcb7f47c/attachment.sig>