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

[David Fuhrmann]

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

> Le mardi 26 mars 2013 22:58:25, David Fuhrmann a écrit :
>> 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).
> 
> To disable compiler optimization you would need to add the volatile keyword at 
> the very least. In other words, no. But it's irrelevant anyway.

volatile is already used. And this is relevant for me because this assumption is made in several places in the code.

> 
>> 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.
> 
> The whole point is that this is the same problem and locking the mutex changes 
> nothing.

I gave an example where locking the mutex around vlc_cond_signal *will* change something, in my eyes. Just saying the opposite is no helpful argument for me.

> It just moves the race to the time between moving the tail and 
> locking the mutex, instead of the time between moving the tail and signaling 
> the variable.

This is another scenario. Now you are mentioning the time between consuming the buffer, and calling pthread_cond_signal.
In this scenario it can be even assumed that the buffer is not refilling anymore (if Flush was already called). So I still fail to find a schedule for calls to Flush at the one thread, and calls to RenderCallbackAnalog on the other thread, where a deadlock might occur. This is why I asked if you have a concrete example (schedule).

I admit that current checking for the buffer fill count might be a problem as it uses the return value. But if I change it to something like
vlc_mutex_lock(&p_sys->lock);
        TPCircularBufferTail(&p_sys->circular_buffer, &availableBytes);
        while (availableBytes > 0) {
            vlc_cond_wait(&p_sys->cond, &p_sys->lock);
            TPCircularBufferTail(&p_sys->circular_buffer, &availableBytes);
        }
vlc_mutex_unlock(&p_sys->lock);

we come back to my question above in how this readout of the buffer count (availableBytes) can be considered atomic.
(Or what might be problematic if it is not atomic? As availableBytes is only decreasing, the while loop will end in any case, at some time.)

Best regards,
David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130328/0c9f53fa/attachment.html>