codec/avcodec/vaapi*: Mark difference between FREE, DECODE, DISPLAY of surface.

The surface i_refcount and i_order counter are no longer sufficient
to tell which surface is free to use.

modules/codec/avcodec/va.h

@@ -27,6 +27,7 @@
 typedef struct vlc_va_t vlc_va_t;
 struct vlc_va_t {
     char *description;
+    vlc_object_t *obj;
 
     int  (*setup)(vlc_va_t *, void **hw, vlc_fourcc_t *output,
                   int width, int height);

modules/codec/avcodec/vaapi.c

@@ -179,6 +179,7 @@ static void DestroySurfaces( vlc_va_vaapi_t *p_va )
     for( int i = 0; i < p_va->i_surface_count && p_va->p_surface; i++ )
     {
         vlc_va_surface_t *p_surface = &p_va->p_surface[i];
+        p_surface->i_state = FREE;
         if( p_surface->i_id != VA_INVALID_SURFACE )
             vaDestroySurfaces( p_va->conn->p_display, &p_surface->i_id, 1 );
     }
@@ -223,6 +224,7 @@ static int CreateSurfaces( vlc_va_vaapi_t *p_va, void **pp_hw_ctx, vlc_fourcc_t
         p_surface->i_id = pi_surface_id[i];
         p_surface->i_refcount = 0;
         p_surface->i_order = 0;
+        p_surface->i_state = FREE;
         p_surface->i_cache = VA_INVALID_ID;
     }
 
@@ -405,6 +407,20 @@ error:
     return VLC_EGENERIC;
 }
 
+static void DumpSurfaceState( vlc_va_t *p_external )
+{
+    vlc_va_vaapi_t *p_va = vlc_va_vaapi_Get(p_external);
+
+    msg_Dbg( p_external->obj, "Dumping Surface Array Content" );
+    for( int i = 0; i < p_va->i_surface_count; i++ )
+    {
+        vlc_va_surface_t *p_surface = &p_va->p_surface[i];
+        msg_Dbg( p_external->obj, " %2d: id = %d, order = %3d, refcount = %d, state = %d",
+                 i, p_surface->i_id, p_surface->i_order,
+                 p_surface->i_refcount, (int)p_surface->i_state );
+    }
+}
+
 static vlc_va_surface_t *FindSurface( vlc_va_t *p_external, const VASurfaceID i_surface_id )
 {
     vlc_va_vaapi_t *p_va = vlc_va_vaapi_Get(p_external);
@@ -415,10 +431,16 @@ static vlc_va_surface_t *FindSurface( vlc_va_t *p_external, const VASurfaceID i_
         vlc_va_surface_t *p_tmp = &p_va->p_surface[i];
         if( p_tmp->i_id == i_surface_id )
         {
-            /* NOTE: p_tmp->i_refcount can be greater then 1, when surfaces are being reclaimed
-             * this usually only happens when the vout vaapi-x11 is not instantiated yet.
+            //if( p_tmp->i_state == DISPLAY || p_tmp->i_state == DECODE )
+            //    DumpSurfaceState( p_external );
+
+            assert( p_tmp->i_state == DECODE );
+            assert( p_tmp->i_refcount == 1 );
+            /* NOTE: The surface is still in use by our decoder. Increase the refcount
+             * as we want to keep it until it has been displayed.
              */
             p_tmp->i_refcount++;
+            p_tmp->i_state = DISPLAY;
             p_surface = p_tmp;
             break;
         }
@@ -432,6 +454,7 @@ static int DisplayPicture( vlc_va_t *p_external, picture_t *p_picture, AVFrame *
 
     p_va->conn->lock();
 
+    msg_Info( p_external->obj, "No. Surfaces %d", p_va->i_surface_count );
     VASurfaceID i_surface_id = (VASurfaceID)(uintptr_t)p_ff->data[3];
 
 #if VA_CHECK_VERSION(0,31,0)
@@ -453,6 +476,7 @@ static int DisplayPicture( vlc_va_t *p_external, picture_t *p_picture, AVFrame *
     return VLC_SUCCESS;
 
 error:
+    assert(0);
     p_va->conn->unlock();
     return VLC_EGENERIC;
 }
@@ -469,20 +493,28 @@ static int Get( vlc_va_t *p_external, AVFrame *p_ff )
     {
         vlc_va_surface_t *p_surface = &p_va->p_surface[i];
 
-        if( p_surface->i_refcount == 0 )
+        if( p_surface->i_refcount == 0 &&
+            p_surface->i_state == FREE )
             break;
 
         if( p_surface->i_order < p_va->p_surface[i_old].i_order )
             i_old = i;
     }
     if( i >= p_va->i_surface_count )
+    {
+        vlc_va_surface_t *p_surface = &p_va->p_surface[i];
+        if( p_surface->i_state != FREE )
+            return VLC_EGENERIC;
         i = i_old;
+    }
 
     vlc_va_surface_t *p_surface = &p_va->p_surface[i];
-    /* NOTE: when the surface is in use and not consumed by vout vaapi-x11,
-     * then p_surface->i_refcount can be greater then 0. Thus always increment.
+    assert( p_surface->i_state == FREE );
+    /* NOTE: reset surface refcount otherwise they will be leaked and
+     * mayhem may happen down the road.
      */
     p_surface->i_refcount = 1;
+    p_surface->i_state = DECODE;
     p_surface->i_order = p_va->i_surface_order++;
 
     /* */
@@ -496,6 +528,7 @@ static int Get( vlc_va_t *p_external, AVFrame *p_ff )
     }
     return VLC_SUCCESS;
 }
+
 static void Release( vlc_va_t *p_external, AVFrame *p_ff )
 {
     vlc_va_vaapi_t *p_va = vlc_va_vaapi_Get(p_external);
@@ -507,7 +540,17 @@ static void Release( vlc_va_t *p_external, AVFrame *p_ff )
         vlc_va_surface_t *p_surface = &p_va->p_surface[i];
 
         if( p_surface->i_id == i_surface_id )
-            p_surface->i_refcount--;
+        {
+            if( p_surface->i_refcount > 0 )
+                p_surface->i_refcount--;
+            if (p_surface->i_state == DECODE)
+            {
+                assert( p_surface->i_refcount == 0 );
+                p_surface->i_state = FREE;
+            }
+            else if( p_surface->i_state == DISPLAY )
+                p_surface->i_refcount = 1;
+        }
     }
 }
 
