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

[wm4]

On Wed, 14 Mar 2018 16:02:36 -0300
James Almer <jamrial at gmail.com> wrote:

> On 3/14/2018 3:59 PM, wm4 wrote:
> > On Wed, 14 Mar 2018 14:45:33 -0300
> > James Almer <jamrial at gmail.com> wrote:
> >   
> >> On 3/14/2018 1:41 PM, wm4 wrote:  
> >>> 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).    
> >>
> >> My idea is to use it in place of av_fast_malloc() on code were an
> >> AVBufferRef would come in handy. Namely h2645_parse, so the escaped NALu
> >> buffers can be reused without the need of a bunch of memcpy.
> >> In such cases the references created may be gone by the time
> >> av_buffer_fast_alloc() is called again, so no new buffer would have to
> >> be allocated, but there's no warranty of that.
> >>
> >> A dynamic size buffer pool however has a lot more use cases across the
> >> codebase.
> >>  
> >>> 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.    
> >>
> >> That's probably easy to implement but i guess not too efficient, as you
> >> said. A better implementation would be a true dynamic size pool where
> >> each time you request a buffer the API looks at all those in the pool
> >> for one big enough and returns it if available, and if not, instead of
> >> allocating another buffer that will eventually make it to the pool, it
> >> replaces the until then biggest one.
> >> A function can also be added to resize all the buffers in the pool to
> >> make sure they are all within an arbitrary limit. Or simply a way to
> >> limit the pool right during init().  
> > 
> > Yeah, but that's called malloc(), heh. Or at least it's approximating
> > some sort of generic purpose memory allocator.  
> 
> I don't know what libc malloc() does, but that's not the case with other
> implementations.

Yeah, that was mostly a comment about the first part. If we can find
some simple way to handle pool allocation with dynamic sizes that
avoids the problems that can come with it, that would be nice.