On Tue, Aug 30, 2011 at 01:35:56PM +0200, Donovan Watteau wrote:
> OK, thanks! So here's a new version, with the following changes:
> - Added SHARED_ONLY=Yes, regen'd PLIST and friends.
> - Added MAINTAINER=Me.
> - Fixed WANTLIB-flac and WANTLIB-wavpack.
> - Fixed HOSTCC/HOSTLD (which defaulted to gcc) and HOST_CFLAGS (which
> defaulted to their own flags). I think it should follow CC/LD and
> CFLAGS by default when not crossbuilding; I'll drop them a mail
> about that.
> - Removed CONFIG_WAV=y which is the default choice. I wanted to make
> it explicit but it doesn't seem to be the rule here.
> - Removed -Wredundant-decls which makes useless noise on OpenBSD
> because of some problems in system headers.
> - Added $OpenBSD$ in patches.
>
> If no one complains about the sndio backend, I'll ask cmus
> developers to merge it.
>
Thanks. It works for me except with small block sizes on my setup,
example, if I start:
aucat -r 48000 -b 960 -z 480
and then try to play a .mp3 there's no sound. The diff below seems to
fix it. Besides that, few remarks inlined below:
> static long long rdpos;
> static long long wrpos;
> static long bytes_per_sec;
>
> static void onmove_cb(void *addr, int delta)
> {
> rdpos += delta * (int)(par.bps * par.pchan);
> }
rdpos is never used
>
> static int sndio_mixer_set_volume(int l, int r)
> {
> sndio_volume = l > r ? l : r;
>
> if (hdl != NULL)
> sio_setvol(hdl, sndio_volume * SIO_MAXVOL / 100);
>
> return 0;
> }
>
> static int sndio_mixer_get_volume(int *l, int *r)
> {
> *l = *r = sndio_volume;
>
> return 0;
> }
>
> static int sndio_set_sf(sample_format_t sf)
> {
...
> switch (sf_get_bits(sndio_sf)) {
> case 16:
> par.bits = 16;
> break;
> case 8:
> par.bits = 8;
> break;
> default:
> return -1;
> }
>
I'd set the buffer size to around 300ms here, ex:
par.appbufsz = par.rate * 300 / 1000;
on OpenBSD file reads are always blocking (if one thread reads from a
file, then all threads are blocked during the read), and buffers
should be large enough to avoid underruns.
> if (!sio_setpar(hdl, &par))
> return -1;
>
> if (!sio_getpar(hdl, &par))
> return -1;
>
> wrpos = 0;
> rdpos = 0;
> sio_onmove(hdl, onmove_cb, NULL);
these are not used currenty
> sndio_mixer_set_volume(sndio_volume, sndio_volume);
>
> if (!sio_start(hdl))
> return -1;
>
> bytes_per_sec = par.bps * par.pchan * par.rate;
^^^^^^^^^^^^^
this one isn't used either
> return 0;
> }
>
>
> static int sndio_write(const char *buf, int cnt)
> {
> const char *t;
>
> cnt /= 4;
> cnt *= 4;
it's ok to not round cnt to sample size
> t = buf;
> while (cnt > 0) {
> int rc = sio_write(hdl, buf, cnt);
> if (rc == -1) {
rc is positive and EINTR is handled in sio_write()
> if (errno == EINTR)
> continue;
> else
> return rc;
> }
> buf += rc;
> cnt -= rc;
> }
it's ok to call sio_write() only once because in non-blocking mode it
always writes 'cnt' bytes and returns 0 on error (eg. usb audio device
unplugged, aucat killed).
>
> return (buf - t);
> }
>
> static int sndio_pause(void)
> {
> sndio_paused = 1;
>
synchronization is not an issue afaics, so you could call sio_stop()
here. This would save CPU cycles (and battery) during the pause
> return 0;
> }
>
> static int sndio_unpause(void)
> {
> sndio_paused = 0;
>
ditto, I'd call sio_start() here.
> return 0;
> }
>
> static int sndio_buffer_space(void)
> {
> return par.bufsz;
> }
It's not clear for me what exactly this function is supposed to
return. It seems that it should return the amount of buffer space
available for writing without blocking, which doesn't make sense for
sndio. Returning the full buffer size seems to not hurt cmus though.
The return value is in bytes, so 'bufsz * bps * pchan' may be
what you intend
-- Alexandre
--- sndio.c.old Wed Aug 31 00:59:45 2011
+++ sndio.c Wed Aug 31 01:37:26 2011
@@ -36,15 +36,6 @@ static struct sio_hdl *hdl = NULL;
static int sndio_volume = 100;
static int sndio_paused;
-static long long rdpos;
-static long long wrpos;
-static long bytes_per_sec;
-
-static void onmove_cb(void *addr, int delta)
-{
- rdpos += delta * (int)(par.bps * par.pchan);
-}
-
static int sndio_mixer_set_volume(int l, int r)
{
sndio_volume = l > r ? l : r;
@@ -94,7 +85,7 @@ static int sndio_set_sf(sample_format_t sf)
default:
return -1;
}
-
+ par.appbufsz = par.rate * 300 / 1000;
apar = par;
if (!sio_setpar(hdl, &par))
@@ -103,15 +94,16 @@ static int sndio_set_sf(sample_format_t sf)
if (!sio_getpar(hdl, &par))
return -1;
- wrpos = 0;
- rdpos = 0;
- sio_onmove(hdl, onmove_cb, NULL);
+ if (apar.rate != par.rate || apar.pchan != par.pchan ||
+ apar.bits != par.bits || (par.bits > 8 && apar.le != par.le) ||
+ apar.sig != par.sig)
+ return -1;
+
sndio_mixer_set_volume(sndio_volume, sndio_volume);
if (!sio_start(hdl))
return -1;
- bytes_per_sec = par.bps * par.pchan * par.rate;
return 0;
}
@@ -150,24 +142,12 @@ static int sndio_open(sample_format_t sf)
static int sndio_write(const char *buf, int cnt)
{
- const char *t;
+ size_t rc;
- cnt /= 4;
- cnt *= 4;
- t = buf;
- while (cnt > 0) {
- int rc = sio_write(hdl, buf, cnt);
- if (rc == -1) {
- if (errno == EINTR)
- continue;
- else
- return rc;
- }
- buf += rc;
- cnt -= rc;
- }
-
- return (buf - t);
+ rc = sio_write(hdl, buf, cnt);
+ if (rc == 0)
+ return -1;
+ return rc;
}
static int op_sndio_set_option(int key, const char *val)
@@ -182,21 +162,29 @@ static int op_sndio_get_option(int key, char **val)
static int sndio_pause(void)
{
- sndio_paused = 1;
-
+ if (!sndio_paused) {
+ sio_stop(hdl);
+ sndio_paused = 1;
+ }
return 0;
}
static int sndio_unpause(void)
{
- sndio_paused = 0;
-
+ if (sndio_paused) {
+ sio_start(hdl);
+ sndio_paused = 0;
+ }
return 0;
}
static int sndio_buffer_space(void)
{
- return par.bufsz;
+ /*
+ * do as if there's always some space
+ * and let sio_write() block
+ */
+ return par.bufsz * par.bps * par.pchan;
}
static int sndio_mixer_init(void)