@@ -548,6 +591,7 @@ vlc_va_t *vlc_va_NewVaapi( vlc_object_t *obj, int i_codec_id )
     }
 
     /* */
+    p_va->va.obj = obj;
     p_va->va.setup = Setup;
     p_va->va.get = Get;
     p_va->va.release = Release;

modules/codec/avcodec/vaapi.h

@@ -30,11 +30,13 @@ struct vlc_va_conn_t
     void (*unlock)(void);
 };
 
+typedef enum { FREE=0, DECODE=1, DISPLAY=2 } vlc_va_state_t;
 typedef struct
 {
     VASurfaceID  i_id;
     int          i_refcount;
     unsigned int i_order;
+    vlc_va_state_t i_state;
 
     /* subpicture */
     unsigned int i_cache;
@@ -44,6 +46,7 @@ struct picture_sys_t
 {
     /* surface */
     vlc_va_surface_t *surface;
+    vlc_object_t     *obj;
 };
 
 #endif

modules/codec/avcodec/vaapi_x11.c

@@ -1094,42 +1094,6 @@ cleanup:
     return VLC_EGENERIC;
 }
 
-static void PictureRelease(picture_t *picture)
-{
-    if (picture->i_refcount > 0)
-        picture->i_refcount--;
-    if (picture->p_sys)
-    {
-        vlc_va_surface_t *surface = picture->p_sys->surface;
-
-        if (surface && (surface->i_cache != VA_INVALID_ID))
-        {
-#if 0 /* FIXME: leaking subpicture associations */
-            vlc_va_conn_t *conn = vlc_va_get_conn();
-
-            vlc_mutex_lock(&sys->cache_lock);
-            subpicture_cache_t *cache = cache_find(&sys->cache, surface->i_cache);
-            assert(cache);
-            const int count = vlc_array_count(&cache->subpictures);
-            for (int i = 0; i < count; i++)
-            {
-                vasubpicture_cache_t *vasubpic;
-                vasubpic = (vasubpicture_cache_t *) vlc_array_item_at_index(&cache->subpictures, i_sub);
-                assert(vasubpic);
-                VAStatus status = SubpictureUnlink(conn, surface->i_id, vasubpic->i_id);
-                if (status != VA_STATUS_SUCCESS)
-                    msg_Err(vd, "- %d unlink failed (status = %d)", i, status);
-            }
-            vlc_mutex_unlock(&sys->cache_lock);
-#endif
-        }
-        /* */
-        picture->p_sys->surface = NULL;
-    }
-    free(picture->p_sys);
-    picture->p_sys = NULL;
-}
-
 static void Render(vout_display_t *vd, picture_t *picture, subpicture_t *subpicture)
 {
     vout_display_sys_t *sys = vd->sys;
@@ -1256,6 +1220,58 @@ static void ReleaseSurface(vlc_va_surface_t *surface)
 {
     if (surface->i_refcount > 0)
         surface->i_refcount--;
+    //assert(surface->i_refcount == 0);
+    surface->i_refcount = 0;
+    surface->i_cache = VA_INVALID_ID;
+    surface->i_state = FREE;
+}
+
+static void PicturePrivateRelease(picture_t *picture)
+{
+    /* FIXME: Do we need this picture longer ? */
+    if (picture->p_sys)
+    {
+        vlc_va_surface_t *surface = (vlc_va_surface_t *) picture->p_sys->surface;
+        vout_display_t *vd = (vout_display_t*) picture->p_sys->obj;
+        vout_display_sys_t *sys = (vout_display_sys_t *) vd->sys;
+
+        if (surface)
+        {
+#if 0 /* FIXME: leaking subpicture associations */
+            if (surface->i_cache != VA_INVALID_ID))
+            {
+                vlc_va_conn_t *conn = vlc_va_get_conn();
+
+                vlc_mutex_lock(&sys->cache_lock);
+                subpicture_cache_t *cache = cache_find(&sys->cache, surface->i_cache);
+                assert(cache);
+                const int count = vlc_array_count(&cache->subpictures);
+                for (int i = 0; i < count; i++)
+                {
+                    vasubpicture_cache_t *vasubpic;
+                    vasubpic = (vasubpicture_cache_t *) vlc_array_item_at_index(&cache->subpictures, i_sub);
+                    assert(vasubpic);
+                    VAStatus status = SubpictureUnlink(conn, surface->i_id, vasubpic->i_id);
+                    if (status != VA_STATUS_SUCCESS)
+                        msg_Err(vd, "- %d unlink failed (status = %d)", i, status);
+                }
+                vlc_mutex_unlock(&sys->cache_lock);
+            }
+#endif
+            ReleaseSurface(surface);
+        }
+        /* */
+        picture->p_sys->surface = NULL;
+    }
+}
+
+static void PictureRelease(picture_t *picture)
+{
+    if (picture->i_refcount > 0)
+        picture->i_refcount--;
+
+    free(picture->p_sys);
+    picture->p_sys = NULL;
 }
 
 static void DisplayVASurface(vout_display_t *vd, VASurfaceID surface, picture_t *picture)
