[vlc-devel] [PATCH 2/7] picture_pool: add the video format requested when waiting for a picture

[Steve Lhomme]

On 2020-11-06 18:00, Rémi Denis-Courmont wrote:
> Le perjantaina 6. marraskuuta 2020, 16.42.09 EET Steve Lhomme a écrit :
>> In push mode it's convenient to keep the same pool of buffers when only
>> small changes to the video format occur.
> 
> This makes no sense to me. In push mode, the allocator is also the format
> controller. It is free to switch pool if it needs to, or if it is simpler, or
> to keep the same pool.

Yes, except 99% of the code doesn't do that (nvdec being a rare 
exception). The pool is invisible to the decoder, it is handled in 
decoder_helpers.c.

The good news is that it makes easier to change this for 99% of the usage.

> The need to reallocate buffers is anyway in general dependent on the specific
> allocator/decoder, so I don't even follow how passing the format in generic
> functions helps at all.
> 
> I also cannot relate this to any plan that was agreed upon in the past.

The generic/clean way is to create a new pool. But it is creating a lot 
of stress on the memory. Imagine decoding 4K HEVC on 32 cores. You have 
a huge pool. And you would allocate such a pool every time the video 
format changes. That may happen often with HDR metadata (in HDR10+ I 
suppose that would be the case). Same thing with interlacing in push. 
Then at some point you could have 4/10/whatever the same huge pool 
allocated in memory.

The picture pools exist for 2 reasons:
- preallocate the buffers so we don't run out of memory midstream
- pace the decoder so it doesn't allocate an infinite amount of memory

None of this is related to the video_format_t. It has everything to do 
with the allocated buffers (decoder width/height and planes for the 
chroma). The rest of the video_format_t is just attached to the buffers 
when we use them. The fact what is in the video_format_t changes is 
useless to the pool.

IMO the picture pool should only deal with the buffer pooling and not 
the video_format_t attached to it. This patch is one step in that 
direction. I can do the rest if needed.

That's also what the nvdec_pool_t and the va_pool_t do. They deal with 
surface pooling and don't care about the video_format_t. I think we 
should unify all this.