diff mbox

[FFmpeg-devel,RFC,WIP] avutil/buffer: add add a dynamnic size buffer pool API

Message ID 20180316182141.10156-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer March 16, 2018, 6:21 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
This is a proof of concept for a dynamic size buffer pool API.

For the purpose of easy testing and reviewing I replaced the current
linked list used to keep a pool of fixed size buffers with the tree
based pool that will be used to keep a pool of varying size buffers,
instead of adding a new set of functions exclusively for the new API.
The final committed work doesn't necessarely have to do the above, as
there's no real benefit using a tree when you only need a fixed size
buffer pool, other than simplying things.

I'm open to suggestions about how to introduce this. Completely
separate set of functions and struct names? Sharing the struct and
init/uninit functions and only adding a new get() one like in this
patch?
Any preferences with function/struct naming, for that matter?

 libavutil/buffer.c          | 98 ++++++++++++++++++++++++++++++++++++---------
 libavutil/buffer.h          |  2 +
 libavutil/buffer_internal.h |  6 ++-
 3 files changed, 85 insertions(+), 21 deletions(-)

Comments

Michael Niedermayer March 17, 2018, 11:48 p.m. UTC | #1
On Fri, Mar 16, 2018 at 03:21:41PM -0300, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This is a proof of concept for a dynamic size buffer pool API.
> 
> For the purpose of easy testing and reviewing I replaced the current
> linked list used to keep a pool of fixed size buffers with the tree
> based pool that will be used to keep a pool of varying size buffers,
> instead of adding a new set of functions exclusively for the new API.
> The final committed work doesn't necessarely have to do the above, as
> there's no real benefit using a tree when you only need a fixed size
> buffer pool, other than simplying things.
> 
> I'm open to suggestions about how to introduce this. Completely
> separate set of functions and struct names? Sharing the struct and
> init/uninit functions and only adding a new get() one like in this
> patch?
> Any preferences with function/struct naming, for that matter?
> 
>  libavutil/buffer.c          | 98 ++++++++++++++++++++++++++++++++++++---------
>  libavutil/buffer.h          |  2 +
>  libavutil/buffer_internal.h |  6 ++-
>  3 files changed, 85 insertions(+), 21 deletions(-)

not sure its not intended but this causes differences
in error concealment on many files
 an example would be something like this:

ffmpeg -threads 1 -i 1069/green-block-artifacts-from-canon-100-hs.MOV test.avi


[...]
James Almer March 18, 2018, 12:26 a.m. UTC | #2
On 3/17/2018 8:48 PM, Michael Niedermayer wrote:
> On Fri, Mar 16, 2018 at 03:21:41PM -0300, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> This is a proof of concept for a dynamic size buffer pool API.
>>
>> For the purpose of easy testing and reviewing I replaced the current
>> linked list used to keep a pool of fixed size buffers with the tree
>> based pool that will be used to keep a pool of varying size buffers,
>> instead of adding a new set of functions exclusively for the new API.
>> The final committed work doesn't necessarely have to do the above, as
>> there's no real benefit using a tree when you only need a fixed size
>> buffer pool, other than simplying things.
>>
>> I'm open to suggestions about how to introduce this. Completely
>> separate set of functions and struct names? Sharing the struct and
>> init/uninit functions and only adding a new get() one like in this
>> patch?
>> Any preferences with function/struct naming, for that matter?
>>
>>  libavutil/buffer.c          | 98 ++++++++++++++++++++++++++++++++++++---------
>>  libavutil/buffer.h          |  2 +
>>  libavutil/buffer_internal.h |  6 ++-
>>  3 files changed, 85 insertions(+), 21 deletions(-)
> 
> not sure its not intended but this causes differences
> in error concealment on many files

Not intended by me, at least. And not sure how using a tree instead of a
linked list to keep a pool of buffers could affect that. The way they
are allocated or returned is not changed, and all the existing users are
still only creating fixed sized buffers.

