On Wed, 14 Mar 2018 11:13:52 -0300
James Almer <jamr...@gmail.com> wrote:
> On 3/14/2018 4:14 AM, wm4 wrote:
> > On Tue, 13 Mar 2018 20:48:56 -0300
> > James Almer <jamr...@gmail.com> wrote:
> >
> >> Same concept as av_fast_malloc(). If the buffer passed to it is writable
> >> and big enough it will be reused, otherwise a new one will be allocated
> >> instead.
> >>
> >> Signed-off-by: James Almer <jamr...@gmail.com>
> >> ---
> >> TODO: Changelog and APIChanges entries, version bump.
> >>
> >> libavutil/buffer.c | 63
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> libavutil/buffer.h | 42 ++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 105 insertions(+)
> >>
> >> diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> >> index 8d1aa5fa84..16ce1b82e2 100644
> >> --- a/libavutil/buffer.c
> >> +++ b/libavutil/buffer.c
> >> @@ -215,6 +215,69 @@ int av_buffer_realloc(AVBufferRef **pbuf, int size)
> >> return 0;
> >> }
> >>
> >> +static av_always_inline int buffer_fast_alloc(AVBufferRef **pbuf, int
> >> size, int zero_alloc)
> >> +{
> >> + AVBufferRef *buf = *pbuf;
> >> + AVBuffer *b;
> >> + uint8_t *data;
> >> +
> >> + if (!buf || !av_buffer_is_writable(buf) || buf->data !=
> >> buf->buffer->data) {
> >> + /* Buffer can't be reused, and neither can the entire AVBufferRef.
> >> + * Unref the latter and alloc a new one. */
> >> + av_buffer_unref(pbuf);
> >> +
> >> + buf = av_buffer_alloc(size);
> >> + if (!buf)
> >> + return AVERROR(ENOMEM);
> >> +
> >> + if (zero_alloc)
> >> + memset(buf->data, 0, size);
> >> +
> >> + *pbuf = buf;
> >> + return 0;
> >> + }
> >> + b = buf->buffer;
> >> +
> >> + if (size <= b->size) {
> >> + /* Buffer can be reused. Update the size of AVBufferRef but leave
> >> the
> >> + * AVBuffer untouched. */
> >> + buf->size = FFMAX(0, size);
> >> + return 0;
> >> + }
> >> +
> >> + /* Buffer can't be reused, but there's no need to alloc new AVBuffer
> >> and
> >> + * AVBufferRef structs. Free the existing buffer, allocate a new one,
> >> and
> >> + * reset AVBuffer and AVBufferRef to default values. */
> >> + b->free(b->opaque, b->data);
> >> + b->free = av_buffer_default_free;
> >> + b->opaque = NULL;
> >> + b->flags = 0;
> >> +
> >> + data = av_malloc(size);
> >> + if (!data) {
> >> + av_buffer_unref(pbuf);
> >> + return AVERROR(ENOMEM);
> >> + }
> >> +
> >> + if (zero_alloc)
> >> + memset(data, 0, size);
> >> +
> >> + b->data = buf->data = data;
> >> + b->size = buf->size = size;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +int av_buffer_fast_alloc(AVBufferRef **pbuf, int size)
> >> +{
> >> + return buffer_fast_alloc(pbuf, size, 0);
> >> +}
> >> +
> >> +int av_buffer_fast_allocz(AVBufferRef **pbuf, int size)
> >> +{
> >> + return buffer_fast_alloc(pbuf, size, 1);
> >> +}
> >> +
> >> 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 73b6bd0b14..1166017d22 100644
> >> --- a/libavutil/buffer.h
> >> +++ b/libavutil/buffer.h
> >> @@ -197,6 +197,48 @@ int av_buffer_make_writable(AVBufferRef **buf);
> >> */
> >> int av_buffer_realloc(AVBufferRef **buf, int size);
> >>
> >> +/**
> >> + * Allocate a buffer, reusing the given one if writable and large enough.
> >> + *
> >> + * @code{.c}
> >> + * AVBufferRef *buf = ...;
> >> + * int ret = av_buffer_fast_alloc(&buf, size);
> >> + * if (ret < 0) {
> >> + * // Allocation failed; buf already freed
> >> + * return ret;
> >> + * }
> >> + * @endcode
> >> + *
> >> + * @param buf A buffer reference. *buf may be NULL. On success, a new
> >> buffer
> >> + * reference will be written in its place. On failure, it
> >> will be
> >> + * unreferenced and set to NULL.
> >> + * @param size Required buffer size.
> >> + *
> >> + * @return 0 on success, a negative AVERROR on failure.
> >> + *
> >> + * @see av_buffer_realloc()
> >> + * @see av_buffer_fast_allocz()
> >> + */
> >> +int av_buffer_fast_alloc(AVBufferRef **buf, int size);
> >> +
> >> +/**
> >> + * Allocate and clear a buffer, reusing the given one if writable and
> >> large
> >> + * enough.
> >> + *
> >> + * Like av_buffer_fast_alloc(), but all newly allocated space is initially
> >> + * cleared. Reused buffer is not cleared.
> >> + *
> >> + * @param buf A buffer reference. *buf may be NULL. On success, a new
> >> buffer
> >> + * reference will be written in its place. On failure, it
> >> will be
> >> + * unreferenced and set to NULL.
> >> + * @param size Required buffer size.
> >> + *
> >> + * @return 0 on success, a negative AVERROR on failure.
> >> + *
> >> + * @see av_buffer_fast_alloc()
> >> + */
> >> +int av_buffer_fast_allocz(AVBufferRef **buf, int size);
> >> +
> >> /**
> >> * @}
> >> */
> >
> > Wouldn't it be better to avoid writing a lot of new allocation code
> > (with possibly subtle problems), and just use av_buffer_realloc() to
> > handle reallocation? (Possibly it could be moved to an internal
> > function to avoid the memcpy() required by realloc.)
>
> There are some differences that make most code in av_buffer_realloc()
> hard to be reused.
> In this one I'm using av_malloc instead of av_realloc, I update the
> AVBufferRef size with the requested size if it's equal or smaller than
> the available one and return doing nothing else instead of returning
> doing nothing at all only if the requested size is the exact same as the
> available one, I need to take precautions about what kind of buffer is
> passed to the function by properly freeing it using its callback then
> replacing it with default values, instead of knowing beforehand that the
> buffer was effectively created by av_buffer_realloc() itself where it
> can simply call av_realloc, etc.
>
> Trying to reuse code to follow all those details would make it harder to
> read and probably more prone to mistakes than just carefully writing the
> two separate functions doing the specific checks and allocations they
> require.
Fine, if you say so. I'd probably argue we should still try to share
some code, since it duplicates all the allocation _and_ deallocation
code, only for the additional check to skip doing anything if the
underlying buffer is big enough. Anyway, I'm not blocking the patch
over this, and I see no technical issues in the patch.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
