[vlc-commits] picture: do not clobber picture reference count when destroying a pool

[Rémi Denis-Courmont]

vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Wed Aug 14 19:36:31 2013 +0300| [4d9f343c5341399b082b84af8010f28a36c2757e] | committer: Rémi Denis-Courmont

picture: do not clobber picture reference count when destroying a pool

I am not taking a stand on whether pictures should or should not have
references when the pool is destroyed. It is hard to support for some
video outputs (and indeed not all of them do so correctly).
Nevertheless, it can happen, so the core might as well deal with this.

Feel free to undo this and add an assertion, if you fix all affected
code paths.

> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=4d9f343c5341399b082b84af8010f28a36c2757e
---

 src/misc/picture_pool.c |   58 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
index af91a7d..294d671 100644
--- a/src/misc/picture_pool.c
+++ b/src/misc/picture_pool.c
@@ -47,6 +47,7 @@ struct picture_gc_sys_t {
     void (*unlock)(picture_t *);
 
     /* */
+    atomic_bool zombie;
     int64_t tick;
 };
 
@@ -90,23 +91,38 @@ picture_pool_t *picture_pool_NewExtended(const picture_pool_configuration_t *cfg
     if (!pool)
         return NULL;
 
+    /*
+     * NOTE: When a pooled picture is released, it must be returned to the list
+     * of available pictures from its pool, rather than destroyed.
+     * This requires a dedicated release callback, a pointer to the pool and a
+     * reference count. For simplicity, rather than allocate a whole new
+     * picture_t structure, the pool overrides gc.pf_destroy and gc.p_sys when
+     * created, and restores them when destroyed.
+     * There are some implications to keep in mind:
+     *  - The original creator of the picture (e.g. video output display) must
+     *    not manipulate the gc parameters while the picture is pooled.
+     *  - The picture cannot be pooled more than once, in other words, pools
+     *    cannot be stacked/layered.
+     *  - The picture must be available and its reference count equal to one
+     *    when it gets pooled.
+     *  - Picture plane pointers and sizes must not be mangled in any case.
+     */
     for (int i = 0; i < cfg->picture_count; i++) {
         picture_t *picture = cfg->picture[i];
 
-        /* The pool must be the only owner of the picture */
-        assert(!picture_IsReferenced(picture));
-
-        /* Install the new release callback */
+        /* Save the original garbage collector */
         picture_gc_sys_t *gc_sys = malloc(sizeof(*gc_sys));
-        if (!gc_sys)
+        if (unlikely(gc_sys == NULL))
             abort();
         gc_sys->destroy     = picture->gc.pf_destroy;
         gc_sys->destroy_sys = picture->gc.p_sys;
         gc_sys->lock        = cfg->lock;
         gc_sys->unlock      = cfg->unlock;
+        atomic_init(&gc_sys->zombie, false);
         gc_sys->tick        = 0;
 
-        /* */
+        /* Override the garbage collector */
+        assert(atomic_load(&picture->gc.refcount) == 1);
         atomic_init(&picture->gc.refcount, 0);
         picture->gc.pf_destroy = Destroy;
         picture->gc.p_sys      = gc_sys;
@@ -191,17 +207,20 @@ void picture_pool_Delete(picture_pool_t *pool)
         } else {
             picture_gc_sys_t *gc_sys = picture->gc.p_sys;
 
-            assert(atomic_load(&picture->gc.refcount) == 0);
             assert(!pool->picture_reserved[i]);
 
-            /* Restore old release callback */
-            atomic_init(&picture->gc.refcount, 1);
-            picture->gc.pf_destroy = gc_sys->destroy;
-            picture->gc.p_sys      = gc_sys->destroy_sys;
+            /* Restore the original garbage collector */
+            if (atomic_fetch_add(&picture->gc.refcount, 1) == 0)
+            {   /* Simple case: the picture is not locked, destroy it now. */
+                picture->gc.pf_destroy = gc_sys->destroy;
+                picture->gc.p_sys      = gc_sys->destroy_sys;
+                free(gc_sys);
+            }
+            else /* Intricate case: the picture is still locked and the gc
+                    cannot be modified (w/o memory synchronization). */
+                atomic_store(&gc_sys->zombie, true);
 
             picture_Release(picture);
-
-            free(gc_sys);
         }
     }
     free(pool->picture_reserved);
@@ -263,7 +282,18 @@ int picture_pool_GetSize(picture_pool_t *pool)
 
 static void Destroy(picture_t *picture)
 {
+    picture_gc_sys_t *gc_sys = picture->gc.p_sys;
+
     Unlock(picture);
+
+    if (atomic_load(&gc_sys->zombie))
+    {   /* Picture from an already destroyed pool */
+        picture->gc.pf_destroy = gc_sys->destroy;
+        picture->gc.p_sys      = gc_sys->destroy_sys;
+        free(gc_sys);
+
+        picture->gc.pf_destroy(picture);
+    }
 }
 
 static int Lock(picture_t *picture)
@@ -273,10 +303,10 @@ static int Lock(picture_t *picture)
         return gc_sys->lock(picture);
     return VLC_SUCCESS;
 }
+
 static void Unlock(picture_t *picture)
 {
     picture_gc_sys_t *gc_sys = picture->gc.p_sys;
     if (gc_sys->unlock)
         gc_sys->unlock(picture);
 }
-