Subject: kern/3305: Silience handling is wrong in audio driver
To: None <gnats-bugs@gnats.netbsd.org>
From: Lennart Augustsson <augustss@cs.chalmers.se>
List: netbsd-bugs
Date: 03/08/1997 13:01:45
>Number: 3305
>Category: kern
>Synopsis: Silience handling is wrong in audio driver
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: kern-bug-people (Kernel Bug People)
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sat Mar 8 04:20:02 1997
>Last-Modified:
>Originator: Lennart Augustsson
>Organization:
Department of Computing Science, Chalmers University
>Release: NetBSD-current 97-03-07
>Environment:
System: NetBSD calvin.cs.chalmers.se 1.2C NetBSD 1.2C (CALVIN) #112: Sat Mar 8 00:06:18 MET 1997 augustss@calvin.cs.chalmers.se:/usr/src/sys/arch/i386/compile/CALVIN i386
>Description:
When the audio driver does not recieve samples to play
at a fast enough pace it starts playing silence. The
silence is taken from a block of silence that it keeps
around. The handling of this silence buffer is totally
bogus. Here are the reasons:
1. The (pointer to the) silence buffer is a global variable.
If you happen to have more than one sound card installed and
they use different block lengths and/or sound encodings this will
break. This buffer should be local to each hardware device.
2. The silence buffer is filled with the same byte in every
position. This is correct for (most?) encodings that use
8 bits per sample. It could be very wrong for a 16 bit per
sample encoding, but happens to work for the only one we have
got right now. Anyway, the filling of the silence buffer
should not assume 8 bit per sample.
3. To get the silence byte the hardware independant layer calls
the hardware driver (through the get_silence function).
This is totally unnecessary. The encoding of silence is
not hardware dependant. It depends only on the encoding
mode, which is readily available to the hardware independant
layer. (All the different get_silence in the different
hardware audio drivers do essentially the same thing.)
>How-To-Repeat:
Try installing two sound cards. Or read the source.
>Fix:
The patch below fixes all of these problems.
Of course I've not been able to verify it on all platform, but
I think it's right. After all, it moves functionality to
the MI layer.
diff -c -r sys.old/arch/arm32/mainbus/lmcaudio.c sys/arch/arm32/mainbus/lmcaudio.c
*** sys.old/arch/arm32/mainbus/lmcaudio.c Sat Nov 23 13:16:50 1996
--- sys/arch/arm32/mainbus/lmcaudio.c Fri Mar 7 23:07:24 1997
***************
*** 355,361 ****
int lmcaudio_set_in_port __P((void *, int));
int lmcaudio_get_in_port __P((void *));
int lmcaudio_commit_settings __P((void *));
- u_int lmcaudio_get_silence __P((int));
void lmcaudio_sw_encode __P((void *, int, u_char *, int));
void lmcaudio_sw_decode __P((void *, int, u_char *, int));
int lmcaudio_start_output __P((void *, void *, int, void (*)(), void *));
--- 355,360 ----
***************
*** 556,578 ****
return(0);
}
- u_int
- lmcaudio_get_silence(enc)
- int enc;
- {
- #define ULAW_SILENCE 0x7f
- #define LINEAR_SILENCE 0
- u_int auzero = 0;
-
- switch (enc) {
- case AUDIO_ENCODING_LINEAR:
- default:
- auzero = LINEAR_SILENCE;
- break;
- }
- return(auzero);
- }
-
void
lmcaudio_sw_encode(addr, e, p, cc)
void *addr;
--- 555,560 ----
***************
*** 767,773 ****
lmcaudio_set_in_port,
lmcaudio_get_in_port,
lmcaudio_commit_settings,
- lmcaudio_get_silence,
lmcaudio_sw_encode,
lmcaudio_sw_decode,
lmcaudio_start_output,
--- 749,754 ----
diff -c -r sys.old/arch/arm32/mainbus/vidcaudio.c sys/arch/arm32/mainbus/vidcaudio.c
*** sys.old/arch/arm32/mainbus/vidcaudio.c Sat Nov 23 13:17:00 1996
--- sys/arch/arm32/mainbus/vidcaudio.c Fri Mar 7 23:07:44 1997
***************
*** 315,321 ****
int vidcaudio_set_in_port __P((void *, int));
int vidcaudio_get_in_port __P((void *));
int vidcaudio_commit_settings __P((void *));
- u_int vidcaudio_get_silence __P((int));
void vidcaudio_sw_encode __P((void *, int, u_char *, int));
void vidcaudio_sw_decode __P((void *, int, u_char *, int));
int vidcaudio_start_output __P((void *, void *, int, void (*)(), void *));
--- 315,320 ----
***************
*** 474,492 ****
return 0;
}
- u_int vidcaudio_get_silence ( int enc )
- {
- switch (enc)
- {
- case AUDIO_ENCODING_ULAW:
- return 0x7f;
-
- default:
- return 0;
- }
- return 0;
- }
-
void vidcaudio_sw_encode ( void *addr, int e, u_char *p, int cc )
{
#ifdef DEBUG
--- 473,478 ----
***************
*** 633,639 ****
vidcaudio_set_in_port,
vidcaudio_get_in_port,
vidcaudio_commit_settings,
- vidcaudio_get_silence,
vidcaudio_sw_encode,
vidcaudio_sw_decode,
vidcaudio_start_output,
--- 619,624 ----
diff -c -r sys.old/arch/sparc/dev/amd7930.c sys/arch/sparc/dev/amd7930.c
*** sys.old/arch/sparc/dev/amd7930.c Wed Dec 11 13:19:07 1996
--- sys/arch/sparc/dev/amd7930.c Sat Mar 8 00:47:00 1997
***************
*** 222,228 ****
int amd7930_set_in_port __P((void *, int));
int amd7930_get_in_port __P((void *));
int amd7930_commit_settings __P((void *));
- u_int amd7930_get_silence __P((int));
int amd7930_start_output __P((void *, void *, int, void (*)(void *),
void *));
int amd7930_start_input __P((void *, void *, int, void (*)(void *),
--- 222,227 ----
***************
*** 259,265 ****
amd7930_set_in_port,
amd7930_get_in_port,
amd7930_commit_settings,
- amd7930_get_silence,
NULL,
NULL,
amd7930_start_output,
--- 258,263 ----
***************
*** 624,636 ****
splx(s);
return(0);
- }
-
- u_int
- amd7930_get_silence(enc)
- int enc;
- {
- return(0x7f);
}
int
--- 622,627 ----
diff -c -r sys.old/dev/audio.c sys/dev/audio.c
*** sys.old/dev/audio.c Sat Jan 18 13:19:59 1997
--- sys/dev/audio.c Sat Mar 8 12:58:04 1997
***************
*** 116,124 ****
int audio_backlog = AUDIO_BACKLOG;
int audio_blocksize;
- /* A block of silence */
- char *auzero_block;
-
struct audio_softc *audio_softc[NAUDIO];
int audiosetinfo __P((struct audio_softc *, struct audio_info *));
--- 116,121 ----
***************
*** 145,151 ****
void audio_rpint __P((void *));
int audio_calc_blksize __P((struct audio_softc *));
! void audio_silence_fill __P((struct audio_softc *, u_char *, int));
int audio_silence_copyout __P((struct audio_softc *, int, struct uio *));
void audio_alloc_auzero __P((struct audio_softc *, int));
--- 142,148 ----
void audio_rpint __P((void *));
int audio_calc_blksize __P((struct audio_softc *));
! void audio_fill_silence __P((struct audio_softc *, int, u_char *, int));
int audio_silence_copyout __P((struct audio_softc *, int, struct uio *));
void audio_alloc_auzero __P((struct audio_softc *, int));
***************
*** 226,232 ****
hwp->set_in_port == 0 ||
hwp->get_in_port == 0 ||
hwp->commit_settings == 0 ||
- hwp->get_silence == 0 ||
hwp->start_output == 0 ||
hwp->start_input == 0 ||
hwp->halt_output == 0 ||
--- 223,228 ----
***************
*** 842,873 ****
}
void
! audio_silence_fill(sc, p, n)
struct audio_softc *sc;
u_char *p;
int n;
{
- struct audio_hw_if *hw = sc->hw_if;
u_int auzero;
!
! auzero = hw->get_silence(sc->sc_pencoding);
!
! while (--n >= 0)
! *p++ = auzero;
}
int
audio_silence_copyout(sc, n, uio)
struct audio_softc *sc;
int n;
struct uio *uio;
{
- struct audio_hw_if *hw = sc->hw_if;
struct iovec *iov;
int error = 0;
! u_int auzero;
! auzero = hw->get_silence(sc->sc_rencoding);
while (n > 0 && uio->uio_resid) {
iov = uio->uio_iov;
--- 838,884 ----
}
void
! audio_fill_silence(sc, mode, p, n)
struct audio_softc *sc;
+ int mode;
u_char *p;
int n;
{
u_int auzero;
! u_char *q;
!
! switch (mode == AUMODE_PLAY ? sc->sc_pencoding : sc->sc_rencoding) {
! case AUDIO_ENCODING_ULAW:
! auzero = 0x7f;
! break;
! case AUDIO_ENCODING_ALAW:
! auzero = 0x55;
! break;
! case AUDIO_ENCODING_ADPCM: /* is this right XXX */
! case AUDIO_ENCODING_PCM8:
! case AUDIO_ENCODING_PCM16:
! default:
! auzero = 0; /* fortunately this works for both 8 and 16 bits */
! break;
! }
! q = p;
! while(--n >= 0)
! *q++ = auzero;
}
+ #define NSILENCE 16 /* An arbitrary even constant >= 2 */
int
audio_silence_copyout(sc, n, uio)
struct audio_softc *sc;
int n;
struct uio *uio;
{
struct iovec *iov;
int error = 0;
! u_char zerobuf[NSILENCE];
! int k;
! audio_fill_silence(sc, AUMODE_RECORD, zerobuf, NSILENCE);
while (n > 0 && uio->uio_resid) {
iov = uio->uio_iov;
***************
*** 876,897 ****
uio->uio_iovcnt--;
continue;
}
switch (uio->uio_segflg) {
case UIO_USERSPACE:
! error = copyout(&auzero, iov->iov_base, 1);
if (error)
return (error);
break;
case UIO_SYSSPACE:
! bcopy(&auzero, iov->iov_base, 1);
break;
}
! iov->iov_base++;
! iov->iov_len--;
! uio->uio_resid--;
! uio->uio_offset++;
! n--;
}
return (error);
}
--- 887,909 ----
uio->uio_iovcnt--;
continue;
}
+ k = min(min(n, iov->iov_len), NSILENCE);
switch (uio->uio_segflg) {
case UIO_USERSPACE:
! error = copyout(zerobuf, iov->iov_base, k);
if (error)
return (error);
break;
case UIO_SYSSPACE:
! bcopy(zerobuf, iov->iov_base, k);
break;
}
! iov->iov_base += k;
! iov->iov_len -= k;
! uio->uio_resid -= k;
! uio->uio_offset += k;
! n -= k;
}
return (error);
}
***************
*** 902,927 ****
int bs;
{
struct audio_hw_if *hw = sc->hw_if;
- u_char *p, silence;
- int i;
-
- if (auzero_block)
- free(auzero_block, M_DEVBUF);
! auzero_block = malloc(bs, M_DEVBUF, M_WAITOK);
#ifdef DIAGNOSTIC
! if (auzero_block == 0) {
panic("audio_alloc_auzero: malloc auzero_block failed");
}
#endif
! silence = hw->get_silence(sc->sc_pencoding);
!
! p = auzero_block;
! for (i = 0; i < bs; i++)
! *p++ = silence;
!
if (hw->sw_encode)
! hw->sw_encode(sc->hw_hdl, sc->sc_pencoding, auzero_block, bs);
}
--- 914,932 ----
int bs;
{
struct audio_hw_if *hw = sc->hw_if;
! if (sc->auzero_block)
! free(sc->auzero_block, M_DEVBUF);
!
! sc->auzero_block = malloc(bs, M_DEVBUF, M_WAITOK);
#ifdef DIAGNOSTIC
! if (sc->auzero_block == 0) {
panic("audio_alloc_auzero: malloc auzero_block failed");
}
#endif
! audio_fill_silence(sc, AUMODE_PLAY, sc->auzero_block, bs);
if (hw->sw_encode)
! hw->sw_encode(sc->hw_hdl, sc->sc_pencoding, sc->auzero_block, bs);
}
***************
*** 987,993 ****
cb->nblk = sc->sc_backlog;
cb->tp = cb->hp + sc->sc_backlog * blocksize;
splx(s);
! audio_silence_fill(sc, cb->hp, sc->sc_backlog * blocksize);
}
#endif
/* Calculate sample number of first sample in block we write */
--- 992,998 ----
cb->nblk = sc->sc_backlog;
cb->tp = cb->hp + sc->sc_backlog * blocksize;
splx(s);
! audio_fill_silence(sc, AUMODE_PLAY, cb->hp, sc->sc_backlog * blocksize);
}
#endif
/* Calculate sample number of first sample in block we write */
***************
*** 1024,1030 ****
/* fill with audio silence */
tp += cc;
cc = blocksize - cc;
! audio_silence_fill(sc, tp, cc);
DPRINTF(("audio_write: auzero 0x%x %d 0x%x\n",
tp, cc, *(int *)tp));
tp += cc;
--- 1029,1035 ----
/* fill with audio silence */
tp += cc;
cc = blocksize - cc;
! audio_fill_silence(sc, AUMODE_PLAY, tp, cc);
DPRINTF(("audio_write: auzero 0x%x %d 0x%x\n",
tp, cc, *(int *)tp));
tp += cc;
***************
*** 1345,1355 ****
cb->cb_drops++;
#ifdef AUDIO_DEBUG
if (audiodebug > 1)
! Dprintf("audio_pint: drops=%d auzero %d 0x%x\n", cb->cb_drops, cc, *(int *)auzero_block);
#endif
psilence:
error = hw->start_output(sc->hw_hdl,
! auzero_block, cc,
audio_pint, (void *)sc);
if (error) {
DPRINTF(("audio_pint zero failed: %d\n", error));
--- 1350,1360 ----
cb->cb_drops++;
#ifdef AUDIO_DEBUG
if (audiodebug > 1)
! Dprintf("audio_pint: drops=%d auzero %d 0x%x\n", cb->cb_drops, cc, *(int *)sc->auzero_block);
#endif
psilence:
error = hw->start_output(sc->hw_hdl,
! sc->auzero_block, cc,
audio_pint, (void *)sc);
if (error) {
DPRINTF(("audio_pint zero failed: %d\n", error));
diff -c -r sys.old/dev/audio_if.h sys/dev/audio_if.h
*** sys.old/dev/audio_if.h Fri Mar 8 13:25:43 1996
--- sys/dev/audio_if.h Fri Mar 7 23:41:05 1997
***************
*** 85,93 ****
*/
int (*commit_settings)__P((void *));
- /* Return silence value for encoding */
- u_int (*get_silence)__P((int));
-
/* Software en/decode functions, set if SW coding required by HW */
void (*sw_encode)__P((void *, int, u_char *, int));
void (*sw_decode)__P((void *, int, u_char *, int));
--- 85,90 ----
diff -c -r sys.old/dev/audiovar.h sys/dev/audiovar.h
*** sys.old/dev/audiovar.h Tue Feb 20 13:39:49 1996
--- sys/dev/audiovar.h Fri Mar 7 23:50:58 1997
***************
*** 102,107 ****
--- 102,109 ----
/* Ring buffers, separate for record and play. */
struct audio_buffer rr; /* Record ring */
struct audio_buffer pr; /* Play ring */
+
+ u_char *auzero_block; /* a block of silence */
u_char sc_rbus; /* input dma in progress */
u_char sc_pbus; /* output dma in progress */
diff -c -r sys.old/dev/isa/ad1848.c sys/dev/isa/ad1848.c
*** sys.old/dev/isa/ad1848.c Thu Dec 5 13:18:56 1996
--- sys/dev/isa/ad1848.c Fri Mar 7 23:12:13 1997
***************
*** 1142,1174 ****
return(blk); /* Anything goes :-) */
}
- u_int
- ad1848_get_silence(enc)
- int enc;
- {
- #define ULAW_SILENCE 0x7f
- #define ALAW_SILENCE 0x55
- #define LINEAR_SILENCE 0
- u_int auzero;
-
- switch (enc) {
- case AUDIO_ENCODING_ULAW:
- auzero = ULAW_SILENCE;
- break;
- case AUDIO_ENCODING_ALAW:
- auzero = ALAW_SILENCE;
- break;
- case AUDIO_ENCODING_PCM8:
- case AUDIO_ENCODING_PCM16:
- default:
- auzero = LINEAR_SILENCE;
- break;
- }
-
- return(auzero);
- }
-
-
int
ad1848_open(sc, dev, flags)
struct ad1848_softc *sc;
--- 1142,1147 ----
diff -c -r sys.old/dev/isa/ad1848var.h sys/dev/isa/ad1848var.h
*** sys.old/dev/isa/ad1848var.h Fri Mar 7 23:09:54 1997
--- sys/dev/isa/ad1848var.h Fri Mar 7 23:12:27 1997
***************
*** 129,136 ****
int ad1848_commit_settings __P((void *));
- u_int ad1848_get_silence __P((int));
-
int ad1848_halt_in_dma __P((void *));
int ad1848_halt_out_dma __P((void *));
int ad1848_cont_in_dma __P((void *));
--- 129,134 ----
diff -c -r sys.old/dev/isa/gus.c sys/dev/isa/gus.c
*** sys.old/dev/isa/gus.c Mon Oct 14 18:35:25 1996
--- sys/dev/isa/gus.c Fri Mar 7 23:14:45 1997
***************
*** 641,648 ****
gus_commit_settings,
- ad1848_get_silence,
-
gus_expand,
mulaw_compress,
--- 641,646 ----
***************
*** 3023,3030 ****
gusmax_get_in_port,
gusmax_commit_settings,
-
- ad1848_get_silence,
gusmax_expand, /* XXX use codec */
mulaw_compress,
--- 3021,3026 ----
diff -c -r sys.old/dev/isa/pas.c sys/dev/isa/pas.c
*** sys.old/dev/isa/pas.c Tue Nov 12 13:19:13 1996
--- sys/dev/isa/pas.c Fri Mar 7 23:12:51 1997
***************
*** 122,128 ****
sbdsp_set_in_port,
sbdsp_get_in_port,
sbdsp_commit_settings,
- sbdsp_get_silence,
mulaw_expand,
mulaw_compress,
sbdsp_dma_output,
--- 122,127 ----
diff -c -r sys.old/dev/isa/ps.c sys/dev/isa/ps.c
*** sys.old/dev/isa/ps.c Tue Oct 29 00:11:39 1996
--- sys/dev/isa/ps.c Fri Mar 7 23:14:45 1997
***************
*** 131,137 ****
int ps_contdma __P((void *addr));
int ps_dma_input __P((void *addr, void *p, int cc, void (*intr) __P((void *)), void *arg));
int ps_dma_output __P((void *addr, void *p, int cc, void (*intr) __P((void *)), void *arg));
- u_int ps_get_silence __P((int encoding));
int ps_setfd __P((void *addr, int flag));
int ps_mixer_set_port __P((void *addr, mixer_ctrl_t *cp));
int ps_mixer_get_port __P((void *addr, mixer_ctrl_t *cp));
--- 131,136 ----
***************
*** 784,810 ****
return 1;
}
- u_int
- ps_get_silence(encoding)
- int encoding;
- {
- #define ULAW_SILENCE 0x7f
- #define LINEAR_SILENCE 0
- u_int auzero;
-
- switch (encoding) {
- case AUDIO_ENCODING_ULAW:
- auzero = ULAW_SILENCE;
- break;
- case AUDIO_ENCODING_PCM16:
- default:
- auzero = LINEAR_SILENCE;
- break;
- }
-
- return (auzero);
- }
-
int
ps_setfd(addr, flag)
void *addr;
--- 783,788 ----
***************
*** 926,932 ****
ps_set_in_port,
ps_get_in_port,
ps_commit_settings,
- ps_get_silence,
0,
0,
ps_dma_output,
--- 904,909 ----
diff -c -r sys.old/dev/isa/pss.c sys/dev/isa/pss.c
*** sys.old/dev/isa/pss.c Mon Oct 14 18:36:26 1996
--- sys/dev/isa/pss.c Fri Mar 7 23:13:04 1997
***************
*** 227,233 ****
pss_set_in_port,
pss_get_in_port,
ad1848_commit_settings,
- ad1848_get_silence,
NULL,
NULL,
ad1848_dma_output,
--- 227,232 ----
diff -c -r sys.old/dev/isa/sb.c sys/dev/isa/sb.c
*** sys.old/dev/isa/sb.c Fri Jan 17 13:19:40 1997
--- sys/dev/isa/sb.c Fri Mar 7 23:13:19 1997
***************
*** 103,109 ****
sbdsp_set_in_port,
sbdsp_get_in_port,
sbdsp_commit_settings,
- sbdsp_get_silence,
mulaw_expand,
mulaw_compress,
sbdsp_dma_output,
--- 103,108 ----
diff -c -r sys.old/dev/isa/sbdsp.c sys/dev/isa/sbdsp.c
*** sys.old/dev/isa/sbdsp.c Tue Mar 4 16:51:33 1997
--- sys/dev/isa/sbdsp.c Fri Mar 7 23:13:36 1997
***************
*** 1423,1449 ****
}
#endif
- u_int
- sbdsp_get_silence(encoding)
- int encoding;
- {
- #define ULAW_SILENCE 0x7f
- #define LINEAR_SILENCE 0
- u_int auzero;
-
- switch (encoding) {
- case AUDIO_ENCODING_ULAW:
- auzero = ULAW_SILENCE;
- break;
- case AUDIO_ENCODING_PCM16:
- default:
- auzero = LINEAR_SILENCE;
- break;
- }
-
- return (auzero);
- }
-
int
sbdsp_setfd(addr, flag)
void *addr;
--- 1423,1428 ----
diff -c -r sys.old/dev/isa/sbdspvar.h sys/dev/isa/sbdspvar.h
*** sys.old/dev/isa/sbdspvar.h Fri Mar 7 23:10:00 1997
--- sys/dev/isa/sbdspvar.h Fri Mar 7 23:13:57 1997
***************
*** 188,194 ****
int sbdsp_haltdma __P((void *));
int sbdsp_contdma __P((void *));
- u_int sbdsp_get_silence __P((int));
void sbdsp_compress __P((int, u_char *, int));
void sbdsp_expand __P((int, u_char *, int));
--- 188,193 ----
diff -c -r sys.old/dev/isa/wss.c sys/dev/isa/wss.c
*** sys.old/dev/isa/wss.c Fri Mar 7 17:00:59 1997
--- sys/dev/isa/wss.c Fri Mar 7 23:02:20 1997
***************
*** 176,182 ****
wss_set_in_port,
wss_get_in_port,
ad1848_commit_settings,
- ad1848_get_silence,
NULL,
NULL,
ad1848_dma_output,
--- 176,181 ----
>Audit-Trail:
>Unformatted: