[FFmpeg-devel] [PATCH] libavcodec/utils: Simplify get_buffer compatibility wrapper.

[wm4]

On Sun, 16 Feb 2014 20:46:40 +0100
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> The documentation states that there is no need to have one
> ref buffer per individual pointer, so I why all the extra code
> should be needed at least for video.
> Audio is untouched since I am not sure about it and I have not
> test-case for it.
> 
> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> ---
>  libavcodec/utils.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index ea77654..9a4ebbe 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -912,24 +912,7 @@ do {                                                                    \
>  } while (0)
>  
>          if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> -            const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
> -
> -            planes = av_pix_fmt_count_planes(frame->format);
> -            /* workaround for AVHWAccel plane count of 0, buf[0] is used as
> -               check for allocated buffers: make libavcodec happy */
> -            if (desc && desc->flags & AV_PIX_FMT_FLAG_HWACCEL)
> -                planes = 1;
> -            if (!desc || planes <= 0) {
> -                ret = AVERROR(EINVAL);
> -                goto fail;
> -            }
> -
> -            for (i = 0; i < planes; i++) {
> -                int v_shift    = (i == 1 || i == 2) ? desc->log2_chroma_h : 0;
> -                int plane_size = (frame->height >> v_shift) * frame->linesize[i];
> -
> -                WRAP_PLANE(frame->buf[i], frame->data[i], plane_size);
> -            }
> +            frame->buf[0] = dummy_buf;
>          } else {
>              int planar = av_sample_fmt_is_planar(frame->format);
>              planes = planar ? avctx->channels : 1;
> @@ -951,9 +934,9 @@ do {                                                                    \
>                  WRAP_PLANE(frame->extended_buf[i],
>                             frame->extended_data[i + FF_ARRAY_ELEMS(frame->buf)],
>                             frame->linesize[0]);
> -        }
>  
> -        av_buffer_unref(&dummy_buf);
> +            av_buffer_unref(&dummy_buf);
> +        }
>  
>  end:
>          frame->width  = avctx->width;

The assumption of the AVFrame API is that all memory ranges referenced
are covered by buffer refs. You seem to think that it's ok to have
merely an abstract reference for the entire frame, but apparently this
is not enough. The code you remove is an attempt to account for all
data correctly. Since you don't know whether all the planes are
allocated with a single memory allocation or whether they're separate
allocations, you have to assume the worst case, and use a buffer per
plane.

I'm not 100% sure though. I once walked into this trap too, but I forgot
why this is needed.