[vlc-devel] [RFC PATCH 0/3] decoder: mutexes and conds cleanup

[Thomas Guillem]

On Wed, Apr 1, 2015, at 17:41, Rémi Denis-Courmont wrote:
> Le mercredi 01 avril 2015, 16:13:15 Thomas Guillem a écrit :
> > I have been trying to fix a race when the input wait for the decoder that
> > has not decoded any data yet and won't decode anything. See mail "[PATCH]
> > decoder: fix race".
> > 
> > My biggest issue was that the function input_DecoderWait waited for 2
> > different variables protected by 2 different mutexes and signaled by 2
> > different cond. So I decided to minimize the number of mutexes and conds.
> 
> Your "biggest issue" does not exist... input_DecoderWait() only waits on 
> wait_acknowledge (#L2140).
> 
> > Now, in decoder.c, the input Fifo and the DecoderThread share the same mutex
> > and the same cond.
> 
> It should be possible to merge wait_acknowledge and wait_fifo, while
> moving the 
> corresponding state from the owner lock to the FIFO lock. But I don't
> think 
> that suppressing the owner lock completely is a good idea. And in any
> case, we 
> need at the very least two condition variables (one in each direction),
> from 
> the four that currently exist.

I always wondered if I should use one condition variable per direction,
I don't always do it. I think it's better for code clarity, but I don't
see any others advantages. Anyway, I agree, I'll try to do that in the
future.

I have a patch that merge the four conditions into two. But It shouldn't
be applied until some aout_* functions are called inside the owner lock.

> 
> > I had to move some long function call (the ones calling
> > aout/vout) outside the lock in order to don't block or wait too long while
> > the mutex is locked.
> 
> The main point of my most recent changes were to move aout_DecPlay() out
> of 
> the (owner) lock. But to do that, I have to remove all accesses to p_aout 
> outside the lock. And to do that, I have to remove all accesses to p_aout
> from 
> outside the decoder thread. I think I managed to remove
> aout_DecIsEmpty(), but 
> I still have some challenges with aout_DecChangePause().
> 
> tl;dr: you cannot do that so easily.

Thank you for your comments and your clarifications.

> 
> -- 
> Rémi Denis-Courmont
> http://www.remlab.net/
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel