[MPlayer-dev-eng] [PATCH] ao_dart

[Reimar Döffinger]

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.

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

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.