[vlc-devel] [vlc-commits] auhal: implement drain

[David Fuhrmann]

Am 26.03.2013 um 19:49 schrieb Rémi Denis-Courmont <remi at remlab.net>:

> Le mardi 26 mars 2013 20:06:12, David Fuhrmann a écrit :
>> If I parsed the relevant parts of your emails in the right way, I assume
>> that you want to complain that I did not extended the scope of the mutex
>> lock to the manipulation of the buffer.
> 
> I wouldn't put it exactly like that, but roughly yes for short.
> 
>> Please note that I already thought in doing so, before committing.
>> But the CircularBuffer already has some thread safety mechanisms using
>> atomic operations.
> 
> That is irrelevant. It's good that CircularBuffer has thread-safety within 
> itself. But that does not cover the predicate of the condition variable.

It covers the fill state of the buffer. And I am assume that loading a 32 bit integer from RAM can be also considered as atomic (at least in the sense that the result is not inconsistent).
This is why I thought that if we use this result (stored into in another variable), it will be ok?

> 
>> For this scenario I assumed that these will suffice for
>> proper thread synchronization.
> 
> Locking the mutex makes no sense if you don't change state. You could remove 
> the locking and unlocking calls to the same effect.
> 
> Then it would be obvious that scheduling predictability was missing. Hence I 
> strongly suspect that waiting can still deadlock with unfortunate timings.
> In particular, I do not see what synchronization directive prevents the render 
> thread from signalling the condition variable in the short time window after 
> the audio thread checks the buffer tial but before it starts waiting in the 
> condition.

But If I understand right, this is *exactly* the scenario where a mutex lock around vlc_cond_signal makes sense!?
In this short time you are mentioning (between getting the size of the buffer and calling vlc_cond_wait again on the thread which calls flush) the mutex is locked.
Therefore, vlc_cond_signal cannot be called by the coreaudio thread.