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

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(+)

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);
> +
>  /**
>   * @}
>   */
>

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.

On 6/8/2020 8:34 AM, Anton Khirnov wrote:
> 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.

Just a sanity check, in case for some reason the AVBufferRef struct was
copied instead of created using av_buffer_ref(). If src is writable when
it's also equal to dst then it means something wrong was done by either
the caller or some other helper. But It's not important, hence a nit.

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

Then just check if dst is NULL and do a simple *pdst =
av_buffer_ref(src) in that case.

The idea is to keep pdst untouched on failure, which is in line with the
behavior of almost every other buffer handling public function. If you'd
rather not use buffer_replace() to achieve that then that's fine. It's
simply an useful helper made for this very purpose that you could use
here. It can even free dst when src is NULL, letting you remove the
first check you added in the function.

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