[MPlayer-dev-eng] [PATCH] ao_dart

[KO Myung-Hun]

Hi/2.

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.

>> +        case AOCONTROL_GET_VOLUME :
>> +        {
>> +            ao_control_vol_t *vol = (ao_control_vol_t *)arg;
>> +
>> +            vol->left = vol->right = LOUSHORT(dartGetVolume());
>> +
>> +            return CONTROL_OK;
>> +        }
>> +
>> +        case AOCONTROL_SET_VOLUME:
>> +        {
>> +            int mid;
>> +            ao_control_vol_t *vol = (ao_control_vol_t *)arg;
>>     
>
> The casts are not necessary.
>
>   

Fixed.

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

>> +        {"noshare", OPT_ARG_BOOL, &fNoShare, NULL},
>>     
>
> Use "share" here, will give "share" and "noshare" as possible options,
> this will give you "noshare" and "nonoshare".
>
>   

Ok.

>> +        {"bufsize", OPT_ARG_INT, &nDartSamples, NULL},
>>     
>
> I don't think you want to allow negative numbers here, use the
> int_pos or int_non_neg test functions (if the later make bufsize = 0
> mean the default value.
>
>   

Ok.

>> +    n = (af_fmt2bits(format) >> 3) * channels;
>>     
>
> n is not really a good name.
>
>   

Renamed to nBytesPerSample.

>> +    ao_data.bps         = channels * rate * (af_fmt2bits(format) >> 3);
>>     
>
> You could reuse "n" here, too.
>
>   

Ok.

>> +    // 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. Some OS/2 PM(GUI) DLLs enable SIG_FPE but does not 
disable it again. In addition, SIG_FPE exception routine was removed 
from kernel at some point. It's a kind of the historical bug(?).

The reason why I didn't restore it, is that enabling SIG_FPE seems to be 
a bug as the above.

-- 
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/20090217/ddcf09d1/attachment.txt>