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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