Do you have an idea of what could be happening? The way the tree is
being handled is afaik correct, judging by the doxy. Removing and adding
one element at a time using a simple cmp() function and with no way to
have races since it's all controlled by a mutex.

>  an example would be something like this:
> 
> ffmpeg -threads 1 -i 1069/green-block-artifacts-from-canon-100-hs.MOV test.avi
> 
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer March 18, 2018, 1:33 a.m. UTC | #3
On Sat, Mar 17, 2018 at 09:26:32PM -0300, James Almer wrote:
> On 3/17/2018 8:48 PM, Michael Niedermayer wrote:
> > On Fri, Mar 16, 2018 at 03:21:41PM -0300, James Almer wrote:
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >> This is a proof of concept for a dynamic size buffer pool API.
> >>
> >> For the purpose of easy testing and reviewing I replaced the current
> >> linked list used to keep a pool of fixed size buffers with the tree
> >> based pool that will be used to keep a pool of varying size buffers,
> >> instead of adding a new set of functions exclusively for the new API.
> >> The final committed work doesn't necessarely have to do the above, as
> >> there's no real benefit using a tree when you only need a fixed size
> >> buffer pool, other than simplying things.
> >>
> >> I'm open to suggestions about how to introduce this. Completely
> >> separate set of functions and struct names? Sharing the struct and
> >> init/uninit functions and only adding a new get() one like in this
> >> patch?
> >> Any preferences with function/struct naming, for that matter?
> >>
> >>  libavutil/buffer.c          | 98 ++++++++++++++++++++++++++++++++++++---------
> >>  libavutil/buffer.h          |  2 +
> >>  libavutil/buffer_internal.h |  6 ++-
> >>  3 files changed, 85 insertions(+), 21 deletions(-)
> > 
> > not sure its not intended but this causes differences
> > in error concealment on many files
> 
> Not intended by me, at least. And not sure how using a tree instead of a
> linked list to keep a pool of buffers could affect that. The way they
> are allocated or returned is not changed, and all the existing users are
> still only creating fixed sized buffers.
> 

> Do you have an idea of what could be happening? The way the tree is

is always the same buffer used as before ?
a different buffer with different content could probably leak the content
through into the output on some errors.


> being handled is afaik correct, judging by the doxy. Removing and adding
> one element at a time using a simple cmp() function and with no way to
> have races since it's all controlled by a mutex.
> 
> >  an example would be something like this:
> > 
> > ffmpeg -threads 1 -i 1069/green-block-artifacts-from-canon-100-hs.MOV test.avi
> > 
> > 
> > [...]
> > 
> > 
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
James Almer March 18, 2018, 1:46 a.m. UTC | #4
On 3/17/2018 10:33 PM, Michael Niedermayer wrote:
> On Sat, Mar 17, 2018 at 09:26:32PM -0300, James Almer wrote:
>> On 3/17/2018 8:48 PM, Michael Niedermayer wrote:
>>> On Fri, Mar 16, 2018 at 03:21:41PM -0300, James Almer wrote:
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> This is a proof of concept for a dynamic size buffer pool API.
>>>>
>>>> For the purpose of easy testing and reviewing I replaced the current
>>>> linked list used to keep a pool of fixed size buffers with the tree
>>>> based pool that will be used to keep a pool of varying size buffers,
>>>> instead of adding a new set of functions exclusively for the new API.
>>>> The final committed work doesn't necessarely have to do the above, as
>>>> there's no real benefit using a tree when you only need a fixed size
>>>> buffer pool, other than simplying things.
>>>>
>>>> I'm open to suggestions about how to introduce this. Completely
>>>> separate set of functions and struct names? Sharing the struct and
>>>> init/uninit functions and only adding a new get() one like in this
>>>> patch?
>>>> Any preferences with function/struct naming, for that matter?
>>>>
>>>>  libavutil/buffer.c          | 98 ++++++++++++++++++++++++++++++++++++---------
>>>>  libavutil/buffer.h          |  2 +
>>>>  libavutil/buffer_internal.h |  6 ++-
>>>>  3 files changed, 85 insertions(+), 21 deletions(-)
>>>
>>> not sure its not intended but this causes differences
>>> in error concealment on many files
>>
>> Not intended by me, at least. And not sure how using a tree instead of a
>> linked list to keep a pool of buffers could affect that. The way they
>> are allocated or returned is not changed, and all the existing users are
>> still only creating fixed sized buffers.
>>
> 
>> Do you have an idea of what could be happening? The way the tree is
> 
> is always the same buffer used as before ?

