| Message ID | 57b66b97-16be-64cf-bcc7-631e84e83797@gmail.com |
|---|---|
| State | Withdrawn |
| Headers | show |
On Wed, 14 Mar 2018 11:59:59 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/14/2018 11:35 AM, wm4 wrote: > > 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. > > 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.
On 3/14/2018 12:09 PM, wm4 wrote: > On Wed, 14 Mar 2018 11:59:59 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 3/14/2018 11:35 AM, wm4 wrote: >>> 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. >> >> 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.
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. _______________________________________________ ffmpeg-devel mailing list