[vlc-devel] [PATCH 2/2] picture_pool: fix Reserve() not locking/unlocking pictures

[Thomas Guillem]

Pictures cloned from a reserved pool were not locked/unlocked via the master
pool callbacks. This commit fixes this issue. See the new comment in
picture_pool_Reserve().
---
 src/misc/picture_pool.c | 43 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
index 875aff42ab..e97cdbd9ed 100644
--- a/src/misc/picture_pool.c
+++ b/src/misc/picture_pool.c
@@ -66,7 +66,7 @@ void picture_pool_Release(picture_pool_t *pool)
     picture_pool_Destroy(pool);
 }
 
-static void picture_pool_ReleasePicture(picture_t *clone)
+static void picture_pool_ReleasePicture(picture_t *clone, bool locked)
 {
     picture_priv_t *priv = (picture_priv_t *)clone;
     uintptr_t sys = (uintptr_t)priv->gc.opaque;
@@ -76,7 +76,7 @@ static void picture_pool_ReleasePicture(picture_t *clone)
 
     free(clone);
 
-    if (pool->pic_unlock != NULL)
+    if (locked && pool->pic_unlock != NULL)
         pool->pic_unlock(picture);
     picture_Release(picture);
 
@@ -89,14 +89,25 @@ static void picture_pool_ReleasePicture(picture_t *clone)
     picture_pool_Destroy(pool);
 }
 
+static void picture_pool_ReleasePictureLocked(picture_t *clone)
+{
+    picture_pool_ReleasePicture(clone, true);
+}
+
+static void picture_pool_ReleasePictureUnlocked(picture_t *clone)
+{
+    picture_pool_ReleasePicture(clone, false);
+}
+
 static picture_t *picture_pool_ClonePicture(picture_pool_t *pool,
-                                            unsigned offset)
+                                            unsigned offset, bool locked)
 {
     picture_t *picture = pool->picture[offset];
     uintptr_t sys = ((uintptr_t)pool) + offset;
     picture_resource_t res = {
         .p_sys = picture->p_sys,
-        .pf_destroy = picture_pool_ReleasePicture,
+        .pf_destroy = locked ? picture_pool_ReleasePictureLocked
+                             : picture_pool_ReleasePictureUnlocked,
     };
 
     for (int i = 0; i < picture->i_planes; i++) {
@@ -121,7 +132,7 @@ static int fnsll(unsigned long long x, unsigned i)
     return ffsll(x & ~((1ULL << i) - 1));
 }
 
-static picture_t *picture_pool_GetInternal(picture_pool_t *pool)
+static picture_t *picture_pool_GetInternal(picture_pool_t *pool, bool lock)
 {
     vlc_mutex_lock(&pool->lock);
     assert(pool->refs > 0);
@@ -139,7 +150,7 @@ static picture_t *picture_pool_GetInternal(picture_pool_t *pool)
 
         picture_t *picture = pool->picture[i - 1];
 
-        if (pool->pic_lock != NULL
+        if (lock && pool->pic_lock != NULL
          && pool->pic_lock(picture) != VLC_SUCCESS)
         {
             vlc_mutex_lock(&pool->lock);
@@ -147,7 +158,7 @@ static picture_t *picture_pool_GetInternal(picture_pool_t *pool)
             continue;
         }
 
-        picture_t *clone = picture_pool_ClonePicture(pool, i - 1);
+        picture_t *clone = picture_pool_ClonePicture(pool, i - 1, lock);
         if (clone != NULL) {
             assert(clone->p_next == NULL);
             atomic_fetch_add(&pool->refs, 1);
@@ -222,12 +233,22 @@ picture_pool_t *picture_pool_Reserve(picture_pool_t *master, unsigned count)
     unsigned i;
 
     for (i = 0; i < count; i++) {
-        picture[i] = picture_pool_GetInternal(master);
+        /* Pictures that are used to initialize the reserved pool should not be
+         * locked and unlocked (hence the false). Only pictures cloned from the
+         * reserved pool should be locked/unlocked with the same callbacks than
+         * the master ones. */
+        picture[i] = picture_pool_GetInternal(master, false);
         if (picture[i] == NULL)
             goto error;
     }
 
-    picture_pool_t *pool = picture_pool_New(count, picture);
+    picture_pool_configuration_t pool_cfg = {
+        .picture_count = count,
+        .picture       = picture,
+        .lock          = master->pic_lock,
+        .unlock        = master->pic_unlock,
+    };
+    picture_pool_t *pool = picture_pool_NewExtended(&pool_cfg);
     if (!pool)
         goto error;
 
@@ -241,7 +262,7 @@ error:
 
 picture_t *picture_pool_Get(picture_pool_t *pool)
 {
-    return picture_pool_GetInternal(pool);
+    return picture_pool_GetInternal(pool, true);
 }
 
 picture_t *picture_pool_Wait(picture_pool_t *pool)
@@ -276,7 +297,7 @@ picture_t *picture_pool_Wait(picture_pool_t *pool)
         return NULL;
     }
 
-    picture_t *clone = picture_pool_ClonePicture(pool, i - 1);
+    picture_t *clone = picture_pool_ClonePicture(pool, i - 1, true);
     if (clone != NULL) {
         assert(clone->p_next == NULL);
         atomic_fetch_add(&pool->refs, 1);
-- 
2.11.0