@@ -1280,7 +1296,7 @@ static void DisplayVASurface(vout_display_t *vd, VASurfaceID surface, picture_t
     if (status != VA_STATUS_SUCCESS)
         msg_Err(vd, "failed displaying picture: %d (surface id=%d): %p",
                 status, surface, picture);
-
+    msg_Dbg(vd, "diplayed picture (refcount %d)", picture->i_refcount);
     sys->conn->unlock();
 }
 
@@ -1301,7 +1317,8 @@ static void DisplayPicture(vout_display_t *vd, picture_t *pic, subpicture_t *sub
     if (pic->p_sys->surface == NULL)
     {
         assert(pic->format.i_chroma == VLC_CODEC_VAAPI_SURFACE);
-        msg_Err(vd, "discarding picture without surface information: %p", pic);
+        msg_Err(vd, "discarding picture without surface information: %p (refcount %d, forced %s)",
+                pic, pic->i_refcount, pic->b_force ? "true" : "false" );
         if (subpicture)
             subpicture_Delete(subpicture);
         picture_Release(pic);
@@ -1311,7 +1328,8 @@ static void DisplayPicture(vout_display_t *vd, picture_t *pic, subpicture_t *sub
 
     vlc_va_surface_t *surface = pic->p_sys->surface;
 
-    DisplayVASurface(vd, surface->i_id, pic);
+    if (surface->i_state == DISPLAY)
+        DisplayVASurface(vd, surface->i_id, pic);
 
     if (surface->i_cache != VA_INVALID_ID)
     {
@@ -1363,14 +1381,11 @@ static void DisplayPicture(vout_display_t *vd, picture_t *pic, subpicture_t *sub
     }
 
     sys->display.i_cache = surface->i_cache;
-    surface->i_cache = VA_INVALID_ID;
-
     ReleaseSurface(surface);
 
     if (subpicture)
         subpicture_Delete(subpicture);
 
-    pic->p_sys->surface = NULL;
     picture_Release(pic);
 }
 
@@ -1405,10 +1420,12 @@ static picture_pool_t *Pool (vout_display_t *vd, unsigned requested_count)
         pic->format.i_chroma = VLC_CODEC_VAAPI_SURFACE;
 
         sys->surface = NULL;
+        sys->obj = VLC_OBJECT(vd);
 
         pic_array[count] = pic;
         pic_array[count]->b_progressive = true;
         pic_array[count]->p_sys = sys;
+        pic_array[count]->pf_private_release = PicturePrivateRelease;
         pic_array[count]->pf_release = PictureRelease;
     }