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

[wm4]

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