On Mon, Aug 03, 2015 at 10:05:23PM +0200, wm4 wrote: > On Mon, 3 Aug 2015 19:17:16 +0200 > Michael Niedermayer <michae...@gmx.at> wrote: > > > From: Michael Niedermayer <mich...@niedermayer.cc> > > > > Signed-off-by: Michael Niedermayer <mich...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
