[FFmpeg-devel] [PATCH] avutil/buffer: add av_buffer_fast_alloc()
James Almer
jamrial at gmail.com
Wed Mar 14 17:31:39 EET 2018
On 3/14/2018 12:09 PM, wm4 wrote:
> On Wed, 14 Mar 2018 11:59:59 -0300
> James Almer <jamrial at gmail.com> wrote:
>
>> On 3/14/2018 11:35 AM, wm4 wrote:
>>> On Wed, 14 Mar 2018 11:13:52 -0300
>>> James Almer <jamrial at gmail.com> wrote:
>>>
>>>> On 3/14/2018 4:14 AM, wm4 wrote:
>>>>> On Tue, 13 Mar 2018 20:48:56 -0300
>>>>> James Almer <jamrial at 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 <jamrial at 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.
Just sent a new patch using that approach, then.
More information about the ffmpeg-devel
mailing list