[MPlayer-dev-eng] [PATCH] ao_dart

[KO Myung-Hun]

Hi/2.

Reimar Döffinger wrote:
> On Mon, Feb 23, 2009 at 06:13:26PM +0900, KO Myung-Hun wrote:
>   
>> +static volatile int      m_nBufSize = 0;
>> +static volatile int      m_nBufLen  = 0;
>> +static volatile int      m_iBufPos  = 0;
>>     
>
> I would propose an ordinary buffer instead of a FIFO, i.e. without using
> m_iBufPos.
>
>   

Sorry, I misunderstood. ^^

>> +static ULONG APIENTRY dart_audio_callback(PVOID pCBData, PVOID pBuffer,
>> +                                          ULONG ulSize)
>> +{
>> +    int   nCopySize;
>> +    ULONG rc;
>> +
>> +    do {
>> +        rc = DosRequestMutexSem(m_hmtxBuf, SEM_INDEFINITE_WAIT);
>> +    } while (rc == ERROR_INTERRUPT);
>> +
>> +    nCopySize = ulSize < m_nBufLen ? ulSize : m_nBufLen;
>> +
>> +    if (m_iBufPos + nCopySize > m_nBufSize) {
>> +        int len = m_nBufSize - m_iBufPos;
>> +
>> +        fast_memcpy(pBuffer, m_audioBuf + m_iBufPos, len);
>> +        fast_memcpy((uint8_t *)pBuffer + len, m_audioBuf, nCopySize - len);
>> +    }
>> +    else
>> +        fast_memcpy(pBuffer, m_audioBuf + m_iBufPos, nCopySize);
>> +
>> +    m_iBufPos  = (m_iBufPos + nCopySize) % m_nBufSize;
>> +    m_nBufLen -= nCopySize;
>> +
>> +    memset((uint8_t *)pBuffer + nCopySize, DART.bSilence, ulSize - nCopySize);
>> +
>> +    DosReleaseMutexSem(m_hmtxBuf);
>>     
>
> (include libavutil/common.h to get FFMIN)
>
> (get mutex)
> nCopySize = FFMIN(ulSize, m_nBufLen);
> fast_memcpy(pBuffer, m_audioBuf, nCopySize);
> m_nBufLen -= nCopySize;
> if (m_nBufLen)
>   fast_memcpy(m_audioBuf, m_audioBuf + nCopySize, m_nBufLen);
> (release mutex, return nCopySize)
>
>   
>> +// return: how many bytes can be played without blocking
>> +static int get_space(void)
>> +{
>> +    return m_nBufSize - m_nBufLen;
>> +}
>>     
>
> Since you use locking anyway, it would be safer to lock here, too.
> And yes, reads need to be locked, too. The reason is that a Mutex does
> not create a atomic operation, it just makes sure that you will not be
> interrupted by code that is _also_ inside a mutex.
> So e.g. adding a mutex over only one piece of code will not have any
> effect at all, that would only work on the OS level when you disable
> interrupts instead of using a Mutex.
> Keep in mind that OS/2 probably only provides very simple mutexes, so
> you may not call get_space from play or dart_audio_callback after
> adding Mutex code here.
>
>   
>> +    do {
>> +        rc = DosRequestMutexSem(m_hmtxBuf, SEM_INDEFINITE_WAIT);
>> +    } while (rc == ERROR_INTERRUPT);
>> +
>> +    start_pos = ( m_iBufPos + m_nBufLen ) % m_nBufSize;
>> +
>> +    end_pos = (start_pos + len) % m_nBufSize;
>> +    if (end_pos <= start_pos) {
>> +        int len1 = m_nBufSize - start_pos;
>> +
>> +        if (end_pos > m_iBufPos)
>> +            end_pos = m_iBufPos;
>> +
>> +        fast_memcpy(m_audioBuf + start_pos, data, len1);
>> +        fast_memcpy(m_audioBuf, (uint8_t *)data + len1, end_pos);
>> +
>> +        len = len1 + end_pos;
>> +    }
>> +    else
>> +        fast_memcpy(m_audioBuf + start_pos, data, len);
>> +
>> +    m_nBufLen += len;
>> +
>> +    DosReleaseMutexSem(m_hmtxBuf);
>>     
>
> This is basically why I suggested simplifying and getting rid of the
> code. You at least miss a check on len, e.g. for len = m_nBufSize + 1
> you would happily write beyond the bounds of the buffer causing a buffer
> overflow. Since MPlayer checks the free space beforehand, this is
> unlikely to happen currently but still wrong.
>
>   

I also know it already. But I thought MPlayer would check free space 
before using get_space().

Anyway, It's wrong you're right.

> Suggested replacement code:
> (get mutex)
> len = FFMIN(len, m_nBufSize - m_nBufLen);
> fast_memcpy(m_audioBuf + m_nBufLen, data, len);
> m_nBufLen += len;
> (release mutex, return len)
>
>   
>> +// return: delay in seconds between first and last sample in buffer
>> +static float get_delay(void)
>> +{
>> +    return (float)m_nBufLen / (float)ao_data.bps;
>> +}
>>     
>
> Locking here will avoid surprises in the future, too.
>   

Ooops... There are too many lock/unlock. It seems to be not simple.

When I wrote these codes at first, I looked at ao_sdl.c and modified 
like this. But it seems to have many problems. And I've noticed that 
ao_sdl.c and ao_jack.c use the same ring-buffer codes. So I returned to 
the first.

Please point out any problems.

Finally, thanks for your very kindness to correct my codes.

-- 
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/20090224/5511a5ea/attachment.txt>