[vlc-commits] picture_pool: stop meddling with picture reference counters

[Rémi Denis-Courmont]

vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Sat Jun 27 16:34:00 2015 +0300| [22e61f10e14d60578c171b83cb5f9d5b5dea82ce] | committer: Rémi Denis-Courmont

picture_pool: stop meddling with picture reference counters

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

 include/vlc_picture_pool.h |    7 +++---
 src/misc/picture_pool.c    |   55 ++++++++++++++------------------------------
 src/test/picture_pool.c    |    7 +++++-
 3 files changed, 27 insertions(+), 42 deletions(-)

diff --git a/include/vlc_picture_pool.h b/include/vlc_picture_pool.h
index 37ef7b3..1ce92ce 100644
--- a/include/vlc_picture_pool.h
+++ b/include/vlc_picture_pool.h
@@ -137,9 +137,10 @@ VLC_API void picture_pool_Enum( picture_pool_t *,
 /**
  * Forcefully return all pictures in the pool to free/unallocated state.
  *
- * @warning This can only be called when it is known that all pending
- * references to the picture pool are stale, e.g. a decoder failed to
- * release pictures properly when it terminated.
+ * @warning If any picture in the pool is not free, this function will leak
+ * and may eventually cause invalid memory accesses.
+ *
+ * @note This function has no effects if all pictures in the pool are free.
  *
  * @return the number of picture references that were freed
  */
diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
index c544253..77d613f 100644
--- a/src/misc/picture_pool.c
+++ b/src/misc/picture_pool.c
@@ -69,14 +69,8 @@ void picture_pool_Release(picture_pool_t *pool)
     if (likely(!destroy))
         return;
 
-    for (unsigned i = 0; i < pool->picture_count; i++) {
-        picture_t *picture = pool->picture[i];
-        picture_priv_t *priv = (picture_priv_t *)picture;
-
-        picture_Release(priv->gc.opaque->picture);
-        free(priv->gc.opaque);
-        free(picture);
-    }
+    for (unsigned i = 0; i < pool->picture_count; i++)
+        picture_Release(pool->picture[i]);
 
     vlc_mutex_destroy(&pool->lock);
     free(pool);
@@ -89,7 +83,7 @@ static void picture_pool_ReleasePicture(picture_t *picture)
     picture_pool_t *pool = sys->pool;
 
     if (pool->pic_unlock != NULL)
-        pool->pic_unlock(picture);
+        pool->pic_unlock(sys->picture);
 
     vlc_mutex_lock(&pool->lock);
     assert(!(pool->available & (1ULL << sys->offset)));
@@ -97,6 +91,9 @@ static void picture_pool_ReleasePicture(picture_t *picture)
     vlc_mutex_unlock(&pool->lock);
 
     picture_pool_Release(pool);
+
+    free(sys);
+    free(picture);
 }
 
 static picture_t *picture_pool_ClonePicture(picture_pool_t *pool,
@@ -147,16 +144,8 @@ picture_pool_t *picture_pool_NewExtended(const picture_pool_configuration_t *cfg
     pool->available = (1ULL << cfg->picture_count) - 1;
     pool->refs = 1;
     pool->picture_count = cfg->picture_count;
-
-    for (unsigned i = 0; i < cfg->picture_count; i++) {
-        picture_t *picture = picture_pool_ClonePicture(pool, cfg->picture[i], i);
-        if (unlikely(picture == NULL))
-            abort();
-
-        atomic_init(&((picture_priv_t *)picture)->gc.refs, 0);
-
-        pool->picture[i] = picture;
-    }
+    memcpy(pool->picture, cfg->picture,
+           cfg->picture_count * sizeof (picture_t *));
     return pool;
 
 }
@@ -224,8 +213,6 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
     assert(pool->refs > 0);
 
     for (unsigned i = 0; i < pool->picture_count; i++) {
-        picture_t *picture = pool->picture[i];
-
         if (!(pool->available & (1ULL << i)))
             continue;
 
@@ -233,6 +220,8 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
         pool->refs++;
         vlc_mutex_unlock(&pool->lock);
 
+        picture_t *picture = pool->picture[i];
+
         if (pool->pic_lock != NULL && pool->pic_lock(picture) != 0) {
             vlc_mutex_lock(&pool->lock);
             pool->available |= 1ULL << i;
@@ -240,10 +229,9 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
             continue;
         }
 
-        assert(atomic_load(&((picture_priv_t *)picture)->gc.refs) == 0);
-        atomic_init(&((picture_priv_t *)picture)->gc.refs, 1);
-        picture->p_next = NULL;
-        return picture;
+        picture_t *clone = picture_pool_ClonePicture(pool, picture, i);
+        assert(unlikely(clone == NULL) || clone->p_next == NULL);
+        return clone;
     }
 
     vlc_mutex_unlock(&pool->lock);
@@ -252,21 +240,12 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
 
 unsigned picture_pool_Reset(picture_pool_t *pool)
 {
-    unsigned ret = 0;
-retry:
+    unsigned ret;
+
     vlc_mutex_lock(&pool->lock);
     assert(pool->refs > 0);
-
-    for (unsigned i = 0; i < pool->picture_count; i++) {
-        picture_t *picture = pool->picture[i];
-
-        if (!(pool->available & (1ULL << i))) {
-            vlc_mutex_unlock(&pool->lock);
-            picture_Release(picture);
-            ret++;
-            goto retry;
-        }
-    }
+    ret = pool->picture_count - popcountll(pool->available);
+    pool->available = (1ULL << pool->picture_count) - 1;
     vlc_mutex_unlock(&pool->lock);
 
     return ret;
diff --git a/src/test/picture_pool.c b/src/test/picture_pool.c
index 93d70c0..2ff3d51 100644
--- a/src/test/picture_pool.c
+++ b/src/test/picture_pool.c
@@ -60,8 +60,13 @@ static void test(bool zombie)
         picture_Release(pics[i]);
 
     for (unsigned i = 0; i < PICTURES; i++) {
+        void *plane = pics[i]->p[0].p_pixels;
+        assert(plane != NULL);
         picture_Release(pics[i]);
-        assert(picture_pool_Get(pool) == pics[i]);
+
+        pics[i] = picture_pool_Get(pool);
+        assert(pics[i] != NULL);
+        assert(pics[i]->p[0].p_pixels == plane);
     }
 
     for (unsigned i = 0; i < PICTURES; i++)