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

[Steve Lhomme]

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.