[MPlayer-dev-eng] [PATCH] ao_dart

[KO Myung-Hun]

Hi/2.

KO Myung-Hun wrote:
> 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.

How about applying if no objections ?

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