[FFmpeg-devel] [PATCH] avutil/buffer: add av_buffer_fast_alloc()
James Almer
jamrial at gmail.com
Wed Mar 14 16:59:59 EET 2018
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.
More information about the ffmpeg-devel
mailing list