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

[Rémi Denis-Courmont]

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.

> 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?

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

>> I don't disagree that overloading the picture reference count and
>> destroy callback is ugly, but you can't just remove it. Instead you
>> should assert that the picture is not referenced.
> 
> The picture are referenced, twice. One by the vout, one by the threaded
> decoder. The vout is being closed and reopened, the decoder left
> churning frames. Calling that function as it is pulls the pictures from
> the decoder hands, thus causing a double free at least.

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

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.

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