[MPlayer-dev-eng] [PATCH] ao_dart

[KO Myung-Hun]

Hi/2.

Reimar Döffinger wrote:
> On Tue, Feb 17, 2009 at 11:58:27AM +0900, KO Myung-Hun wrote:
>   
>> Reimar Döffinger wrote:
>>     
>>> On Mon, Feb 16, 2009 at 10:17:01PM +0900, KO Myung-Hun wrote:
>>>   
>>>       
>>>> +static volatile int m_nBufSize = 0;
>>>> +static volatile int m_nBufLen = 0;
>>>> +static volatile int m_iBufPos = 0;
>>>>         
>>> You are writing m_nBufLen from both threads, thus your code is not
>>> thread-safe.
>>> E.g. ao_jack and libavutil/fifo.c should be thread-safe implementations
>>> for this one reader, one writer case (or you can use locking), ao_jack
>>> is a bit better optimized for this specific use.
>>> They also do not need modulus-operation which you use and can be quite 
>>> slow.
>>>       
>> Ok. I introduced m_iBufReadPos, m_iBufWritePos and m_fBufFull instead of 
>> m_nBufLen and m_iBufPos.
>>     
>
> That still has other issues. I know of only one lock-free thread-safe
> buffer type (and even that assumes atomic reads/writes, so it would
> break on e.g. a 16 bit microcontroller), and dropping the volatile is
> not such a good idea (yes, I know I referred you to fifo.c which does
> not use it, but using it is a bit safe with minimal effort).
>   

Ok. I restored it.

> Note that I assume that OS/2 implements non-cooperative scheduling, i.e.
> a thread switch can happen at any point of the code, and that
> dart_audio_callback executes in a different thread than the MPlayer
> core.
>
>   

Exactly.

>>>> +    opt_t subopts[] = {
>>>>     
>>>>         
>>> const
>>>       
>> Ok. But it generates the following warning.
>>
>> -----
>> libao2/ao_dart.c: In function 'init':
>> libao2/ao_dart.c:169: warning: passing argument 2 of 'subopt_parse' 
>> discards qualifiers from pointer target type
>> -----
>>     
>
> Oh, I am sorry. The const is indeed wrong, I was thinking about fixing
> it but never did it.
>
>   

No problem, removed it. ^^

>>>> +    // mask off all floating-point exceptions
>>>> +    _control87(MCW_EM, MCW_EM);
>>>>         
>>> Hm. Quite ugly, and you don't restore them in uninit.
>>> Also explaining why they are needed would be good (performance reasons,
>>> crash, and is it a bug in DART or a documented requirement - and no,
>>> no need to research, just write what you know already).
>>>       
>> Why ugly ? Anyway it's a way to avoid SIG_FPE, which is well-known problem 
>> on OS/2.
>>     
>
> Which does not explain what exactly this code is doing in an ao
> (changing that kind of thing is not commonly expected from an ao).
> There is also the obvious questions what causes a SIG_FPE, correctly
> written code should not cause any.
>
>   

Some AAC encoded movie causes SIG_FPE(underflow) in libfaad2.

If you want, I would provide it with you. It's about 380MB.

>> Some OS/2 PM(GUI) DLLs enable SIG_FPE but does not disable it 
>> again.
>>     
>
> So it is a bug workaround?
>
>   

I think so.

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

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

> But since I was promised that I will not have to maintain OS/2 code, I
> will not object to such self-contained parts to be applied as-is,
> I just want to be sure anyone who cares knows the code is not correct,
> even if hardly anyone will see it fail.
>   

No problem. I also want codes everyone can agree with.

-- 
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/20090218/666da43c/attachment.txt>