[FFmpeg-devel] [PATCH] avcodec/internal: improve min_size documentation for ff_alloc_packet2()
Michael Niedermayer
michael at niedermayer.cc
Mon Aug 3 22:59:48 CEST 2015
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>
More information about the ffmpeg-devel
mailing list