On Wed, 14 Mar 2018 11:59:59 -0300
James Almer <jamr...@gmail.com> wrote:
> On 3/14/2018 11:35 AM, wm4 wrote:
> > 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.
>
> I admittedly did a lot of ricing here trying to reuse the AVBufferRef
> and AVBuffer structs, so if you think it's kinda risky then we could
> always instead just do something simpler/safer and slightly slower only
> if a bigger buffer is needed, like
>
> ---------
> diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> index 8d1aa5fa84..31237d1f5c 100644
> --- a/libavutil/buffer.c
> +++ b/libavutil/buffer.c
> @@ -215,6 +215,38 @@ int av_buffer_realloc(AVBufferRef **pbuf, int size)
> return 0;
> }
>
> +static inline int buffer_fast_alloc(AVBufferRef **pbuf, int size,
> + AVBufferRef* (*buffer_alloc)(int size))
> +{
> + AVBufferRef *buf = *pbuf;
> +
> + if (buf && av_buffer_is_writable(buf) &&
> + buf->data == buf->buffer->data && size <= buf->buffer->size) {
> + buf->size = FFMAX(0, size);
> + return 0;
> + }
> +
> + av_buffer_unref(pbuf);
> +
> + buf = buffer_alloc(size);
> + if (!buf)
> + return AVERROR(ENOMEM);
> +
> + *pbuf = buf;
> +
> + return 0;
> +}
> +
> +int av_buffer_fast_alloc(AVBufferRef **pbuf, int size)
> +{
> + return buffer_fast_alloc(pbuf, size, av_buffer_alloc);
> +}
> +
> +int av_buffer_fast_allocz(AVBufferRef **pbuf, int size)
> +{
> + return buffer_fast_alloc(pbuf, size, av_buffer_allocz);
> +}
> ---------
>
> Both options are fine with me.
I like that much better. Smaller code size is a merit on its own.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
