[MPlayer-dev-eng] [PATCH] ao_dart

[Reimar Döffinger]

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

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

>>> +    // 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 OS/2 PM(GUI) DLLs enable SIG_FPE but does not disable it 
> again.

So it is a bug workaround?

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

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

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

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.