Re: DSOUND: tolerate inexact conversion between different frequencies

[Alex Villací­s Lasso]

Robert Reif wrote:
> Alex Villací­s Lasso wrote:
> > During the winealsa tests, I am frequently annoyed by the following: 
> > say, in playing a 11050Hz secondary buffer with an 8000Hz primary 
> > buffer, the number of samples are converted form secondary->primary 
> > and back. In the process, the number of consumed samples in the 
> > secondary buffer drifts by a few frames from the actual value, and 
> > this triggers the warning about "problem with underflow detection". 
> > This patch allows a tolerance of up to 4 bytes before issuing the 
> > message.
> >
> > Alex Villacís Lasso
>
> Does this patch help?  It should only fix the errors that are off by 
> one sample (up to 4 bytes).  It will not help with the errors greater 
> than 4 bytes.
>
> It works by keeping track of the round off error and factoring it back 
> in.
>
> ------------------------------------------------------------------------
>
> Index: dlls/dsound/buffer.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/dsound/buffer.c,v
> retrieving revision 1.51
> diff -p -u -r1.51 buffer.c
> --- dlls/dsound/buffer.c	1 Jun 2005 19:57:42 -0000	1.51
> +++ dlls/dsound/buffer.c	10 Sep 2005 02:20:21 -0000
> @@ -416,7 +416,9 @@ DWORD DSOUND_CalcPlayPosition(IDirectSou
>   pmix /= This->dsound->device->pwfx->nBlockAlign;
>   TRACE("primary back-samples=%ld\n",pmix);
>   /* adjust for our frequency */
> -	pmix = (pmix * This->freqAdjust) >> DSOUND_FREQSHIFT;
> +	pmix = pmix * This->freqAdjust;
> +	This->freqRem = pmix & ((1 << DSOUND_FREQSHIFT) - 1);
> +	pmix = pmix >> DSOUND_FREQSHIFT;
>   /* multiply by our own sample size */
>   pmix *= This->pwfx->nBlockAlign;
>   TRACE("this back-offset=%ld\n", pmix);
> @@ -426,6 +428,7 @@ DWORD DSOUND_CalcPlayPosition(IDirectSou
>   if (This->leadin && ((bplay < This->startpos) || (bplay > This->buf_mixpos))) {
>       /* seems we haven't started playing yet */
>       TRACE("this still in lead-in phase\n");
> +		This->freqRem = 0;
>       bplay = This->startpos;
>   }
>   /* return the result */
> @@ -1123,6 +1126,8 @@ HRESULT WINAPI IDirectSoundBufferImpl_Cr
>   dsb->buf_mixpos = 0;
>   dsb->state = STATE_STOPPED;
>
> +	dsb->freqAcc = 0;
> +	dsb->freqRem = 0;
>   dsb->freqAdjust = (dsb->freq << DSOUND_FREQSHIFT) /
>       ds->device->pwfx->nSamplesPerSec;
>   dsb->nAvgBytesPerSec = dsb->freq *
> Index: dlls/dsound/dsound_private.h
> ===================================================================
> RCS file: /home/wine/wine/dlls/dsound/dsound_private.h,v
> retrieving revision 1.31
> diff -p -u -r1.31 dsound_private.h
> --- dlls/dsound/dsound_private.h	12 Jul 2005 19:21:37 -0000	1.31
> +++ dlls/dsound/dsound_private.h	10 Sep 2005 02:20:22 -0000
> @@ -212,7 +212,7 @@ struct IDirectSoundBufferImpl
>     DSVOLUMEPAN                 volpan, cvolpan;
>     DSBUFFERDESC                dsbd;
>     /* used for frequency conversion (PerfectPitch) */
> -    ULONG                       freqAdjust, freqAcc;
> +    ULONG                       freqAdjust, freqAcc, freqRem;
>     /* used for intelligent (well, sort of) prebuffering */
>     DWORD                       probably_valid_to, last_playpos;
>     DWORD                       primary_mixpos, buf_mixpos;
> Index: dlls/dsound/mixer.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/dsound/mixer.c,v
> retrieving revision 1.40
> diff -p -u -r1.40 mixer.c
> --- dlls/dsound/mixer.c	3 Aug 2005 19:13:58 -0000	1.40
> +++ dlls/dsound/mixer.c	10 Sep 2005 02:20:22 -0000
> @@ -746,10 +746,19 @@ static DWORD DSOUND_MixOne(IDirectSoundB
>       	 * there's no telling what the app is thinking anyway */
>       } else {
>       	/* adjust for our frequency and our sample size */
> -			probably_valid_left = MulDiv(probably_valid_left,
> -						     1 << DSOUND_FREQSHIFT,
> -						     dsb->pwfx->nBlockAlign * dsb->freqAdjust) *
> -				              dsb->dsound->device->pwfx->nBlockAlign;
> +			ULONGLONG left = probably_valid_left / dsb->pwfx->nBlockAlign;
> +			left <<= DSOUND_FREQSHIFT;
> +			if (dsb->freqRem >= dsb->freqAcc)
> +				left += (dsb->freqRem - dsb->freqAcc);
> +			else {
> +				DWORD sub = dsb->freqAcc - dsb->freqRem;
> +				if (sub > left)
> +					left = 0;
> +				else
> +					left -= sub;
> +			}
> +			probably_valid_left = left / dsb->freqAdjust;
> +			probably_valid_left *= dsb->dsound->device->pwfx->nBlockAlign;
>       	/* check whether to clip mix_len */
>       	if (probably_valid_left < mixlen) {
>       		TRACE("clipping to probably_valid_left=%ld\n", probably_valid_left);

I tried this patch. Yes, it does remove the messages about buffer 
underruns, but it also introduces a delay in the playing of many tests, 
so they fail with the message "sample played for 1504 ms instead of 1000 
ms", with varying values for the actual value of the delay. However, all 
of these problems went away when I modified my modprobe.conf file:

alias eth0 via-rhine
alias snd-card-0 snd-via82xx
alias snd-card-1 snd-via82xx-modem
options snd-card-0 index=0
options snd-via82xx index=0 dxs_support=5
remove snd-via82xx { /usr/sbin/alsactl store 0 >/dev/null 2>&1 || : ; }; 
/sbin/modprobe -r --ignore-remove snd-via82xx
alias usb-controller ehci-hcd
alias usb-controller1 uhci-hcd
options parport_pc io=0x378,0xe800 irq=7,none
options lp parport=1
alias char-major-4 8250-pci
alias char-major-29 savagefb

I added the dxs_support=5 option to the snd-via82xx module, and now the 
tests play correctly. The odd thing is that, when the dxs_support 
option, I can no longer trigger the buffer underrun message, even 
without the patch. But since the patch did not make anything worse in 
any of the actual games I have (with or without dxs_support), I kept it 
applied.