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

Luca Barbato lu_zero at gentoo.org
Thu Jul 11 10:31:28 CEST 2013

"Rémi Denis-Courmont" <remi at remlab.net> wrote:

>On Thu, 11 Jul 2013 08:20:29 +0200, Luca Barbato <lu_zero at gentoo.org>
>> If a picture is still referenced would be double-freed.
>I guess Laurent's rationale was that a picture cannot be referenced by
>time the pool is deleted.

Sadly does not work this way, there is something wrong in the vo reshape logic then.

>> ---
>> If there are places that assume that picture_pool_Delete must wipe
>> the references doesn't matter what we can add a second function.
>>  src/misc/picture_pool.c | 5 -----
>>  1 file changed, 5 deletions(-)
>> 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
>lost when picture_pool_Delete() returns. So instead of actually freeing
>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...

>And last but not least, the video output plugins assume that pictures
>destroyed when they destroy their pool. Video output pictures must
>obviously be destroyed before the video output that knows how to
>and deallocate them. (This limitation could be avoided on X11: you
>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.

Looks like there is more stuff behaving unexpectedly.

>>              picture_Release(picture);
>>              free(gc_sys);
>Rémi Denis-Courmont
>Sent from my collocated server
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:

More information about the vlc-devel mailing list