[vlc-devel] [PATCH] picture: always copy context when cloning

[Steve Lhomme]

So I could use this patch right now to replace the nvdec (temporary) 
p_sys usage with an picture context init before putting the pictures in 
the pool.

So feel free to merge (or maybe I can put it in my picture cleanup branch).

On 2020-07-01 9:09, Steve Lhomme wrote:
> This could work but I'd prefer with an example of using this feature 
> (calling picture_pool_New() with pictures that already have a 
> picture_context_t) to see if it's going to lead to leaks of the 
> picture_pool. Right now the ref counting is done outside of the 
> picture_pool and the last clone released releases the pool.
> 
> On 2020-06-30 19:50, Alexandre Janniaux wrote:
>> picture_Clone was copying the original picture context into the new
>> clone while picture_pool_Clone wasn't. As it is possible to create a
>> pool from existing pictures, it's quite handy to be able to associate
>> the graphical resources needed for each picture to their context so
>> that it can be retrieved after a call to picture_pool_Wait/Get.
>>
>> The rationale is that because resources like pixels are shared between
>> the original picture and the clone, we should probably do the same with
>> picture_context and have a unified way for picture producer to
>> reference such resources.
>>
>> This patch remove the special handling in picture_Clone and move it to
>> picture_InternalClone for that reason.
>> ---
>>   src/misc/picture.c      | 11 +++++++----
>>   src/misc/picture_pool.c |  6 ++++--
>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/misc/picture.c b/src/misc/picture.c
>> index e00b1303253..047d4d22477 100644
>> --- a/src/misc/picture.c
>> +++ b/src/misc/picture.c
>> @@ -440,18 +440,21 @@ picture_t *picture_InternalClone(picture_t 
>> *picture,
>>       picture_t *clone = picture_NewFromResource(&picture->format, &res);
>>       if (likely(clone != NULL)) {
>>           ((picture_priv_t *)clone)->gc.opaque = opaque;
>> +
>> +        /* The picture context is responsible for potentially holding 
>> the
>> +         * video context attached to the picture if needed. */
>> +        if (picture->context != NULL)
>> +            clone->context = picture->context->copy(picture->context);
>> +
>>           picture_Hold(picture);
>>       }
>> +
>>       return clone;
>>   }
>>   picture_t *picture_Clone(picture_t *picture)
>>   {
>>       picture_t *clone = picture_InternalClone(picture, 
>> picture_DestroyClone, picture);
>> -    if (likely(clone != NULL)) {
>> -        if (picture->context != NULL)
>> -            clone->context = picture->context->copy(picture->context);
>> -    }
>>       return clone;
>>   }
>> diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
>> index 5f0f41eba8e..1b979d5a23a 100644
>> --- a/src/misc/picture_pool.c
>> +++ b/src/misc/picture_pool.c
>> @@ -90,8 +90,10 @@ static picture_t 
>> *picture_pool_ClonePicture(picture_pool_t *pool,
>>       picture_t *picture = pool->picture[offset];
>>       uintptr_t sys = ((uintptr_t)pool) + offset;
>> -    return picture_InternalClone(picture, picture_pool_ReleasePicture,
>> -                                 (void*)sys);
>> +    picture_t *clone = picture_InternalClone(picture,
>> +                                             
>> picture_pool_ReleasePicture,
>> +                                             (void*)sys);
>> +    return clone;
>>   }
>>   picture_pool_t *picture_pool_New(unsigned count, picture_t *const *tab)
>> -- 
>> 2.27.0
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>>