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

[James Almer]

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.

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

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

> + *         A negative error code on failure.
> + */
> +int av_buffer_replace(AVBufferRef **dst, AVBufferRef *src);
> +
>  /**
>   * @}
>   */
>