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

[James Almer]

On 3/27/2018 2:58 PM, wm4 wrote:
> On Tue, 27 Mar 2018 14:16:15 -0300
> James Almer <jamrial at 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 at 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.