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 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. 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. Are there any concrete benchmarks which prove that this optimization is worth doing? 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. Also, doesn't it depend on the muxer which is the better choice? (Some muxers can always write the packet immediately, some need to buffer them for stream interleaving. I don't know much about muxers, but I think this is how it works. I suppose having a packet queue between encoder and muxer might also help to compensate for the effects of blocking I/O?) 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) - 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) Sorry for the rant, but I sure love it when things look suspicious on the surface, and when you dig deeper everything falls apart. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel