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

[Steve Lhomme]

I think I experiented with that before but didn't get it to work. In 
short the last picture released is first released as a clone and then 
the original picture in the picture pool. This last release is done in 
the picture_pool code which has to free the instance it is running with. 
This didn't work well when I tried.

That's why I created the va_pool API which can deal with this case.

But now that the code is cleaner and you can make this work, it's 
probably better than attaching a context on the pool picture each time 
you get a picture from it.

On 2020-07-01 11:32, Alexandre Janniaux wrote:
> Hi,
> 
> I can wait for the patchset using this yes.
> 
> Just for info here is how I use it:
> 
> When creating the video context
> + I allocate N output pictures
> + I create N context, one for each picture
> + I call picture_pool_New on my pictures
> 
> For the picture context
> + the _copy_ operation hold my video context
> + the _destroy_ operation release my video context
> 
> And for the video context
> + the _destroy_ operation destroy the pool, removes the context
>    from the pictures, free them and release the pictures.
> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> On Wed, Jul 01, 2020 at 09:09:49AM +0200, 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
>>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>