[vlc-devel] [PATCH] picture_pool: do not unconditionally free the pictures

[Rémi Denis-Courmont]

On Fri, 12 Jul 2013 01:46:09 +0200, Luca Barbato <lu_zero at gentoo.org>
wrote:
> On 07/11/2013 09:48 PM, fenrir at elivagar.org wrote:
>> The current code will not work if some of the pictures are not released
>> before
>> the pool.
> 
> Thus why I said that we can have two functions, one to wipe and the
> other just just drop the pool (assuming another pool is taking care of
> the pictures, as it is happening now).

That would only move the problem. With that, the wiped pools would have to
be kept around somewhere... until some as yet unspecified point of time. I
don't really see the point.

If we decide to allow pictures to outlive their pool, then a simplified
and fixed variant of your patch should work. Of course, that would just the
tip of the iceberg; then every video output would need to be checked, and
some of them fixed, to deal with that change in semantics.

>>> The whole thing is a mess since the assumption (wrong) is that you
>>> call that function when there aren't any other active user holding a
>>> reference (assuming something went really wrong and whoever had that
>>> additional reference died an horrible and dirty death).
>>>
>>> That's not the case and the whole assumption looks wrong from start to
>>> me.
> 
>> I disagree. I mean it's obviously not the only possible solution, but
>> is a valid one if some requirements are respected.

I think it was very valid and obvious way back when. Nowadays, looking at
the Linux port, there is not a single *genuine* direct rendering video
output. And modern codecs require more picture references than scanout
buffers exposed by typical hardware abstractions, so even if there were a
direct output, direct rendering would probably not work all the way back to
the decoder.

OpenGL requires a picture upload/copy if I recall correctly. XVideo and
X11 require a memory copy in the X server. DRI2 is not supported by VLC
(and if it were, it would require scaling and YCbCr conversion in
software).

> It assumes that after the call nothing with a reference to the picture
> is active anymore. Sadly it is not happening.

Lets not get into a fight on terminology. The current approach is
theoretically valid, with advantages and disadvantages, but its current
implementation is buggered.

> It assumes that some code would leak pictures. That should be fixed
> notwithstanding.

Filters should be fine. The problem is with broken decoders. I doubt
fixing all decoders and keeping them fixed forever is very pragmatic. This
may need a separate list of pictures held by decoders. That should be easy
since decoders use decoder-specific functions for allocation and
deallocation of pictures.

>> The asumption the vout makes is that a decoder will NOT have any
>> pictures references left before making a resolution changes.
> 
> Depends on the decoder, if you want a native QSV codec you'd have the
> same issue. Same goes for other hwaccelerated ones.
> 
>> Up until threaded avcodec, this assumption was valid and thus it was
>> not necessary to worry about this issue.
>> (I know that this also means that on a resolution changes a few
>> pictures were lost).
> 
>> Now, if the avcodec decoder ends up referencing pictures before and
>> after a resolution change, the current code will not work.
> 
>> IMHO, the simplest and most risk free fix would be to ensure that the
>> decoder flush itself on resize i.e. obeying one of the the vout
>> requirements.

It might not be that easy. I am wary of dead locks. I am also not certain
that the design of newer multi-threaded or hardware-assisted codecs will be
very amenable thereto.

> I pestered people on give me a map so I can do that myself, I tend to
> get lost jumping from a p_sys to a p_owner usually ^^;
> 
>> Another solution could be to refcount the pool and destroy it only when
>> it is unreferenced and all the pictures associated are unreferenced
>> too. But this opens a new can of worm (you must ensure that the
>> vout_display does not change or refcount it as it may be responsible
>> for releasing them, it also means that you allocate twice video memory
>> and this is a scarse resource when using HD, etc)

No. Just release the remaining pictures one-by-one when their reference
count drops to zero, after the pool is destroyed. That way the memory
overhead will be much smaller in most cases, just a few pictures rather
than the whole pool.

> Keeping a refcount on the pool seems sort of simple, if there are more
> references to the pool you just do the normal release, if the count is
> zero you do a wipe over the pool and warn about broken code not cleaning
> up properly.

I do not see any benefit to that third approach. It consumes more memory,
is more intricate, and still requires changes to the video outputs.

-- 
Rémi Denis-Courmont
Sent from my collocated server