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

[Reimar Döffinger]

On Sun, Feb 16, 2014 at 11:17:05PM +0100, wm4 wrote:
> 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.

The problem is the only thing in the documentation hinting at this is:
> every single data plane must be contained in one of the buffers

However a single buffer starting at address 0 and going all the way up
to the highest addressable address would fulfill this, which is
almost what my patch does.
If there are other assumptions this documentation needs to describe them
properly instead of it being magic that nobody can use except by
copy-pasting code from FFmpeg.