Was thinking that, yes. The pool organized by the tree most assuredly
orders the buffers in a different way than the linked list (Which i
think is simply FIFO), so the h264 decoder or whatever is handling the
pool here now gets a different buffer after calling av_buffer_pool_get().
This would mean the EC code is overreading bytes, and that the buffer is
not zeroed after being requested. The former should probably be fixed.

Fortunately not a bug from this patch/implementation, in that case.

> a different buffer with different content could probably leak the content
> through into the output on some errors.
> 
> 
>> being handled is afaik correct, judging by the doxy. Removing and adding
>> one element at a time using a simple cmp() function and with no way to
>> have races since it's all controlled by a mutex.
>>
>>>  an example would be something like this:
>>>
>>> ffmpeg -threads 1 -i 1069/green-block-artifacts-from-canon-100-hs.MOV test.avi
>>>
>>>
>>> [...]
>>>
>>>
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer March 18, 2018, 9:41 a.m. UTC | #5
On Sat, Mar 17, 2018 at 10:46:22PM -0300, James Almer wrote:
> On 3/17/2018 10:33 PM, Michael Niedermayer wrote:
> > On Sat, Mar 17, 2018 at 09:26:32PM -0300, James Almer wrote:
> >> On 3/17/2018 8:48 PM, Michael Niedermayer wrote:
> >>> On Fri, Mar 16, 2018 at 03:21:41PM -0300, James Almer wrote:
> >>>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>>> ---
> >>>> This is a proof of concept for a dynamic size buffer pool API.
> >>>>
> >>>> For the purpose of easy testing and reviewing I replaced the current
> >>>> linked list used to keep a pool of fixed size buffers with the tree
> >>>> based pool that will be used to keep a pool of varying size buffers,
> >>>> instead of adding a new set of functions exclusively for the new API.
> >>>> The final committed work doesn't necessarely have to do the above, as
> >>>> there's no real benefit using a tree when you only need a fixed size
> >>>> buffer pool, other than simplying things.
> >>>>
> >>>> I'm open to suggestions about how to introduce this. Completely
> >>>> separate set of functions and struct names? Sharing the struct and
> >>>> init/uninit functions and only adding a new get() one like in this
> >>>> patch?
> >>>> Any preferences with function/struct naming, for that matter?
> >>>>
> >>>>  libavutil/buffer.c          | 98 ++++++++++++++++++++++++++++++++++++---------
> >>>>  libavutil/buffer.h          |  2 +
> >>>>  libavutil/buffer_internal.h |  6 ++-
> >>>>  3 files changed, 85 insertions(+), 21 deletions(-)
> >>>
> >>> not sure its not intended but this causes differences
> >>> in error concealment on many files
> >>
> >> Not intended by me, at least. And not sure how using a tree instead of a
> >> linked list to keep a pool of buffers could affect that. The way they
> >> are allocated or returned is not changed, and all the existing users are
> >> still only creating fixed sized buffers.
> >>
> > 
> >> Do you have an idea of what could be happening? The way the tree is
> > 
> > is always the same buffer used as before ?
> 
> Was thinking that, yes. The pool organized by the tree most assuredly
> orders the buffers in a different way than the linked list (Which i
> think is simply FIFO), so the h264 decoder or whatever is handling the
> pool here now gets a different buffer after calling av_buffer_pool_get().
> This would mean the EC code is overreading bytes, and that the buffer is
> not zeroed after being requested. The former should probably be fixed.

