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

[James Almer]

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 at 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.