[FFmpeg-devel] [PATCH 1/4] lavu/buffer: add a convenience function for replacing buffers

[Anton Khirnov]

Quoting James Almer (2020-06-05 15:05:33)
> On 6/5/2020 7:02 AM, Anton Khirnov wrote:
> > A common pattern e.g. in libavcodec is replacing/updating buffer
> > references: unref old one, ref new one. This function allows simplifying
> > such code an avoiding unnecessary refs+unrefs if the references are
> > already equivalent.
> > ---
> >  doc/APIchanges     |  3 +++
> >  libavutil/buffer.c | 22 ++++++++++++++++++++++
> >  libavutil/buffer.h | 17 +++++++++++++++++
> >  3 files changed, 42 insertions(+)
> > 
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index fb5534b5f5..757d814eee 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >  
> >  API changes, most recent first:
> >  
> > +2020-xx-xx - xxxxxxxxxx - lavc 56.50.100 - buffer.h
> > +  Add a av_buffer_replace() convenience function.
> > +
> >  2020-xx-xx - xxxxxxxxxx - lavc 58.88.100 - avcodec.h codec.h
> >    Move AVCodec-related public API to new header codec.h.
> >  
> > diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> > index 6d9cb7428e..ecd83da9c3 100644
> > --- a/libavutil/buffer.c
> > +++ b/libavutil/buffer.c
> > @@ -216,6 +216,28 @@ int av_buffer_realloc(AVBufferRef **pbuf, int size)
> >      return 0;
> >  }
> >  
> > +int av_buffer_replace(AVBufferRef **pdst, AVBufferRef *src)
> > +{
> > +    AVBufferRef *dst = *pdst;
> > +
> > +    if (!src) {
> > +        av_buffer_unref(pdst);
> > +        return !!dst;
> > +    }
> > +
> > +    if (dst && dst->buffer == src->buffer) {
> > +        /* make sure the data pointers match */
> 
> Maybe overkill, but if dst->buffer == src->buffer then you could also
> add an assert to ensure that src->buffer is not writable.

Why? That should always be true, since there are two references, but I
don't see how is that related to what the function is doing.

> 
> > +        dst->data = src->data;
> > +        dst->size = src->size;
> > +        return 0;
> > +    }
> > +
> 
> > +    av_buffer_unref(pdst);
> > +    *pdst = av_buffer_ref(src);
> > +
> > +    return *pdst ? 1 : AVERROR(ENOMEM);
> > +}
> 
> Nit: How about instead something like
> 
> newbuf = av_buffer_ref(src);
> if (!newbuf)
>     return AVERROR(ENOMEM);
> 
> buffer_replace(pdst, &newbuf);
> 
> return 0;
> 
> It's IMO cleaner looking, and prevents pdst from being modified on failure.

That requires pdst to be an existing buffer, which is not guranteed.
Also, I don't see the logic behind buffer_replace(), so it does not seem
simpler.

> 
> > +
> >  AVBufferPool *av_buffer_pool_init2(int size, void *opaque,
> >                                     AVBufferRef* (*alloc)(void *opaque, int size),
> >                                     void (*pool_free)(void *opaque))
> > diff --git a/libavutil/buffer.h b/libavutil/buffer.h
> > index e0f94314f4..497dc98c20 100644
> > --- a/libavutil/buffer.h
> > +++ b/libavutil/buffer.h
> > @@ -197,6 +197,23 @@ int av_buffer_make_writable(AVBufferRef **buf);
> >   */
> >  int av_buffer_realloc(AVBufferRef **buf, int size);
> >  
> > +/**
> > + * Ensure dst refers to the same data as src.
> > + *
> > + * When *dst is already equivalent to src, do nothing. Otherwise unreference dst
> > + * and replace it with a new reference to src.
> > + *
> > + * @param dst Pointer to either a valid buffer reference or NULL. On success,
> > + *            this will point to a buffer reference equivalent to src. On
> > + *            failure, dst will be unreferenced.
> > + * @param src A buffer reference to replace dst with. May be NULL, then this
> > + *            function is equivalent to av_buffer_unref(dst).
> > + * @return 0 if dst was equivalent to src on input and nothing was done
> > + *         1 if dst was replaced with a new reference to src
> 
> Either of these values mean success, and the user will not really care
> if the function was a no-op or a ref (the next three patches are an
> example of this). In the end, dst is guaranteed to point to the same
> underlying buffer as src as long as the return value is not negative,
> regardless of what the function did internally.
> 
> This is already the case for av_buffer_make_writable(), where we don't
> discriminate between a no-op and a reallocation either, and in other
> similar functions (packet, frame, etc), so I'd strongly prefer if we can
> keep this consistent.

Ok, changed locally.

-- 
Anton Khirnov