the problem is probably that damaged video frames leave some areas 
uninitialized. Quite possibly a consequence of some corner cases not
being supported in the EC code.
This is a bug of course if its what happens and should be fixed.

[...]
James Almer March 20, 2018, 10:44 p.m. UTC | #6
On 3/16/2018 3:21 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This is a proof of concept for a dynamic size buffer pool API.
> 
> For the purpose of easy testing and reviewing I replaced the current
> linked list used to keep a pool of fixed size buffers with the tree
> based pool that will be used to keep a pool of varying size buffers,
> instead of adding a new set of functions exclusively for the new API.
> The final committed work doesn't necessarely have to do the above, as
> there's no real benefit using a tree when you only need a fixed size
> buffer pool, other than simplying things.
> 
> I'm open to suggestions about how to introduce this. Completely
> separate set of functions and struct names? Sharing the struct and
> init/uninit functions and only adding a new get() one like in this
> patch?
> Any preferences with function/struct naming, for that matter?

Ping?
James Almer March 27, 2018, 5:16 p.m. UTC | #7
On 3/20/2018 7:44 PM, James Almer wrote:
> On 3/16/2018 3:21 PM, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> This is a proof of concept for a dynamic size buffer pool API.
>>
>> For the purpose of easy testing and reviewing I replaced the current
>> linked list used to keep a pool of fixed size buffers with the tree
>> based pool that will be used to keep a pool of varying size buffers,
>> instead of adding a new set of functions exclusively for the new API.
>> The final committed work doesn't necessarely have to do the above, as
>> there's no real benefit using a tree when you only need a fixed size
>> buffer pool, other than simplying things.
>>
>> I'm open to suggestions about how to introduce this. Completely
>> separate set of functions and struct names? Sharing the struct and
>> init/uninit functions and only adding a new get() one like in this
>> patch?
>> Any preferences with function/struct naming, for that matter?
> 
> Ping?

No comments or preferences at all on how to introduce this?
wm4 March 27, 2018, 5:58 p.m. UTC | #8
On Tue, 27 Mar 2018 14:16:15 -0300
James Almer <jamrial@gmail.com> wrote:

> On 3/20/2018 7:44 PM, James Almer wrote:
> > On 3/16/2018 3:21 PM, James Almer wrote:  
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >> This is a proof of concept for a dynamic size buffer pool API.
> >>
> >> For the purpose of easy testing and reviewing I replaced the current
> >> linked list used to keep a pool of fixed size buffers with the tree
> >> based pool that will be used to keep a pool of varying size buffers,
> >> instead of adding a new set of functions exclusively for the new API.
> >> The final committed work doesn't necessarely have to do the above, as
> >> there's no real benefit using a tree when you only need a fixed size
> >> buffer pool, other than simplying things.
> >>
> >> I'm open to suggestions about how to introduce this. Completely
> >> separate set of functions and struct names? Sharing the struct and
> >> init/uninit functions and only adding a new get() one like in this
> >> patch?
> >> Any preferences with function/struct naming, for that matter?  
> > 
> > Ping?  
> 
> No comments or preferences at all on how to introduce this?

Well, it's a pretty big mess (so many things to consider). That's
mostly about the implementation details.

I think the API you picked is relatively nice. It feels weird that a
pool that is initialized with a size has a function to allocate buffers
with a different size. So if you want some bikeshedding, sure, I can
provide you with some colors:

 AVDynamicBufferPool *av_dynamic_buffer_pool_create(alloc_fn, free_fn);
 AVBufferRef *av_dynamic_buffer_pool_get(AVDynamicBufferPool *pool,
                                         size_t size);
 void av_dynamic_buffer_pool_uininit(AVDynamicBufferPool **pool);

