Discussion:
[quake2] Choppy sound with default (non-SDL) client
Alexey Dokuchaev
2013-04-27 07:28:03 UTC
Permalink
Hi,

I've heard several reports over quite some time that default (non-SDL)
client executable gives choppy sound on FreeBSD (that is, OSS). SDL
version (quake2-sdl) works fine. Is this a known problem? What might be
the cause (and the fix)?

./danfe
Ozkan Sezer
2013-04-27 10:24:18 UTC
Permalink
Post by Alexey Dokuchaev
Hi,
I've heard several reports over quite some time that default (non-SDL)
client executable gives choppy sound on FreeBSD (that is, OSS). SDL
version (quake2-sdl) works fine. Is this a known problem? What might be
the cause (and the fix)?
./danfe
_______________________________________________
quake2 mailing list
quake2 at icculus.org
http://icculus.org/mailman/listinfo/quake2
It is because the SNDCTL_DSP_GETOSPACE ioctl is issued before setting
other important parameters: if it is move to the place just below the
mmaping it is fixed. See the patch I just cooked up (which contains a
few additional bits)

BTW, a maintained and updated version of icculus-quake2 is at:
http://www.yamagi.org/quake2/

Cheers.

--
O.S.

Index: src/linux/snd_linux.c
===================================================================
--- src/linux/snd_linux.c (revision 205)
+++ src/linux/snd_linux.c (working copy)
@@ -25,13 +25,9 @@
#include <sys/mman.h>
#include <sys/shm.h>
#include <sys/wait.h>
-#if defined(__FreeBSD__)
#include <sys/soundcard.h>
-#endif
-#if defined(__linux__)
-#include <linux/soundcard.h>
-#endif
#include <stdio.h>
+#include <unistd.h>

#include "../client/client.h"
#include "../client/snd_loc.h"
@@ -45,6 +41,7 @@
cvar_t *snddevice;

static int tryrates[] = { 11025, 22051, 44100, 48000, 8000 };
+static unsigned long mmaplen;

