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

[James Almer]

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.