or:

 // sets *pool if it was NULL
 // (where to put alloc/free FNs for custom alloc?)
 AVBufferRef *av_dynamic_buffer_pool_get(AVDynamicBufferPool **pool,
                                         size_t size);
 void av_dynamic_buffer_pool_uininit(AVDynamicBufferPool **pool);

or:

  // sets *pool if it was NULL
  // free the pool with av_buffer_unref()
  AVBufferRef *av_dynamic_buffer_pool_get(AVBufferRef **pool,
                                         size_t size);

or just force the API user to pass size=0 to av_buffer_pool_init() and
enforce the use of the correct alloc function (fixed size/dynamic size)
at runtime.
James Almer March 27, 2018, 10:50 p.m. UTC | #9
On 3/27/2018 2:58 PM, wm4 wrote:
> On Tue, 27 Mar 2018 14:16:15 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> On 3/20/2018 7:44 PM, James Almer wrote:
>>> On 3/16/2018 3:21 PM, James Almer wrote:  
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> This is a proof of concept for a dynamic size buffer pool API.
>>>>
>>>> For the purpose of easy testing and reviewing I replaced the current
>>>> linked list used to keep a pool of fixed size buffers with the tree
>>>> based pool that will be used to keep a pool of varying size buffers,
>>>> instead of adding a new set of functions exclusively for the new API.
>>>> The final committed work doesn't necessarely have to do the above, as
>>>> there's no real benefit using a tree when you only need a fixed size
>>>> buffer pool, other than simplying things.
>>>>
>>>> I'm open to suggestions about how to introduce this. Completely
>>>> separate set of functions and struct names? Sharing the struct and
>>>> init/uninit functions and only adding a new get() one like in this
>>>> patch?
>>>> Any preferences with function/struct naming, for that matter?  
>>>
>>> Ping?  
>>
>> No comments or preferences at all on how to introduce this?
> 
> Well, it's a pretty big mess (so many things to consider). That's
> mostly about the implementation details.
> 
> I think the API you picked is relatively nice. It feels weird that a
> pool that is initialized with a size has a function to allocate buffers
> with a different size.

I submitted it like this for easy review and testing. I wasn't really
expecting to push it without changes precisely because the init()
function is meant for fixed sized buffers.

> So if you want some bikeshedding, sure, I can
> provide you with some colors:
> 
>  AVDynamicBufferPool *av_dynamic_buffer_pool_create(alloc_fn, free_fn);
>  AVBufferRef *av_dynamic_buffer_pool_get(AVDynamicBufferPool *pool,
>                                          size_t size);
>  void av_dynamic_buffer_pool_uininit(AVDynamicBufferPool **pool);
> 
> or:
> 
>  // sets *pool if it was NULL
>  // (where to put alloc/free FNs for custom alloc?)
>  AVBufferRef *av_dynamic_buffer_pool_get(AVDynamicBufferPool **pool,
>                                          size_t size);
>  void av_dynamic_buffer_pool_uininit(AVDynamicBufferPool **pool);
> 
> or:
> 
>   // sets *pool if it was NULL
>   // free the pool with av_buffer_unref()
>   AVBufferRef *av_dynamic_buffer_pool_get(AVBufferRef **pool,
>                                          size_t size);

Ok so, separate set of struct and functions. Probably best in order to
avoid having to add all kinds of workarounds for things like calling dyn
functions using pools created with the fixed size init().

> 
> or just force the API user to pass size=0 to av_buffer_pool_init() and
> enforce the use of the correct alloc function (fixed size/dynamic size)
> at runtime.
diff mbox

Patch

diff --git a/libavutil/buffer.c b/libavutil/buffer.c
index 8d1aa5fa84..9fe5aa9825 100644
--- a/libavutil/buffer.c
+++ b/libavutil/buffer.c
@@ -251,19 +251,27 @@  AVBufferPool *av_buffer_pool_init(int size, AVBufferRef* (*alloc)(int size))
     return pool;
 }
 
