Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)

[Alex Villací­s Lasso]

Robert Reif wrote:
> Alex Villací­s Lasso wrote:
> > Robert Reif wrote:
> > > Fix the wine ALSA driver rather than the test unless you can prove 
> > > that the test fails on Windows.
> >
> > Hers is a real quick hack that fixes the problem.  This should give 
> > you someplace to start.
>
> ------------------------------------------------------------------------
>
> ? audio.c.test
> ? audio.diff
> Index: audio.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/winmm/winealsa/audio.c,v
> retrieving revision 1.96
> diff -p -u -r1.96 audio.c
> --- audio.c	12 Aug 2005 11:17:54 -0000	1.96
> +++ audio.c	31 Aug 2005 00:59:33 -0000
> @@ -2527,14 +2527,16 @@ static DWORD wodOpen(WORD wDevID, LPWAVE
>           getFormat(wwo->format.Format.wFormatTag));
>
> dir=0; 
> -    EXIT_ON_ERROR( snd_pcm_hw_params_set_buffer_time_near(pcm, hw_params, &buffer_time, &dir), MMSYSERR_INVALPARAM, "unable to set buffer time");
> +//    EXIT_ON_ERROR( snd_pcm_hw_params_set_buffer_time_near(pcm, hw_params, &buffer_time, &dir), MMSYSERR_INVALPARAM, "unable to set buffer time");
> +    buffer_size = 0x10000;
> +    EXIT_ON_ERROR( snd_pcm_hw_params_set_buffer_size_near(pcm, hw_params, &buffer_size), MMSYSERR_INVALPARAM, "unable to set buffer size");
>     dir=0; 
>     EXIT_ON_ERROR( snd_pcm_hw_params_set_period_time_near(pcm, hw_params, &period_time, &dir), MMSYSERR_INVALPARAM, "unable to set period time");
>
> EXIT_ON_ERROR( snd_pcm_hw_params(pcm, hw_params), MMSYSERR_INVALPARAM, "unable to set hw params for playback");
>
> err = snd_pcm_hw_params_get_period_size(hw_params, &period_size, &dir);
> -    err = snd_pcm_hw_params_get_buffer_size(hw_params, &buffer_size);
> +//    err = snd_pcm_hw_params_get_buffer_size(hw_params, &buffer_size);
>
> snd_pcm_sw_params_current(pcm, sw_params);
>     EXIT_ON_ERROR( snd_pcm_sw_params_set_start_threshold(pcm, sw_params, dwFlags & WAVE_DIRECTSOUND ? INT_MAX : 1 ), MMSYSERR_ERROR, "unable to set start threshold");

From what I could see from the patch, it tries to set a buffer size 
instead of a buffer time. However, the patch did not solve the problems 
as it is.

First, the patch supplies a constant value for 
snd_pcm_hw_params_set_buffer_size_near(). However, the ALSA 
documentation states that the parameter for buffer_size for this 
function is given in *frames*, not bytes. Since a frame (nChannels * 
bitsPerSample / 8) can be from 1 to 4 bytes depending on the requested 
format, this patch only fixes the frame length, not the byte length. So 
the attached patch divides the required buffer size (in bytes) by the 
frame size to arrive to the buffer size in frames.

Second, even after the division, the ALSA API is unable to honor the 
exact value required (at least in my hardware). For most formats the 
upper limit on the byte size is 30104 bytes, but for the reference tone 
it changes to 15052 bytes. So it triggers the buffer resize all over 
again. The attached patch hardcodes the value of 15052 bytes (0x3acc) as 
the required buffer size, and then the ALSA API doesn't resize the 
buffer. But obviously, this is a machine-dependent value that might be 
different in everybody else's machine. Any other value greater than 
15052 triggers the buffer resize, and other values below it cause awful 
buzzings and silences on the games I have tested. Even the chosen value 
of 15052 bytes, though it works in the tested games, causes buzzings 
instead of the proper tone on some 8000Hz and 11050Hz tests (the tests 
where the playtime is around 100-300 ms instead of 1000 ms in the 
attached trace).

Third (but this is no longer a winealsa issue), I still get the critical 
section timeouts on invalid threads in aprox. 1 in 5 runs of the 
interactive dsound test, even on an idle CPU. This proves that the 
timeout bug is not because of CPU load, and should be investigated further.

All of this leads me to think that the winealsa driver should give up on 
trying to provide access to the direct mmap buffer, and revert to 
providing a fixed-size buffer that is then used to feed the actual ALSA 
buffer. This would a) hide the buffer resizes from the tests and the 
games, and b) remove the need of the undocumented hardware pointer 
function. In fact, I do not remember what was the rationale behind 
switching from the old model to the current one. In addition, I have 
tested a few games

Any comments on this are welcome.

Alex Villacís Lasso

WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)

[Robert Reif]

Alex Villací­s Lasso wrote:
> Robert Reif wrote:
> > Alex Villací­s Lasso wrote:
> > > Robert Reif wrote:
> > > > Fix the wine ALSA driver rather than the test unless you can prove 
> > > > that the test fails on Windows.
> > >
> > > Hers is a real quick hack that fixes the problem.  This should give 
> > > you someplace to start.

Direct sound expects the size of the buffer in bytes to remain
constant regardless of the format and the intent of the patch was
to show where the changes need to be made.  That's the bug that
need fixing.

I don't have the time to fix this bug properly in the near future so
all I could do was give a hint.