| Message ID | 20200601163540.13179-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,PoC,1/2] avutil/buffer: add av_buffer_create2() to allow AVBufferRef to store complex structures | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Quoting James Almer (2020-06-01 18:35:39) > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This is an attempt at solving the annoying constrain of only supporting flat > arrays within any given AVBufferRef. It introduces a new function to create > buffers that accepts two new callback functions, one to allocate a new buffer > when a new writable reference needs to be created, and one to copy data to it. This makes me rather uneasy. Is this constraint really so limiting that we need to get rid of it? In my experience the limitation actually tends to be beneficial - it serves as a powerful hint that maybe you're doing things in a way you shouldn't. What use cases do you imagine this would allow that are not possible currently? The video enc params thing has been implemented as a flat array without that much ugliness. It is a bit more convoluted than it could be with this change, but then again making it a flat memcpyable array also has advantages. More generally, looking back at AVBuffer I have come to regret some design choices about it. It was a mistake to (ab)use it as a base for ALL refcounting everywhere. Instead, I should have added an AVRefcount for a thread safe counter+opaque+destructor and then used that as a base for anything that needed to be refcounted. The bottom line is that I believe AVBuffer should remain as close as possible to its original idea of "refcounted data buffer". If we want more complex refcounted structures, we should make them separate objects.
On 6/1/2020 5:20 PM, Anton Khirnov wrote: > Quoting James Almer (2020-06-01 18:35:39) >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This is an attempt at solving the annoying constrain of only supporting flat >> arrays within any given AVBufferRef. It introduces a new function to create >> buffers that accepts two new callback functions, one to allocate a new buffer >> when a new writable reference needs to be created, and one to copy data to it. > > This makes me rather uneasy. > Is this constraint really so limiting that we need to get rid of it? In > my experience the limitation actually tends to be beneficial - it > serves as a powerful hint that maybe you're doing things in a way you > shouldn't. > > What use cases do you imagine this would allow that are not possible > currently? Seeing the way the video enc params was designed and the side data types it replaces (qp table properties and qp table data as separate types) made me think we're constrained by the current AVBuffer API, and new API designs (especially those meant to be used as side data) being affected by it. Then there's some recent changes in CBS that Mark sent to the ML that in essence are an attempt to work around this limitation, and would hopefully be simplified by something like this. Also, i've noticed the way the buffer pools in some of the hwcontext implementations are used is very wtf, being both extremely fragile/hacky and depending on undocumented behavior. I didn't test, but i suspect that configuring with --enable-memory-poisoning will break a few of them, like d3d11, as they depend on the buffer returned by the pool to be in the exact same state as it was last time it was returned to the pool, which is not how the pools are meant to work. But i also don't think this patch here would help much in that regard. > The video enc params thing has been implemented as a flat > array without that much ugliness. It is a bit more convoluted than it > could be with this change, but then again making it a flat memcpyable > array also has advantages. > > More generally, looking back at AVBuffer I have come to regret some > design choices about it. It was a mistake to (ab)use it as a base for ALL > refcounting everywhere. Instead, I should have added an AVRefcount for a > thread safe counter+opaque+destructor and then used that as a base for > anything that needed to be refcounted. > The bottom line is that I believe AVBuffer should remain as close as > possible to its original idea of "refcounted data buffer". If we want > more complex refcounted structures, we should make them separate > objects. This could be a good alternative solution, yeah.
Quoting James Almer (2020-06-01 22:55:12) > Also, i've noticed the way the buffer pools in some of the hwcontext > implementations are used is very wtf, being both extremely fragile/hacky > and depending on undocumented behavior. I didn't test, but i suspect > that configuring with --enable-memory-poisoning will break a few of > them, like d3d11, as they depend on the buffer returned by the pool to > be in the exact same state as it was last time it was returned to the > pool, which is not how the pools are meant to work. But i also don't > think this patch here would help much in that regard. As I see it, that problem is rooted exactly in using AVBuffer as something it's not supposed to be. But changing that at this point would be very invasive.
diff --git a/libavutil/buffer.c b/libavutil/buffer.c index 7ff6adc2ec..b048e168e8 100644 --- a/libavutil/buffer.c +++ b/libavutil/buffer.c @@ -26,9 +26,12 @@ #include "mem.h" #include "thread.h" -AVBufferRef *av_buffer_create(uint8_t *data, int size, - void (*free)(void *opaque, uint8_t *data), - void *opaque, int flags) +AVBufferRef *av_buffer_create2(uint8_t *data, int size, + AVBufferRef* (*alloc)(void *opaque, int size), + int (*copy)(void *opaque, AVBufferRef *dst, + const uint8_t *src, int size), + void (*free)(void *opaque, uint8_t *data), + void *opaque, int flags) { AVBufferRef *ref = NULL; AVBuffer *buf = NULL; @@ -39,6 +42,8 @@ AVBufferRef *av_buffer_create(uint8_t *data, int size, buf->data = data; buf->size = size; + buf->alloc = alloc ? alloc : av_buffer_default_alloc; + buf->copy = copy ? copy : av_buffer_default_copy; buf->free = free ? free : av_buffer_default_free; buf->opaque = opaque; @@ -59,11 +64,29 @@ AVBufferRef *av_buffer_create(uint8_t *data, int size, return ref; } +AVBufferRef *av_buffer_create(uint8_t *data, int size, + void (*free)(void *opaque, uint8_t *data), + void *opaque, int flags) +{ + return av_buffer_create2(data, size, NULL, NULL, free, opaque, flags); +} + void av_buffer_default_free(void *opaque, uint8_t *data) { av_free(data); } +AVBufferRef *av_buffer_default_alloc(void *opaque, int size) +{ + return av_buffer_alloc(size); +} + +int av_buffer_default_copy(void *opaque, AVBufferRef *dst, const uint8_t *src, int size) +{ + memcpy(dst->data, src, size); + return 0; +} + AVBufferRef *av_buffer_alloc(int size) { AVBufferRef *ret = NULL; @@ -151,15 +174,21 @@ int av_buffer_get_ref_count(const AVBufferRef *buf) int av_buffer_make_writable(AVBufferRef **pbuf) { AVBufferRef *newbuf, *buf = *pbuf; + AVBuffer *b = buf->buffer; + int ret; if (av_buffer_is_writable(buf)) return 0; - newbuf = av_buffer_alloc(buf->size); + newbuf = b->alloc(b->opaque, buf->size); if (!newbuf) return AVERROR(ENOMEM); - memcpy(newbuf->data, buf->data, buf->size); + ret = b->copy(b->opaque, newbuf, buf->data, buf->size); + if (ret < 0) { + av_buffer_unref(&newbuf); + return ret; + } buffer_replace(pbuf, &newbuf); diff --git a/libavutil/buffer.h b/libavutil/buffer.h index e0f94314f4..375e04034a 100644 --- a/libavutil/buffer.h +++ b/libavutil/buffer.h @@ -131,6 +131,47 @@ AVBufferRef *av_buffer_create(uint8_t *data, int size, void (*free)(void *opaque, uint8_t *data), void *opaque, int flags); +/** + * Create an AVBuffer from an existing array. + * + * If this function is successful, data is owned by the AVBuffer. The caller may + * only access data through the returned AVBufferRef and references derived from + * it. + * If this function fails, data is left untouched. + * @param data data array + * @param size size of data in bytes + * @param alloc a callback for allocating a new buffer when a new writable + * reference for this buffer is created + * @param copy a callback for copying this buffer's data into the newly + * allocated buffer by the alloc callback + * @param free a callback for freeing this buffer's data + * @param opaque parameter to be got for processing or passed to alloc/copy/free + * @param flags a combination of AV_BUFFER_FLAG_* + * + * @return an AVBufferRef referring to data on success, NULL on failure. + */ +AVBufferRef *av_buffer_create2(uint8_t *data, int size, + AVBufferRef* (*alloc)(void *opaque, int size), + int (*copy)(void *opaque, AVBufferRef *dst, + const uint8_t *src, int size), + void (*free)(void *opaque, uint8_t *data), + void *opaque, int flags); + +/** + * Default alloc callback, which calls av_buffer_alloc() and returns the + * newly allocated buffer. + * This function is meant to be passed to av_buffer_create2() or + * av_buffer_pool_init2(), not called directly. + */ +AVBufferRef *av_buffer_default_alloc(void *opaque, int size); + +/** + * Default copy callback, which copies the data pointed by src to dst. + * This function is meant to be passed to av_buffer_create2(), not called + * directly. + */ +int av_buffer_default_copy(void *opaque, AVBufferRef *dst, const uint8_t *src, int size); + /** * Default free callback, which calls av_free() on the buffer data. * This function is meant to be passed to av_buffer_create(), not called diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h index 70d2615a06..27fcb5f015 100644 --- a/libavutil/buffer_internal.h +++ b/libavutil/buffer_internal.h @@ -39,6 +39,16 @@ struct AVBuffer { */ atomic_uint refcount; + /** + * a callback to allocate a new writable buffer + */ + AVBufferRef* (*alloc)(void *opaque, int size); + + /** + * a callback to copy the data into a newly allocated writable buffer + */ + int (*copy)(void *opaque, AVBufferRef *dst, const uint8_t *src, int size); + /** * a callback for freeing the data */
Signed-off-by: James Almer <jamrial@gmail.com> --- This is an attempt at solving the annoying constrain of only supporting flat arrays within any given AVBufferRef. It introduces a new function to create buffers that accepts two new callback functions, one to allocate a new buffer when a new writable reference needs to be created, and one to copy data to it. In the default scenario, the alloc and copy callbacks simply call av_buffer_alloc() and memcpy() respectively, which is the current behavior of treating the buffer as a flat array. In a more complex scenario, the real benefit comes from the copy callback, which will let a custom implementation set up the new buffer how it pleases, including handling pointers within the complex struct it may be storing. Patch 2/2 is an example implementation of this. libavutil/buffer.c | 39 ++++++++++++++++++++++++++++++----- libavutil/buffer.h | 41 +++++++++++++++++++++++++++++++++++++ libavutil/buffer_internal.h | 10 +++++++++ 3 files changed, 85 insertions(+), 5 deletions(-)