[vlc-devel] [PATCH 3/3] avcodec: va: use a vlc_sem_t to manage the available surfaces

[Steve Lhomme]

BTW, VDPAU has a similar system using the same 
vlc_tick_sleep(VOUT_OUTMEM_SLEEP);

It could be adapted the same way to use vlc_sem_t. A notable difference 
is that the surface (vlc_vdp_video_field_t) it's giving is a clone of 
the original vlc_vdp_video_field_t. So it can differentiate whether the 
release() is for the surface seen by the decoder or the internal 
release. I could adapt va_surface to do the same although I don't think 
it's necessary.

On 2020-07-09 17:40, Steve Lhomme wrote:
> On 2020-07-09 16:33, Rémi Denis-Courmont wrote:
>> Le jeudi 9 juillet 2020, 14:42:02 EEST Steve Lhomme a écrit :
>>> This is cleaner than using a loop of vlc_tick_sleep(VOUT_OUTMEM_SLEEP);
>>>
>>> We don't potentially wait 19.9999ms before giving a surface to the 
>>> decoder.
>>> ---
>>>   modules/codec/avcodec/va_surface.c | 27 +++++++++++++++++----------
>>>   1 file changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/modules/codec/avcodec/va_surface.c
>>> b/modules/codec/avcodec/va_surface.c index 9045273fa3f..bbf3f6b06cb 
>>> 100644
>>> --- a/modules/codec/avcodec/va_surface.c
>>> +++ b/modules/codec/avcodec/va_surface.c
>>> @@ -56,6 +56,7 @@ struct va_pool_t
>>>       struct va_pool_cfg callbacks;
>>>
>>>       atomic_uintptr_t  poolrefs; // 1 ref for the pool creator, 1 
>>> ref per
>>> surface alive +    vlc_sem_t    available_surfaces;
>>>   };
>>>
>>>   static void va_pool_AddRef(va_pool_t *va_pool)
>>> @@ -102,6 +103,8 @@ int va_pool_SetupDecoder(vlc_va_t *va, va_pool_t
>>> *va_pool, AVCodecContext *avctx va_pool->surface_height = fmt->i_height;
>>>       va_pool->surface_count = count;
>>>
>>> +    vlc_sem_init(&va_pool->available_surfaces, count);
>>> +
>>>       for (size_t i = 0; i < va_pool->surface_count; i++) {
>>>           vlc_va_surface_t *surface = &va_pool->surface[i];
>>>           atomic_init(&surface->refcount, 1);
>>> @@ -139,12 +142,9 @@ vlc_va_surface_t *va_pool_Get(va_pool_t *va_pool)
>>>       if (va_pool->surface_count == 0)
>>>           return NULL;
>>>
>>> -    while ((surface = GetSurface(va_pool)) == NULL)
>>> -    {
>>> -        /* Pool empty. Wait for some time as in src/input/decoder.c.
>>> -         * XXX: Both this and the core should use a semaphore or a 
>>> CV. */
>>> -        vlc_tick_sleep(VOUT_OUTMEM_SLEEP);
>>> -    }
>>> +    vlc_sem_wait(&va_pool->available_surfaces);
>>> +    surface = GetSurface(va_pool);
>>> +    assert(surface != NULL);
>>>       return surface;
>>>   }
>>>
>>> @@ -155,10 +155,17 @@ void va_surface_AddRef(vlc_va_surface_t *surface)
>>>
>>>   void va_surface_Release(vlc_va_surface_t *surface)
>>>   {
>>> -    if (atomic_fetch_sub(&surface->refcount, 1) != 1)
>>> -        return;
>>> -
>>> -    va_pool_Release(surface->va_pool);
>>> +    uintptr_t had_refcount = atomic_fetch_sub(&surface->refcount, 1);
>>> +    if (had_refcount == 2)
>>> +    {
>>> +        // the surface is not used anymore
>>> +        vlc_sem_post(&surface->va_pool->available_surfaces);
>>> +    }
>>> +    else if (had_refcount == 1)
>>> +    {
>>> +        // the surface has been released
>>> +        va_pool_Release(surface->va_pool);
>>> +    }
>>>   }
>>>
>>>   size_t va_surface_GetIndex(const vlc_va_surface_t *surface)
>>
>> I don't follow the logic. There's no guarantees that the last reference
>> belongs to any thing or other. The pool could be destroyed before the
>> pictures.
> 
> This is how it's designed to work:
> 
> refcount; // 1 ref for the surface existence, 1 per surface/clone in-flight
> 
> If you end up with a reference of 0 (so had_refcount=1) you are sure 
> after that the surface cannot be used anymore. (code is equivalent as 
> before).
> 
> If you end up with a reference of 1 (so had_refcount=2) that means the 
> surface was queried by the decoder and most likely pushed for 
> playback/filtering/use.
> If you are in a rare case that the decoder asks for the picture but ends 
> up releasing it early, you still release it for the decoder to use it 
> for the next time.
> There's also the case of draining pictures at the end of a stream. In 
> this case the pool is released before the last picture. It's true that 
> we'll reach had_refcount = 2 when the surface is still pending for 
> display. But the decoder is not going to query for one anymore until the 
> last picture is used.
> 
> I don't know if there's another draining case where it starts draining 
> and before the draining is done it starts using new pictures. Even 
> though the reference help by the pool wouldn't change if the pool is not 
> released. So it would just end up in the case where it gets/releases 
> references only for the decoder.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel