diff mbox series

[FFmpeg-devel,PoC,1/2] avutil/buffer: add av_buffer_create2() to allow AVBufferRef to store complex structures

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer June 1, 2020, 4:35 p.m. UTC
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(-)

Comments

Anton Khirnov June 1, 2020, 8:20 p.m. UTC | #1
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.
James Almer June 1, 2020, 8:55 p.m. UTC | #2
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.
Anton Khirnov June 2, 2020, 10:58 a.m. UTC | #3
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 mbox series

Patch

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
      */