[vlc-devel] [PATCH 3/8] picture_pool: use the internal condition/lock indirectly
Steve Lhomme
robux4 at ycbcr.xyz
Mon Jul 27 19:40:49 CEST 2020
On 2020-07-27 18:03, Rémi Denis-Courmont wrote:
> 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.
I agree this is not nice. But I didn't find a cleaner way.
> 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.
I don't think I said there's no deadlock anymore. There is one right now
at least. It's easy to reproduce, you pause playbak and the stop (or
exit). If the decoder is waiting on another pool than decoder->out_pool
it deadlocks.
In lavc it happens in one of the decoder threads. And that thread holds
the decoder lock, so another thread doesn't come and change the output
format before this frame is done with. That means there is no way to use
a callback that would tell lavc to stop waiting, because we would need
the lock to find out if we need to tell the VA or some other pool to
stop. All things that are modified under this lock.
In pull model it was easy. The pool belonged to the vout so it was not
tied to such lock. It was possible to cancel the pool regardless of who
is waiting. Now that pools are in many decoders (and in many forms) it
becomes tricky to do this. But we need a solution anyway.
The proposed solution is the cleanest/shortest I came up so far. I first
tried using a callback system, but that's where I ended up on the
decoder lock that should not be removed. Sharing a "wait/cancel
condition" between the input thread and the decoder thread doesn't seem
too bad. I don't know a clean way to do this. There has to be a link
between the two threads so that the input thread can unblock the decoder
thread(s).
> 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)
>
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser
> ma brièveté.
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list