[FFmpeg-devel] [PATCH] avcodec/internal: improve min_size documentation for ff_alloc_packet2()
Michael Niedermayer
michael at niedermayer.cc
Tue Aug 4 00:51:17 CEST 2015
On Tue, Aug 04, 2015 at 12:12:24AM +0200, wm4 wrote:
> 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.
This is going into quite unproductive/useless talk i think but
i send a patch on the 6th june and waited 20 days for usefull
feedback and got none just a statement of dislike, i asked the people
who didnt like the patch on IRC and got on ok to IIRC add a flag or
something but not with much happyness. So i tried my best to take
into accout what people said and added the min_size field instead.
I pushed directly as there was no feedback in the previous 20 days
on the ML and noone seemed to care.
I do care about fixing a speed regression
>
> 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.
no, i disagree, theres no reason to live with something thats bad.
if this code has an issue, we should fix 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
>
> 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.
There was a bug report of a overall (aka time ffmpeg) speedloss
in the % values with high res rawvideo
>
> >
> > 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.
I want to fix the mess
and i would suggest that we first test if any / which encoders benefit
from the static buffer.
It where quite a few long ago but it seems from a few quick tests that
this is not so anymore.
Can someone confirm ?
if it has become useless then I could drop quite a bit of code
>
> >
> > >
> > > 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).
yes
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20150804/2f925410/attachment.sig>
More information about the ffmpeg-devel
mailing list