qboolean SNDDMA_Init(void)
{
@@ -53,6 +50,7 @@
int fmt;
int tmp;
int i;
+ unsigned long sz;
struct audio_buf_info info;
int caps;
extern uid_t saved_euid;
@@ -65,7 +63,7 @@
if (!snddevice)
{
sndbits = Cvar_Get("sndbits", "16", CVAR_ARCHIVE);
- sndspeed = Cvar_Get("sndspeed", "0", CVAR_ARCHIVE);
+ sndspeed = Cvar_Get("sndspeed", "44100", CVAR_ARCHIVE);
sndchannels = Cvar_Get("sndchannels", "2", CVAR_ARCHIVE);
snddevice = Cvar_Get("snddevice", "/dev/dsp", CVAR_ARCHIVE);
}
@@ -76,7 +74,7 @@
{
seteuid(saved_euid);

- audio_fd = open(snddevice->string, O_RDWR);
+ audio_fd = open(snddevice->string, O_RDWR|O_NONBLOCK);

if (audio_fd == -1)
{
@@ -115,28 +113,19 @@
return 0;
}

- if (ioctl(audio_fd, SNDCTL_DSP_GETOSPACE, &info)==-1)
- {
- perror("GETOSPACE");
- Com_Printf("SNDDMA_Init: GETOSPACE ioctl failed.\n");
- close(audio_fd);
- audio_fd = -1;
- return 0;
- }
-
// set sample bits & speed

dma.samplebits = (int)sndbits->value;
if (dma.samplebits != 16 && dma.samplebits != 8)
{
ioctl(audio_fd, SNDCTL_DSP_GETFMTS, &fmt);
- if (fmt & AFMT_S16_LE) dma.samplebits = 16;
+ if (fmt & AFMT_S16_NE) dma.samplebits = 16;
else if (fmt & AFMT_U8) dma.samplebits = 8;
}

if (dma.samplebits == 16)
{
- rc = AFMT_S16_LE;
+ rc = AFMT_S16_NE;
rc = ioctl(audio_fd, SNDCTL_DSP_SETFMT, &rc);
if (rc < 0)
{
@@ -211,15 +200,27 @@
return 0;
}

+ rc = ioctl(audio_fd, SNDCTL_DSP_GETOSPACE, &info);
+ if (rc < 0)
+ {
+ perror("GETOSPACE");
+ Com_Printf("SNDDMA_Init: GETOSPACE ioctl failed.\n");
+ close(audio_fd);
+ audio_fd = -1;
+ return 0;
+ }
+
dma.samples = info.fragstotal * info.fragsize / (dma.samplebits/8);
dma.submission_chunk = 1;

// memory map the dma buffer
-
+ mmaplen = info.fragstotal * info.fragsize;
+ sz = sysconf (_SC_PAGESIZE);
+ mmaplen += sz - 1;
+ mmaplen &= ~(sz - 1);
if (!dma.buffer)
- dma.buffer = (unsigned char *) mmap(NULL, info.fragstotal
- * info.fragsize, PROT_WRITE|PROT_READ, MAP_FILE|MAP_SHARED, audio_fd, 0);
- if (!dma.buffer || dma.buffer == MAP_FAILED)
+ dma.buffer = (unsigned char *) mmap(NULL, mmaplen,
PROT_WRITE|PROT_READ, MAP_FILE|MAP_SHARED, audio_fd, 0);
+ if (dma.buffer == MAP_FAILED)
{
perror(snddevice->string);
Com_Printf("SNDDMA_Init: Could not mmap %s.\n", snddevice->string);
@@ -236,6 +237,8 @@
{
perror(snddevice->string);
Com_Printf("SNDDMA_Init: Could not toggle. (1)\n");
+ munmap (dma.buffer, mmaplen);
+ dma.buffer=NULL;
close(audio_fd);
audio_fd = -1;
return 0;
@@ -246,6 +249,8 @@
{
perror(snddevice->string);
Com_Printf("SNDDMA_Init: Could not toggle. (2)\n");
+ munmap (dma.buffer, mmaplen);
+ dma.buffer=NULL;
close(audio_fd);
audio_fd = -1;
return 0;
@@ -267,6 +272,8 @@
{
perror(snddevice->string);
Com_Printf("SNDDMA_GetDMAPos: GETOPTR failed.\n");
+ munmap (dma.buffer, mmaplen);
+ dma.buffer=NULL;
close(audio_fd);
audio_fd = -1;
snd_inited = 0;
@@ -284,6 +291,8 @@
#if 0
if (snd_inited)
{
+ munmap (dma.buffer, mmaplen);
+ dma.buffer=NULL;
close(audio_fd);
audio_fd = -1;
snd_inited = 0;
Alexey Dokuchaev
2013-04-28 01:23:29 UTC
Permalink
Post by Ozkan Sezer
Post by Alexey Dokuchaev
I've heard several reports over quite some time that default (non-SDL)
client executable gives choppy sound on FreeBSD (that is, OSS). SDL
version (quake2-sdl) works fine. Is this a known problem? What might be
the cause (and the fix)?
It is because the SNDCTL_DSP_GETOSPACE ioctl is issued before setting
other important parameters: if it is move to the place just below the
mmaping it is fixed. See the patch I just cooked up (which contains a
few additional bits)
OK, I see. Yes, I've noticed similar patch for fteqw we also have in the
[FreeBSD] ports, but haven't give it a try yet.
Post by Ozkan Sezer
http://www.yamagi.org/quake2/
Cool, I'll add it to FreeBSD ports collection. I can instate you as
maintainer of it if you wish. :)

Thanks for the insight!

./danfe (maintainer of icculus-quake2 in FreeBSD)
Alexey Dokuchaev
2013-04-28 04:07:50 UTC
Permalink
Post by Ozkan Sezer
Post by Alexey Dokuchaev
I've heard several reports over quite some time that default (non-SDL)
client executable gives choppy sound on FreeBSD (that is, OSS).
It is because the SNDCTL_DSP_GETOSPACE ioctl is issued before setting
other important parameters: if it is move to the place just below the
mmaping it is fixed. See the patch I just cooked up (which contains a
few additional bits)
I confirm that moving SNDCTL_DSP_GETOSPACE ioctl() call lower does fix the
problem for me here on FreeBSD 8.3, thanks!

May I ask a few questions on the remaining bits of the patch?
Post by Ozkan Sezer
@@ -76,7 +74,7 @@
{
seteuid(saved_euid);
- audio_fd = open(snddevice->string, O_RDWR);
+ audio_fd = open(snddevice->string, O_RDWR|O_NONBLOCK);
What happens when opening file in blocking mode? I did not observe any
problems... Although I've seen this change in some other quake engine
source code.
Post by Ozkan Sezer
- if (fmt & AFMT_S16_LE) dma.samplebits = 16;
+ if (fmt & AFMT_S16_NE) dma.samplebits = 16;
else if (fmt & AFMT_U8) dma.samplebits = 8;
}
if (dma.samplebits == 16)
{
- rc = AFMT_S16_LE;
+ rc = AFMT_S16_NE;
Do I understand correctly that using AFMT_S16_LE would break on big-endian
machines? I.e., AFMT_S16_NE makes code CPU (endian) agnostic, ergo correct?
Post by Ozkan Sezer
+ mmaplen = info.fragstotal * info.fragsize;
+ sz = sysconf (_SC_PAGESIZE);
+ mmaplen += sz - 1;
+ mmaplen &= ~(sz - 1);
What benefits forcing page size aligned mmaplen brings us here?
Post by Ozkan Sezer
+ munmap (dma.buffer, mmaplen);
+ dma.buffer=NULL;
Fixing resource leaks are clear.

./danfe
Ozkan Sezer
2013-04-28 06:24:27 UTC
Permalink
Post by Alexey Dokuchaev
Post by Ozkan Sezer
http://www.yamagi.org/quake2/
Cool, I'll add it to FreeBSD ports collection. I can instate you as
maintainer of it if you wish. :)
Nope, I can't do it right now (I am more of a Linux guy. However Yamagi
himself is a FreeBSD guy, he might be interested in.)
Post by Alexey Dokuchaev
Post by Ozkan Sezer
Post by Alexey Dokuchaev
I've heard several reports over quite some time that default (non-SDL)
client executable gives choppy sound on FreeBSD (that is, OSS).
It is because the SNDCTL_DSP_GETOSPACE ioctl is issued before setting
other important parameters: if it is move to the place just below the
mmaping it is fixed. See the patch I just cooked up (which contains a
few additional bits)
I confirm that moving SNDCTL_DSP_GETOSPACE ioctl() call lower does fix the
problem for me here on FreeBSD 8.3, thanks!
My pleasure,
Post by Alexey Dokuchaev
May I ask a few questions on the remaining bits of the patch?
Post by Ozkan Sezer
@@ -76,7 +74,7 @@
{
seteuid(saved_euid);
- audio_fd = open(snddevice->string, O_RDWR);
+ audio_fd = open(snddevice->string, O_RDWR|O_NONBLOCK);
What happens when opening file in blocking mode? I did not observe any
problems... Although I've seen this change in some other quake engine
source code.
It has been a long time since I added that flag: it has been in my
uHexen2 for more than 7-8 years, and quakeforge does the same too.
I vaguely remember having some issues without it back at the time and
adding it helped, however it didn't change anything on my modern Linux
setup when testing the patch I sent you.
Post by Alexey Dokuchaev
Post by Ozkan Sezer
- if (fmt & AFMT_S16_LE) dma.samplebits = 16;
+ if (fmt & AFMT_S16_NE) dma.samplebits = 16;
else if (fmt & AFMT_U8) dma.samplebits = 8;
}
if (dma.samplebits == 16)
{
- rc = AFMT_S16_LE;
+ rc = AFMT_S16_NE;
Do I understand correctly that using AFMT_S16_LE would break on big-endian
machines? I.e., AFMT_S16_NE makes code CPU (endian) agnostic, ergo correct?
Exactly. The sound file loaders in the engine already converts the
data to host-endian, therefore the device must be open in host-endian
mode, too.
Post by Alexey Dokuchaev
Post by Ozkan Sezer
+ mmaplen = info.fragstotal * info.fragsize;
+ sz = sysconf (_SC_PAGESIZE);
+ mmaplen += sz - 1;
+ mmaplen &= ~(sz - 1);
What benefits forcing page size aligned mmaplen brings us here?
Depends on the arch you are running on. I remember inheriting that one
from quakeforge to fix some weirdness (or was it a crash?) on a foreign
arch, but it was a very long time ago..
Post by Alexey Dokuchaev
Post by Ozkan Sezer
+ munmap (dma.buffer, mmaplen);
+ dma.buffer=NULL;
Fixing resource leaks are clear.
Yup,
Post by Alexey Dokuchaev
./danfe
--
O.S.

Loading...