+/* Callback function to free every pooled buffer during uninit,
+ * to be used with av_tree_enumerate(). */
+static int free_node(void *opaque, void *elem)
+{
+    BufferPoolEntry *buf = elem;
+
+    buf->free(buf->opaque, buf->data);
+    av_free(buf);
+
+    return 0;
+}
+
 /*
  * This function gets called when the pool has been uninited and
  * all the buffers returned to it.
  */
 static void buffer_pool_free(AVBufferPool *pool)
 {
-    while (pool->pool) {
-        BufferPoolEntry *buf = pool->pool;
-        pool->pool = buf->next;
+    av_tree_enumerate(pool->root, NULL, NULL, free_node);
+    av_tree_destroy(pool->root);
 
-        buf->free(buf->opaque, buf->data);
-        av_freep(&buf);
-    }
     ff_mutex_destroy(&pool->mutex);
 
     if (pool->pool_free)
@@ -285,17 +293,29 @@  void av_buffer_pool_uninit(AVBufferPool **ppool)
         buffer_pool_free(pool);
 }
 
+/* Callback function to compare two nodes, order them based
+ * on size, and retrieve them, to be used with av_tree_insert(). */
+static int cmp_insert(const void *key, const void *node)
+{
+    int ret = FFDIFFSIGN(((const BufferPoolEntry *) key)->size, ((const BufferPoolEntry *) node)->size);
+
+    if (!ret)
+        ret = FFDIFFSIGN(((const BufferPoolEntry *) key)->data, ((const BufferPoolEntry *) node)->data);
+    return ret;
+}
+
 static void pool_release_buffer(void *opaque, uint8_t *data)
 {
     BufferPoolEntry *buf = opaque;
     AVBufferPool *pool = buf->pool;
 
     if(CONFIG_MEMORY_POISONING)
-        memset(buf->data, FF_MEMORY_POISON, pool->size);
+        memset(buf->data, FF_MEMORY_POISON, buf->size);
 
     ff_mutex_lock(&pool->mutex);
-    buf->next = pool->pool;
-    pool->pool = buf;
+    /* Add the buffer into the pool, using the preallocated
+     * AVTreeNode stored in buf->node */
+    av_tree_insert(&pool->root, buf, cmp_insert, &buf->node);
     ff_mutex_unlock(&pool->mutex);
 
     if (atomic_fetch_add_explicit(&pool->refcount, -1, memory_order_acq_rel) == 1)
@@ -304,13 +324,13 @@  static void pool_release_buffer(void *opaque, uint8_t *data)
 
 /* allocate a new buffer and override its free() callback so that
  * it is returned to the pool on free */
-static AVBufferRef *pool_alloc_buffer(AVBufferPool *pool)
+static AVBufferRef *pool_alloc_buffer(AVBufferPool *pool, int size)
 {
     BufferPoolEntry *buf;
     AVBufferRef     *ret;
 
-    ret = pool->alloc2 ? pool->alloc2(pool->opaque, pool->size) :
-                         pool->alloc(pool->size);
+    ret = pool->alloc2 ? pool->alloc2(pool->opaque, size) :
+                         pool->alloc(size);
     if (!ret)
         return NULL;
 
@@ -320,9 +340,17 @@  static AVBufferRef *pool_alloc_buffer(AVBufferPool *pool)
         return NULL;
     }
 
+    buf->node = av_tree_node_alloc();
+    if (!buf->node) {
+        av_free(buf);
+        av_buffer_unref(&ret);
+        return NULL;
+    }
+
     buf->data   = ret->buffer->data;
     buf->opaque = ret->buffer->opaque;
     buf->free   = ret->buffer->free;
+    buf->size   = size;
     buf->pool   = pool;
 
     ret->buffer->opaque = buf;
@@ -331,22 +359,49 @@  static AVBufferRef *pool_alloc_buffer(AVBufferPool *pool)
     return ret;
 }
 
