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

[Rémi Denis-Courmont]

On Thu, 11 Jul 2013 10:31:28 +0200, Luca Barbato <lu_zero at gentoo.org>
wrote:
>>> diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
>>> index 14db3b6..356aff3 100644
>>> --- a/src/misc/picture_pool.c
>>> +++ b/src/misc/picture_pool.c
>>> @@ -195,11 +195,6 @@ void picture_pool_Delete(picture_pool_t *pool)
>>>              assert(vlc_atomic_get(&picture->gc.refcount) == 0);
>>>              assert(!pool->picture_reserved[i]);
>>> 
>>> -            /* Restore old release callback */
>>> -            vlc_atomic_set(&picture->gc.refcount, 1);
>>> -            picture->gc.pf_destroy = gc_sys->destroy;
>>> -            picture->gc.p_sys      = gc_sys->destroy_sys;
>>> -
>>
>>You can't do that. The original saved garbage collection context will
>>be
>>lost when picture_pool_Delete() returns. So instead of actually freeing
>>the
>>picture, the destroy callback will try to access the already freed pool
>>internals, and (if it does not crash) leak the picture forever.
> 
> Valgrind and asan claim otherwise...

Then where and how do you think the pictures gets freed?

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.

That will possibly expose hidden bugs though. I am not volunteering to
rewrite or fix the video output core, so I won't do that myself.

>>And last but not least, the video output plugins assume that pictures
>>are
>>destroyed when they destroy their pool. Video output pictures must
>>obviously be destroyed before the video output that knows how to
>>allocate
>>and deallocate them. (This limitation could be avoided on X11: you
>>would
>>need to add support for SysV SHM pictures to the VLC core. I on't know
>>about platforms though.)
> 
> Then you should flush the related decoder before. It isn't happening now
> though.

Either the decoder and the filter chains should be flushed, or the picture
pools should deal with pictures outliving their pools. The latter is less
prone to deadlocks but it would require some changes to the video output
plugins, especially the ones with "invalid" pictures.

. I know there are limitations, such as the impossibility for conversion
filters to retain pictures, around which VDPAU chroma must work.

But regardless, I don't think your "fix" works any better than what we
have now.

> Looks like there is more stuff behaving unexpectedly.

I don't know what your problem is, so I can't say.

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