[vlc-devel] [PATCH 3/8] picture_pool: use the internal condition/lock indirectly

[Rémi Denis-Courmont]

Hi,

That's awful design IMO. A pool should be a self-standing object and the means to synchronise a threaded object are an implementation detail thereof, not somethign that should leak out.

Ideally the pool cancellation hack should be removed, as Thomas and you claimed that there are no deadlocks anymore. But if it needs to stay, there are no reasons to change it.

Le 27 juillet 2020 16:45:24 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>So we can use an external one.
>---
> src/misc/picture_pool.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
>diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
>index 2605b37454f..82b49ebd4c4 100644
>--- a/src/misc/picture_pool.c
>+++ b/src/misc/picture_pool.c
>@@ -40,6 +40,7 @@ static_assert ((POOL_MAX & (POOL_MAX - 1)) == 0, "Not
>a power of two");
> 
> struct picture_pool_t {
>     vlc_cancelable cancel;
>+    vlc_cancelable *cancelable;
> 
>     unsigned long long available;
>     atomic_ushort      refs;
>@@ -73,11 +74,11 @@ static void picture_pool_ReleaseClone(picture_t
>*clone)
> 
>     picture_Release(picture);
> 
>-    vlc_mutex_lock(&pool->cancel.lock);
>+    vlc_mutex_lock(&pool->cancelable->lock);
>     assert(!(pool->available & (1ULL << offset)));
>     pool->available |= 1ULL << offset;
>-    vlc_cond_signal(&pool->cancel.wait);
>-    vlc_mutex_unlock(&pool->cancel.lock);
>+    vlc_cond_signal(&pool->cancelable->wait);
>+    vlc_mutex_unlock(&pool->cancelable->lock);
> 
>     picture_pool_Destroy(pool);
> }
>@@ -106,6 +107,7 @@ picture_pool_t *picture_pool_New(unsigned count,
>picture_t *const *tab)
>         return NULL;
> 
>     vlc_cancelable_Init( &pool->cancel );
>+    pool->cancelable = &pool->cancel;
> 
>     if (count == POOL_MAX)
>         pool->available = ~0ULL;
>@@ -168,7 +170,7 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
> {
>     unsigned long long available;
> 
>-    vlc_mutex_lock(&pool->cancel.lock);
>+    vlc_mutex_lock(&pool->cancelable->lock);
>     assert(pool->refs > 0);
>     available = pool->available;
> 
>@@ -176,11 +178,11 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
>     {
>         int i = ctz(available);
> 
>-        if (unlikely(pool->cancel.canceled))
>+        if (unlikely(pool->cancelable->canceled))
>             break;
> 
>         pool->available &= ~(1ULL << i);
>-        vlc_mutex_unlock(&pool->cancel.lock);
>+        vlc_mutex_unlock(&pool->cancelable->lock);
>         available &= ~(1ULL << i);
> 
>         picture_t *clone = picture_pool_ClonePicture(pool, i);
>@@ -191,27 +193,27 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
>         return clone;
>     }
> 
>-    vlc_mutex_unlock(&pool->cancel.lock);
>+    vlc_mutex_unlock(&pool->cancelable->lock);
>     return NULL;
> }
> 
> picture_t *picture_pool_Wait(picture_pool_t *pool)
> {
>-    vlc_mutex_lock(&pool->cancel.lock);
>+    vlc_mutex_lock(&pool->cancelable->lock);
>     assert(pool->refs > 0);
> 
>     while (pool->available == 0)
>     {
>-        if (!vlc_cancelable_Wait(&pool->cancel))
>+        if (!vlc_cancelable_Wait(pool->cancelable))
>         {
>-            vlc_mutex_unlock(&pool->cancel.lock);
>+            vlc_mutex_unlock(&pool->cancelable->lock);
>             return NULL;
>         }
>     }
> 
>     int i = ctz(pool->available);
>     pool->available &= ~(1ULL << i);
>-    vlc_mutex_unlock(&pool->cancel.lock);
>+    vlc_mutex_unlock(&pool->cancelable->lock);
> 
>     picture_t *clone = picture_pool_ClonePicture(pool, i);
>     if (clone != NULL) {
>@@ -224,7 +226,7 @@ picture_t *picture_pool_Wait(picture_pool_t *pool)
> void picture_pool_Cancel(picture_pool_t *pool, bool canceled)
> {
>     assert(pool->refs > 0);
>-    vlc_cancelable_SetCanceled( &pool->cancel, canceled );
>+    vlc_cancelable_SetCanceled( pool->cancelable, canceled );
> }
> 
> unsigned picture_pool_GetSize(const picture_pool_t *pool)
>-- 
>2.26.2
>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200727/13711713/attachment.html>