[vlc-devel] [PATCH 0/5] Introducing dynamic pools

[Rémi Denis-Courmont]

Le keskiviikkona 5. joulukuuta 2018, 19.05.57 EET Steve Lhomme a écrit :
> On 05/12/2018 17:15, Rémi Denis-Courmont wrote:
> > Le keskiviikkona 5. joulukuuta 2018, 10.59.01 EET Steve Lhomme a écrit :
> >> As discussed during the vout workshop each decoder/filter/converter will
> >> need to have its own output pool (sharing them may be a future
> >> optimization).
> > 
> > No. They will need to allocate pictures. They may use a picture_pool_t, or
> > a custom pool implementation, or anything else that fits.
> 
> And where is that pool coming from if not from the
> decoder/filter/converter ?

It's coming from nowhere. In buffer-push model, pools are not necessary and 
are not exposed by any interface. Indeed, we don't use pools for demux packets 
(which were always push-buffers) nor for audio buffers (which have been push-
buffers for quite a few years).

A decoder/filter/converter may still elect to use a fixed-size picture pool 
internally to ensure that resources are available (hardware) or perform 
congestion control and enfroce a safe limit on memory consumption (software). 
But that is an implementation detail.

> It may not have the same output format as the
> next in line so it cannot share the pool easily.

Obviously a format-changing filter or converter cannot use input picture 
buffers and thus needs to allocate output picture buffers. No changes there.

> > For software decoding, allocating too many pictures is a non-issue since
> > demand paging will leave the higher numbered pictures phyiscally
> > unallocated (that is why we always take the first free picture, instead
> > of rotating the pool like in the old days). Instead, the pool is there to
> > constrain memory
> It may be a feature but it makes things trickier to debug. I've been
> beaten many times.

There are two options. Either you limit the pictures count, and you will have 
hard-to-debug deadlocks or allocation failures. Or you don't limit it, and you 
will leak memory and likely crash the whole system under pressure. The latter 
option is probably even harder to debug.

Furthermore, the limit provides congestion control for free. If you don't 
limit allocations, then you will need to implement congestion control 
manually, and that's not exactly trivial.

> >> The static pools we have right now impose that.
> > 
> > That is a feature, not a bug. If it does not fit your use case, then don't
> > use the VLC picture pool. The only interface contract that mandates the
> > use of picture_pool_t lies between the core and the vout display, and it
> > only applies to the buffer-pull model.
> 
> You mentioned on your first comment "They may use a picture_pool_t" so I
> don't understand which one is which.

If picture_pool_t as it is suits your purpose, you should probably use it. If 
it does not, then you should probably use something else.

> The pool is a tool. Rather than allocating/freeing pictures all the time
> it seems better to use a pool of pictures to avoid pressure on the
> allocator. It's easy enough to use.

A dynamic pool sounds like a bit of an oxymoron. If you want to allocate 
pictures dynamically, you don't need a pool. The only thing a dynamic pool 
achieves is waste memory by not releasing pictures that are no longer needed.

In fact, a static pool has that problem also, but at least it enforces a 
safety limit. That is basically the only remaining point of picture_pool_t.

> > However, using dynamic pools for hardware decoding sounds like it will end
> > in epic failure, because of hardware resource limits.
> 
> The dynamic feature is opt-in in the code I provided. Static pools many
> not have a use anymore in the end.

If the user of the pool is also the allocater, then adding new state and 
callbacks is just unnecessary complexity. You'd achieve the same functionality 
with a simple function to add a picture buffer to an existing pool.

Though I still don't really see the point. Using a pool in itself seems like 
unnecesary complexity and undesirable inefficiency when allocations are 
dynamic.

-- 
Rémi Denis-Courmont
http://www.remlab.net/