Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)
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 - 1.96 +++ audio.c 31 Aug 2005 00:59:33 - @@ -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 = 0x1; +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 --- wine-20050830-cvs/dlls/winmm/winealsa/audio.c 2005-08-15 11:09:31.0 -0500 +++ wine-20050830-cvs-patch/dlls/winmm/winealsa/audio.c 2005-08-31 23:32:52.0 -0500 @@ -2299,7 +2299,7 @@ snd_pcm_access_taccess; snd_pcm_format_tformat = -1; unsigned intrate; -unsigned intbuffer_time = 50; +//unsigned intbuffer_time = 50; unsigned intperiod_time = 1; snd_pcm_uframes_t buffer_size; snd_pcm_uframes_t period_size;
Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)
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.
Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)
Robert Reif wrote:
Alex Villacís Lasso wrote:
The first test that fails with the ALSA driver is the so-called
reference tone, which, as far as I could see, is played with a
primary buffer, and no secondary buffer. The reference tone uses the
hardware position directly, which wraps around at a buffer size that
changes unexpectedly when switching playback formats. I have prepared
a patch that fixes this, by re-querying the buffer size in the case
of a primary buffer, and displaying a warning if it detects a buffer
size change. However, I don't know if the buffer size is supposed to
remain constant across buffer format changes. If it does, then the
patch would need to be modified to mark this as a TODO.
Fix the wine ALSA driver rather than the test unless you can prove
that the test fails on Windows.
This is a modified patch that marks the problematic buffer resize as a
todo_wine. Could somebody please test this patch and confirm that the
buffer size actually remains constant across playback format changes?
diff -urN wine-20050725-cvs/dlls/dsound/tests/ds3d8.c wine-20050725-cvs-patch/dlls/dsound/tests/ds3d8.c
--- wine-20050725-cvs/dlls/dsound/tests/ds3d8.c 2005-06-20 09:18:04.0 -0500
+++ wine-20050725-cvs-patch/dlls/dsound/tests/ds3d8.c 2005-08-27 22:18:01.0 -0500
@@ -254,6 +254,8 @@
ok(status==0,status=0x%lx instead of 0\n,status);
if (is_primary) {
+DWORD previous_buffer_size;
+
/* We must call SetCooperativeLevel to be allowed to call SetFormat */
/* DSOUND: Setting DirectSound cooperative level to DSSCL_PRIORITY */
rc=IDirectSound8_SetCooperativeLevel(dso,get_hwnd(),DSSCL_PRIORITY);
@@ -292,6 +294,31 @@
wfx.nChannels,wfx.nAvgBytesPerSec,wfx.nBlockAlign);
}
+/* Not all sound implementations guarantee that the buffer size will remain
+ unchanged after a successful SetFormat() call. For example, ALSA is known
+ to change buffer sizes when requested to change sound format. So the
+ buffer capabilities are re-queried in order to play the samples correctly.
+
+ TODO: is this a Windows DirectSound requirement? (bufsize constant across
+ format changes)
+ */
+previous_buffer_size = dsbcaps.dwBufferBytes;
+dsbcaps.dwSize=sizeof(dsbcaps);
+rc=IDirectSoundBuffer_GetCaps(dsbo,dsbcaps);
+ok(rc==DS_OK,IDirectSoundBuffer_GetCaps() failed: %s\n,
+ DXGetErrorString8(rc));
+if (rc==DS_OK winetest_debug 1) {
+trace(Caps (again): flags=0x%08lx size=%ld\n,dsbcaps.dwFlags,
+ dsbcaps.dwBufferBytes);
+}
+/* ALSA fails this test, so it is marked as a TODO */
+todo_wine
+{
+ok(previous_buffer_size == dsbcaps.dwBufferBytes,
+buffer size changed after SetFormat() - previous size is %lu, current size is %lu\n,
+previous_buffer_size, dsbcaps.dwBufferBytes);
+}
+
/* Set the CooperativeLevel back to normal */
/* DSOUND: Setting DirectSound cooperative level to DSSCL_NORMAL */
rc=IDirectSound8_SetCooperativeLevel(dso,get_hwnd(),DSSCL_NORMAL);
diff -urN wine-20050725-cvs/dlls/dsound/tests/ds3d.c wine-20050725-cvs-patch/dlls/dsound/tests/ds3d.c
--- wine-20050725-cvs/dlls/dsound/tests/ds3d.c 2005-07-15 13:36:52.0 -0500
+++ wine-20050725-cvs-patch/dlls/dsound/tests/ds3d.c 2005-08-27 22:17:55.0 -0500
@@ -362,6 +362,8 @@
ok(status==0,status=0x%lx instead of 0\n,status);
if (is_primary) {
+DWORD previous_buffer_size;
+
/* We must call SetCooperativeLevel to be allowed to call SetFormat */
/* DSOUND: Setting DirectSound cooperative level to DSSCL_PRIORITY */
rc=IDirectSound_SetCooperativeLevel(dso,get_hwnd(),DSSCL_PRIORITY);
@@ -400,6 +402,31 @@
wfx.nChannels,wfx.nAvgBytesPerSec,wfx.nBlockAlign);
}
+/* Not all sound implementations guarantee that the buffer size will remain
+ unchanged after a successful SetFormat() call. For example, ALSA is known
+ to change buffer sizes when requested to change sound format. So the
+ buffer capabilities are re-queried in order to play the samples correctly.
+
+ TODO: is this a Windows DirectSound requirement? (bufsize constant across
+ format changes)
+ */
+previous_buffer_size = dsbcaps.dwBufferBytes;
+dsbcaps.dwSize=sizeof(dsbcaps);
+rc=IDirectSoundBuffer_GetCaps(dsbo,dsbcaps);
+ok(rc==DS_OK,IDirectSoundBuffer_GetCaps() failed: %s\n,
+ DXGetErrorString8(rc));
+if (rc==DS_OK winetest_debug 1) {
+trace(Caps (again): flags=0x%08lx size=%ld\n,dsbcaps.dwFlags,
+ dsbcaps.dwBufferBytes);
+}
+/* ALSA fails this test, so it is marked as a TODO */
+
Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)
Alex Villacís Lasso wrote: Robert Reif wrote: Alex Villacís Lasso wrote: The first test that fails with the ALSA driver is the so-called reference tone, which, as far as I could see, is played with a primary buffer, and no secondary buffer. The reference tone uses the hardware position directly, which wraps around at a buffer size that changes unexpectedly when switching playback formats. I have prepared a patch that fixes this, by re-querying the buffer size in the case of a primary buffer, and displaying a warning if it detects a buffer size change. However, I don't know if the buffer size is supposed to remain constant across buffer format changes. If it does, then the patch would need to be modified to mark this as a TODO. Fix the wine ALSA driver rather than the test unless you can prove that the test fails on Windows. This is a modified patch that marks the problematic buffer resize as a todo_wine. Could somebody please test this patch and confirm that the buffer size actually remains constant across playback format changes? The problem is that the wine ALSA driver creates a fixed time buffer rather than a fixed size buffer. Please try your patched test on a real Windows 9X system and on a Windows 2k/xp system. Windows 2k up doesn't really have a hardware primary buffer because of kmixer but a Windows 9X vdx driver will communicate directly with the hardware. If the test fails on Windows, then we need to change the direct sound implementation to read the new buffer size. Otherwise we need to change the ALSA driver to always create a fixed buffer size regardless of the requested format (like OSS). We can fix the test and the direct sound dll to work around ALSA's current behavior but that won't help other apps that depend on the primary buffer size not changing if that's what Windows really does.
Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)
Robert Reif wrote:
Only the primary buffer supports hardware acceleration. The secondary
buffer(s) are implemented in software and mixed into the primary buffer.
The formats (mono/stereo, 8/16 bit samples, and sample rate) of the
primary and secondary buffers are totally independent and can be
anything.
One of the dsound tests keeps the primary buffer format constant and
iterates through all secondary buffer formats and another keeps the
secondary buffer format constant and iterates through all possible
primary formats.
This is probably what you are seeing. The secondary buffer format
has nothing to do with what is sent to the hardware.
The first test that fails with the ALSA driver is the so-called
reference tone, which, as far as I could see, is played with a primary
buffer, and no secondary buffer. The reference tone uses the hardware
position directly, which wraps around at a buffer size that changes
unexpectedly when switching playback formats. I have prepared a patch
that fixes this, by re-querying the buffer size in the case of a
primary buffer, and displaying a warning if it detects a buffer size
change. However, I don't know if the buffer size is supposed to remain
constant across buffer format changes. If it does, then the patch would
need to be modified to mark this as a TODO.
Changelog:
* Re-query buffer capabilities after format change, and display warning
if buffer size changed after format change. Fixes reference playback
with ALSA.
diff -uN wine-20050725-cvs/dlls/dsound/tests/ds3d8.c wine-20050725-cvs-patch/dlls/dsound/tests/ds3d8.c
--- wine-20050725-cvs/dlls/dsound/tests/ds3d8.c 2005-06-20 09:18:04.0 -0500
+++ wine-20050725-cvs-patch/dlls/dsound/tests/ds3d8.c 2005-08-13 11:35:55.0 -0500
@@ -254,6 +254,8 @@
ok(status==0,status=0x%lx instead of 0\n,status);
if (is_primary) {
+DWORD previous_buffer_size;
+
/* We must call SetCooperativeLevel to be allowed to call SetFormat */
/* DSOUND: Setting DirectSound cooperative level to DSSCL_PRIORITY */
rc=IDirectSound8_SetCooperativeLevel(dso,get_hwnd(),DSSCL_PRIORITY);
@@ -292,6 +294,29 @@
wfx.nChannels,wfx.nAvgBytesPerSec,wfx.nBlockAlign);
}
+/* Not all sound implementations guarantee that the buffer size will remain
+ unchanged after a successful SetFormat() call. For example, ALSA is known
+ to change buffer sizes when requested to change sound format. So the
+ buffer capabilities are re-queried in order to play the samples correctly.
+
+ TODO: is this a Windows DirectSound requirement? (bufsize constant across
+ format changes)
+ */
+previous_buffer_size = dsbcaps.dwBufferBytes;
+dsbcaps.dwSize=sizeof(dsbcaps);
+rc=IDirectSoundBuffer_GetCaps(dsbo,dsbcaps);
+ok(rc==DS_OK,IDirectSoundBuffer_GetCaps() failed: %s\n,
+ DXGetErrorString8(rc));
+if (rc==DS_OK winetest_debug 1) {
+trace(Caps (again): flags=0x%08lx size=%ld\n,dsbcaps.dwFlags,
+ dsbcaps.dwBufferBytes);
+}
+/* should the following check be marked as a TODO? */
+if (previous_buffer_size != dsbcaps.dwBufferBytes) {
+trace(buffer size changed after SetFormat() - previous size is %lu, current size is %lu\n,
+previous_buffer_size, dsbcaps.dwBufferBytes);
+}
+
/* Set the CooperativeLevel back to normal */
/* DSOUND: Setting DirectSound cooperative level to DSSCL_NORMAL */
rc=IDirectSound8_SetCooperativeLevel(dso,get_hwnd(),DSSCL_NORMAL);
diff -uN wine-20050725-cvs/dlls/dsound/tests/ds3d.c wine-20050725-cvs-patch/dlls/dsound/tests/ds3d.c
--- wine-20050725-cvs/dlls/dsound/tests/ds3d.c 2005-07-15 13:36:52.0 -0500
+++ wine-20050725-cvs-patch/dlls/dsound/tests/ds3d.c 2005-08-12 20:53:54.0 -0500
@@ -362,6 +362,8 @@
ok(status==0,status=0x%lx instead of 0\n,status);
if (is_primary) {
+DWORD previous_buffer_size;
+
/* We must call SetCooperativeLevel to be allowed to call SetFormat */
/* DSOUND: Setting DirectSound cooperative level to DSSCL_PRIORITY */
rc=IDirectSound_SetCooperativeLevel(dso,get_hwnd(),DSSCL_PRIORITY);
@@ -400,6 +402,29 @@
wfx.nChannels,wfx.nAvgBytesPerSec,wfx.nBlockAlign);
}
+/* Not all sound implementations guarantee that the buffer size will remain
+ unchanged after a successful SetFormat() call. For example, ALSA is known
+ to change buffer sizes when requested to change sound format. So the
+ buffer capabilities are re-queried in order to play the samples correctly.
+
+ TODO: is this a Windows DirectSound requirement? (bufsize constant across
+ format changes)
+ */
+previous_buffer_size = dsbcaps.dwBufferBytes;
+
Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)
Alex Villacís Lasso wrote: The first test that fails with the ALSA driver is the so-called reference tone, which, as far as I could see, is played with a primary buffer, and no secondary buffer. The reference tone uses the hardware position directly, which wraps around at a buffer size that changes unexpectedly when switching playback formats. I have prepared a patch that fixes this, by re-querying the buffer size in the case of a primary buffer, and displaying a warning if it detects a buffer size change. However, I don't know if the buffer size is supposed to remain constant across buffer format changes. If it does, then the patch would need to be modified to mark this as a TODO. Fix the wine ALSA driver rather than the test unless you can prove that the test fails on Windows.
Re: WINEALSA: comment on unexpected shrinking of mmap-buffer (resend)
Alex Villacís Lasso wrote: Alex Villacís Lasso wrote: The good news: the patch sort of works (in my setup, at least, with Fedora Core 4). All the games I have (Japanese RPGs) now have smooth sound, unless the CPU load is too high. The bad news: the patch does nothing to make the dsound tests pass in Wine (but they were already failing before the patch :-) In a previous post, I commented about DirectSound tests failing when ALSA is used with full hardware acceleration. Now I know the reason why. As far as I can glean from the code, DirectSound is supposed to report the application several properties of the sound system, including the size of the hardware buffer. This hardware buffer is queried by the snd_hw_params_get_buffer_size(), and is then converted from frame units to bytes. Then the application, or in this case, the test, expects the buffer size to remain constant for each and every hardware buffer created, regardless of requested format. Only this assertion fails in ALSA. For example, the capability query causes the following output to be shown: trace:wave:DSDB_CreateMMAP format=U8 frames=11025 channels=2 bits_per_sample=8 bits_per_frame=16 trace:wave:DSDB_CreateMMAP created mmap buffer of 11025 frames (22050 bytes) at 0x7c0fd100 This reported size (22050 bytes) remains constant as long as the request is for U8 format with 2 channels (although the mmap address jumps between three different values in my setup; this may or may not be relevant). However, as soon as the requested format changes, the trace shows the following: trace:wave:DSDB_CreateMMAP format=S16_LE frames=3763 channels=2 bits_per_sample=16 bits_per_frame=32 trace:wave:DSDB_CreateMMAP created mmap buffer of 3763 frames (15052 bytes) at 0x7c0fd6b8 There goes a fourth mmap address, but the truly interesting thing is that the buffer size also changes, and no longer matches the previously reported size of 22050 bytes. The test then goes and plays a 5-second sound with the (now incorrect) buffer size of 22050 bytes, not noting that the reported position is wrapping around at 15052 bytes: ds3d.c:431:Playing 5 second 440Hz tone at 11025x16x2 ... trace:dsound:DSOUND_PrimaryGetPosition (0x7fe03ab8,0x7fa6fa80,(nil)) trace:wave:IDsDriverBufferImpl_GetPosition hw_ptr=0x, playpos=0, writepos=-1, mmap_buflen_bytes=15052 trace:dsound:DSOUND_PrimaryGetPosition playpos = 0, writepos = 0 (0x7fe03ab8, time=1913) trace:dsound:PrimaryBufferImpl_GetCurrentPosition playpos = 0, writepos = 0 (0x7fe03ab8, time=1913) ds3d.c:230:buf size=22050 last_play_pos=0 play_pos=0 played=0 / 220500, fraction_played=0 ds3d.c:248:offset=15052 free=6998 written=15052 / 220500 ... ds3d.c:230:buf size=22050 last_play_pos=10296 play_pos=11700 played=11700 / 220500, fraction_played=1404 ds3d.c:248:offset=10296 free=1404 written=32346 / 220500 ds3d.c:230:buf size=22050 last_play_pos=11700 play_pos=13104 played=13104 / 220500, fraction_played=1404 ds3d.c:248:offset=11700 free=1404 written=33750 / 220500 ds3d.c:230:buf size=22050 last_play_pos=13104 play_pos=14508 played=14508 / 220500, fraction_played=1404 ds3d.c:248:offset=13104 free=1404 written=35154 / 220500 ds3d.c:230:buf size=22050 last_play_pos=14508 play_pos=860 played=22910 / 220500, fraction_played=8402 ds3d.c:248:offset=14508 free=8402 written=36558 / 220500 Please note that the play_pos wrapped from 14508 to 860 because it exceeded 15052 bytes, but the test assumes it wrapped around at 22050 bytes, so it miscalculates the fraction_played, advancing it at an abnormally fast rate. That is how the following result ensues: trace:dsound:DSOUND_PrimaryGetPosition playpos = 176, writepos = 0 (0x7fe03ab8, time=5366) trace:dsound:PrimaryBufferImpl_GetCurrentPosition playpos = 176, writepos = 0 (0x7fe03ab8, time=5366) ds3d.c:230:buf size=22050 last_play_pos=13824 play_pos=176 played=220676 / 220500, fraction_played=8402 ds3d.c:237:all the samples have been played, stopping... ds3d.c:279:stopping playback trace:dsound:PrimaryBufferImpl_Stop (0x7fe143b8) ds3d.c:628: Test failed: The sound played for 3453 ms instead of 5000 ms Many other tests with formats other than U8, 11025Hz fail for the same reason. Any other DirectSound application that reuses the buffer size as the test does will fail in the same way (music mixing up and prematurely stopping). However, I am to understand that this test passes on Windows, in all versions. Now that I have found the issue, I ask for help in suggesting a proper fix. I think that the ALSA implementation used to have a separate buffer from which samples were copied into the hardware buffer, but this implementation was scrapped for some reason (and I think, without re-running the ALSA tests). So I tried using snd_hw_params_set_buffer_size() - it fails to set the buffer size to the previously reported size. So I am still thinking about this. Any comments or suggestions for fixing this will be
