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

[Luca Barbato]

"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>
>wrote:
>> If a picture is still referenced would be double-freed.
>
>I guess Laurent's rationale was that a picture cannot be referenced by
>the
>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
>all
>> 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
>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...

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

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:
>http://mailman.videolan.org/listinfo/vlc-devel