[MPlayer-dev-eng] [PATCH] ao_dart

[KO Myung-Hun]

Hi/2.

Reimar Döffinger wrote:
> 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
>
>   

Added.

>>>> +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.
>   

Read-only parts also need 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.
>   

Ok. This seems to be simple.

> 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...
>   

Oh... I want to avoid that situation even though It's possible I started 
that cycle already. T.T

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dart.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090223/f1b390d6/attachment.txt>