-AVBufferRef *av_buffer_pool_get(AVBufferPool *pool)
+/* Callback function to compare the requested size with the
+ * size of existing buffers in the pool, to be used with
+ * av_tree_find(). */
+static int cmp_int(const void *key, const void *node)
+{
+    return FFDIFFSIGN(*(const int *)key, ((const BufferPoolEntry *) node)->size);
+}
+
+AVBufferRef *av_buffer_pool_get_dyn(AVBufferPool *pool, int size)
 {
     AVBufferRef *ret;
-    BufferPoolEntry *buf;
+    BufferPoolEntry *buf, *next[2] = { NULL, NULL };
 
     ff_mutex_lock(&pool->mutex);
-    buf = pool->pool;
+    /* Find a big enough buffer in the pool. */
+    buf = av_tree_find(pool->root, &size, cmp_int, (void **)next);
+
+    if (!buf)
+        /* If none of the requested size exists, use a bigger one. */
+        buf = next[1];
+    if (!buf && (buf = next[0])) {
+        /* If the pool also doesn't have a bigger buffer, but does
+         * have a smaller one, then replace it with a new buffer of
+         * the requested size. */
+        av_tree_insert(&pool->root, buf, cmp_insert, &buf->node);
+        buf->free(buf->opaque, buf->data);
+        av_free(buf->node);
+        av_freep(&buf);
+    }
+
     if (buf) {
-        ret = av_buffer_create(buf->data, pool->size, pool_release_buffer,
+        ret = av_buffer_create(buf->data, buf->size, pool_release_buffer,
                                buf, 0);
         if (ret) {
-            pool->pool = buf->next;
-            buf->next = NULL;
+            /* Remove the buffer from the pool. Zero and store the
+             * AVTreeNode used for it in buf->node so we can use it
+             * again once the buffer is put back in the pool. */
+            av_tree_insert(&pool->root, buf, cmp_insert, &buf->node);
+            memset(buf->node, 0, av_tree_node_size);
+            ret->size = size;
         }
     } else {
-        ret = pool_alloc_buffer(pool);
+        ret = pool_alloc_buffer(pool, size);
     }
     ff_mutex_unlock(&pool->mutex);
 
@@ -355,3 +410,8 @@  AVBufferRef *av_buffer_pool_get(AVBufferPool *pool)
 
     return ret;
 }
+
+AVBufferRef *av_buffer_pool_get(AVBufferPool *pool)
+{
+    return av_buffer_pool_get_dyn(pool, pool->size);
+}
diff --git a/libavutil/buffer.h b/libavutil/buffer.h
index 73b6bd0b14..244a6de961 100644
--- a/libavutil/buffer.h
+++ b/libavutil/buffer.h
@@ -284,6 +284,8 @@  void av_buffer_pool_uninit(AVBufferPool **pool);
  */
 AVBufferRef *av_buffer_pool_get(AVBufferPool *pool);
 
+AVBufferRef *av_buffer_pool_get_dyn(AVBufferPool *pool, int size);
+
 /**
  * @}
  */
diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h
index 54b67047e5..6672d22516 100644
--- a/libavutil/buffer_internal.h
+++ b/libavutil/buffer_internal.h
@@ -24,6 +24,7 @@ 
 
 #include "buffer.h"
 #include "thread.h"
+#include "tree.h"
 
 /**
  * The buffer is always treated as read-only.
@@ -61,6 +62,7 @@  struct AVBuffer {
 
 typedef struct BufferPoolEntry {
     uint8_t *data;
+    int      size;
 
     /*
      * Backups of the original opaque/free of the AVBuffer corresponding to
@@ -70,12 +72,12 @@  typedef struct BufferPoolEntry {
     void (*free)(void *opaque, uint8_t *data);
 
     AVBufferPool *pool;
-    struct BufferPoolEntry *next;
+    struct AVTreeNode *node;
 } BufferPoolEntry;
 
 struct AVBufferPool {
     AVMutex mutex;
-    BufferPoolEntry *pool;
+    struct AVTreeNode *root;
 
     /*
      * This is used to track when the pool is to be freed.