[FFmpeg-devel] [PATCH v2] avutil/buffer: add av_buffer_fast_alloc()

[wm4]

On Wed, 14 Mar 2018 13:30:28 -0300
James Almer <jamrial at gmail.com> wrote:

> On 3/14/2018 12:51 PM, wm4 wrote:
> > On Wed, 14 Mar 2018 12:30:04 -0300
> > James Almer <jamrial at gmail.com> wrote:
> >   
> >> Same use case 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 <jamrial at gmail.com>
> >> ---
> >> TODO: Changelog and APIChanges entries, version bump.
> >>
> >>  libavutil/buffer.c | 33 +++++++++++++++++++++++++++++++++
> >>  libavutil/buffer.h | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 75 insertions(+)
> >>
> >> diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> >> index 8d1aa5fa84..26e015d7ee 100644
> >> --- a/libavutil/buffer.c
> >> +++ b/libavutil/buffer.c
> >> @@ -215,6 +215,39 @@ 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);
> >> +}
> >> +
> >>  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);
> >> +
> >>  /**
> >>   * @}
> >>   */  
> > 
> > LGTM
> > 
> > Also some comment on the side: I think what you'd actually want is some
> > sort of flexible buffer pool. E.g. consider the allocated AVBufferRef
> > gets used and buffered by the consumer for 1 frame. Then the
> > AVBufferRef will always be not-writable, and will always be newly
> > allocated, and this optimization via fast_alloc does not help. With a
> > buffer pool, it would allocate a second AVBufferRef, but then always
> > reuse it. Anyway, I might be overthinking it.  
> 
> No, that's a good idea, and it would come in handy in a lot more places
> than this fast_alloc API. But it's probably not trivial to implement,
> and would explain why the existing buffer pool API is for fixed sizes only.
> 
> In any case, do you think it's better to try and implement your
> suggestion instead of this patch? You're right that it's a matter of
> creating one extra reference and this function would probably stop being
> "fast".

Depends what's even the point of the new API (i.e. where it's used). A
pool should actually be relatively easy to implement: just resize the
pool if the new buffer exceeds the pool's size. But then it might waste
memory if all later elements are much smaller. So if we expect such
problems it might be better to rely on the libc's memory allocator -
but then why use this fast_alloc stuff?