[MPlayer-dev-eng] [PATCH] ao_dart

[Reimar Döffinger]

On Wed, Feb 18, 2009 at 12:47:14AM +0900, KO Myung-Hun wrote:
>>> The reason why I didn't restore it, is that enabling SIG_FPE seems to be 
>>> a bug as the above.
>>>     
>>
>> Ok, but so what does enable it, the dartPlay function?
>>   
>
> Some PM DLLs loaded at init time. So SIG_FPE should be disabled after all 
> the PM DLLs are loaded. And if there is a problem, it would be dartInit() 
> because it loads system DLL dynamically.

Add a comment like
// might cause PM DLLs to be loaded which incorrectly enable SIG_FPE,
// which AAC decoding might trigger

>>> +static inline int query_buffer_len(void)
>>> +{
>>> +    int iBufLen = m_iBufWritePos - m_iBufReadPos;
>>> +
>>> +    if (iBufLen < 0 || m_fBufFull)
>>> +        iBufLen += m_nBufSize;
>>> +
>>> +    return iBufLen;
>>>     
>>
>> You essentially split the write position into two part, thus write to it
>> are not atomic.
>> E.g. here the MPlayer core thread could have updated m_iBufWritePos but
>> not yet finished setting m_fBufFull to 1.
>> query_buffer_len would then return 0 instead of m_nBufSize.
>
> Ah, I missed it.
>
>>> +    m_iBufWritePos = end_pos;
>>> +    if (m_iBufWritePos == m_iBufReadPos)
>>> +        m_fBufFull = 1;
>>>     
>>
>> I am not aware of any lock-less thread-safe fifo that allows a
>> completely full buffer, and I am fairly certain that such a thing does
>> not exist.
>> Unless you want to spend many hours on this I think you have only two
>> options to come up with correct code:
>> 1) copy a (hopefully) correct implementation from somewhere, which as
>> little changes as possible and avoiding to think too much (because that
>> is likely to introduce bugs).
>> 2) Use the proper thread-locking constructs.
>>
>>   
>
> Ok. I've implemented atomic-operation using DosEnterCritSec() and 
> DosExitCritSec(). That suspends all other threads and this resumes them 
> again.

Exactly the same issue exists unchanged since query_buffer_len is not in
a critical section.
If you use a critical section you should probably return to some simpler
method, e.g. a normal buffer instead of a FIFO and locking _all_ of
dart_audio_callback and play functions.
If you don't have experience with threading, either take a really,
really long time to check, double-check, recheck and re-recheck your
code or think of the absolutely most simple and stupid way to implement
it and put a lock around all of it. Otherwise we can play this game
forever...