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

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@gmail.com>
---
TODO: Changelog and APIChanges entries, version bump.

 libavutil/buffer.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libavutil/buffer.h | 42 ++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

On Tue, 13 Mar 2018 20:48:56 -0300
James Almer <jamrial@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@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.)

On 3/14/2018 4:14 AM, wm4 wrote:
> On Tue, 13 Mar 2018 20:48:56 -0300
> James Almer <jamrial@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@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.

On Wed, 14 Mar 2018 11:13:52 -0300
James Almer <jamrial@gmail.com> wrote:

> On 3/14/2018 4:14 AM, wm4 wrote:
> > On Tue, 13 Mar 2018 20:48:56 -0300
> > James Almer <jamrial@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@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.