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

[Luca Barbato]

On 07/11/2013 12:31 PM, Rémi Denis-Courmont wrote:
> On Thu, 11 Jul 2013 11:26:45 +0200, Luca Barbato <lu_zero at gentoo.org>
> wrote:
>> On 07/11/2013 11:15 AM, Rémi Denis-Courmont wrote:
>>> Then where and how do you think the pictures gets freed?
>>
>> By the code holding the additional references, that does exactly what it
>> is supposed to do =)
> 
> It won't work because you do not restore pf_destroy to its proper value.
> You end up releasing the picture from the (already destroyed) pool instead
> of actually destroying the picture.

Valgrind claims otherwise, probably I'm lucky.

>> 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).
> 
> I agree that it is an ugly mess (not my design), but it used to be much
> worse. But then again, it seems reasonable for video output plugins to rely
> on their pictures no longer being referenced upon invalid pictures reset or
> close. If someone wants to change this, I would be happy to adapt the XCB
> video outputs, but what about the other ones?

I'm not sure what's the best solution, ideally the whole
vout-reset-when-the-decoder-is-active should be avoided.

>> That's not the case and the whole assumption looks wrong from start to
> me.
> 
> I don't know. If the allocated memory is a limited hardware resources
> (rather than just plain dumb main memory), maybe you cannot reallocate a
> new set of pictures before you destroy the old one? Then this design
> limitation kind makes sense. Alternatively, we would have to require that
> any such video outputs sets the slow picture flag (and the flag should be
> renamed accordingly).
> 
> And then there is the problem of picture leaking decoders...

> At the moment, direct rendering does not permit that. Video output plugins
> assume the pictures can be deleted safely when they terminate (or when
> pictures are reset after invalidated).

currently the assumption (wrong) is that the vo gets closed *after* the
decoder. On frame-resize that's not the case.

> Arguably the decoder should flush automatically before it changes format.
> Is there any video codec where pictures reference can cross format change
> boundaries? Admittedly, the codec interface does not make this easy since
> the format change is combined with the picture allocation.

mpeg4, h264, even mpeg2 might change midstream I guess.

lu