| Message ID | 20180314153004.6436-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 14 Mar 2018 12:30:04 -0300 James Almer <jamrial@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@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.
On 3/14/2018 12:51 PM, wm4 wrote: > On Wed, 14 Mar 2018 12:30:04 -0300 > James Almer <jamrial@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@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".
On Wed, 14 Mar 2018 13:30:28 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/14/2018 12:51 PM, wm4 wrote: > > On Wed, 14 Mar 2018 12:30:04 -0300 > > James Almer <jamrial@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@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). 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. So if we expect such problems it might be better to rely on the libc's memory allocator - but then why use this fast_alloc stuff?
On 3/14/2018 1:41 PM, wm4 wrote: > On Wed, 14 Mar 2018 13:30:28 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 3/14/2018 12:51 PM, wm4 wrote: >>> On Wed, 14 Mar 2018 12:30:04 -0300 >>> James Almer <jamrial@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@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(). > So if we expect such > problems it might be better to rely on the libc's memory allocator - > but then why use this fast_alloc stuff? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, 14 Mar 2018 14:45:33 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/14/2018 1:41 PM, wm4 wrote: > > On Wed, 14 Mar 2018 13:30:28 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> On 3/14/2018 12:51 PM, wm4 wrote: > >>> On Wed, 14 Mar 2018 12:30:04 -0300 > >>> James Almer <jamrial@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@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.
On 3/14/2018 3:59 PM, wm4 wrote: > On Wed, 14 Mar 2018 14:45:33 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 3/14/2018 1:41 PM, wm4 wrote: >>> On Wed, 14 Mar 2018 13:30:28 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>> On 3/14/2018 12:51 PM, wm4 wrote: >>>>> On Wed, 14 Mar 2018 12:30:04 -0300 >>>>> James Almer <jamrial@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@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.
On Wed, 14 Mar 2018 16:02:36 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/14/2018 3:59 PM, wm4 wrote: > > On Wed, 14 Mar 2018 14:45:33 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> On 3/14/2018 1:41 PM, wm4 wrote: > >>> On Wed, 14 Mar 2018 13:30:28 -0300 > >>> James Almer <jamrial@gmail.com> wrote: > >>> > >>>> On 3/14/2018 12:51 PM, wm4 wrote: > >>>>> On Wed, 14 Mar 2018 12:30:04 -0300 > >>>>> James Almer <jamrial@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@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. Yeah, that was mostly a comment about the first part. If we can find some simple way to handle pool allocation with dynamic sizes that avoids the problems that can come with it, that would be nice.
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); + /** * @} */
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@gmail.com> --- TODO: Changelog and APIChanges entries, version bump. libavutil/buffer.c | 33 +++++++++++++++++++++++++++++++++ libavutil/buffer.h | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+)