Re: switching between audio devices with no playback interruption

2019-09-20 Thread Alexandre Ratchov 
On Tue, Sep 17, 2019 at 09:57:28AM +0200, Alexandre Ratchov wrote:
> 
> > Regarding the race I was talking about, doesn't dev_reopen() close the
> > current device before trying to open a new one, possibly finding an
> > incompatible device or failing to open it?  What happens in this case?
> 
> After a SIGHUP, if the new device is incompatible, the old one is
> reopened and sound continues. Now I understand your remark: oops!
> close() and open() are called in the wrong order, so another process
> could steal the device. I'll fix that.
> 

Here's a new diff with the fix. Now, the new device is opened before
closing the old one. I've fixed two other bugs in the pure audio
logic.

Index: dev.c
===
RCS file: /cvs/src/usr.bin/sndiod/dev.c,v
retrieving revision 1.61
diff -u -p -u -p -r1.61 dev.c
--- dev.c   19 Sep 2019 05:10:19 -  1.61
+++ dev.c   20 Sep 2019 07:17:54 -
@@ -969,7 +969,8 @@ dev_new(char *path, struct aparams *par,
return NULL;
}
d = xmalloc(sizeof(struct dev));
-   d->path = xstrdup(path);
+   d->path_list = NULL;
+   namelist_add(&d->path_list, path);
d->num = dev_sndnum++;
d->opt_list = NULL;
 
@@ -1174,6 +1175,74 @@ dev_close(struct dev *d)
dev_freebufs(d);
 }
 
+/*
+ * Close the device, but attempt to migrate everything to a new sndio
+ * device.
+ */
+int
+dev_reopen(struct dev *d)
+{
+   struct slot *s;
+   long long pos;
+   unsigned int pstate;
+   int delta;
+
+   /* not opened */
+   if (d->pstate == DEV_CFG)
+   return 1;
+
+   /* save state */
+   delta = d->delta;
+   pstate = d->pstate;
+
+   if (!dev_sio_reopen(d))
+   return 0;
+
+   /* reopen returns a stopped device */
+   d->pstate = DEV_INIT;
+
+   /* reallocate new buffers, with new parameters */
+   dev_freebufs(d);
+   dev_allocbufs(d);
+
+   /*
+* adjust time positions, make anything go back delta ticks, so
+* that the new device can start at zero
+*/
+   for (s = d->slot_list; s != NULL; s = s->next) {
+   pos = (long long)s->delta * d->round + s->delta_rem;
+   pos -= (long long)delta * s->round;
+   s->delta_rem = pos % (int)d->round;
+   s->delta = pos / (int)d->round;
+   if (log_level >= 3) {
+   slot_log(s);
+   log_puts(": adjusted: delta -> ");
+   log_puti(s->delta);
+   log_puts(", delta_rem -> ");
+   log_puti(s->delta_rem);
+   log_puts("\n");
+   }
+
+   /* reinitilize the format conversion chain */
+   slot_initconv(s);
+   }
+   if (d->tstate == MMC_RUN) {
+   d->mtc.delta -= delta * MTC_SEC;
+   if (log_level >= 2) {
+   dev_log(d);
+   log_puts(": adjusted mtc: delta ->");
+   log_puti(d->mtc.delta);
+   log_puts("\n");
+   }
+   }
+
+   /* start the device if needed */
+   if (pstate == DEV_RUN)
+   dev_wakeup(d);
+
+   return 1;
+}
+
 int
 dev_ref(struct dev *d)
 {
@@ -1280,7 +1349,7 @@ dev_del(struct dev *d)
}
midi_del(d->midi);
*p = d->next;
-   xfree(d->path);
+   namelist_clear(&d->path_list);
xfree(d);
 }
 
Index: dev.h
===
RCS file: /cvs/src/usr.bin/sndiod/dev.h,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 dev.h
--- dev.h   12 Jul 2019 06:30:55 -  1.21
+++ dev.h   20 Sep 2019 07:17:54 -
@@ -159,7 +159,7 @@ struct dev {
 #define DEV_INIT   1   /* stopped */
 #define DEV_RUN2   /* playin & recording */
unsigned int pstate;/* one of above */
-   char *path; /* sio path */
+   struct name *path_list;
 
/*
 * actual parameters and runtime state (i.e. once opened)
@@ -201,6 +201,7 @@ extern struct dev *dev_list;
 
 void dev_log(struct dev *);
 void dev_close(struct dev *);
+int dev_reopen(struct dev *);
 struct dev *dev_new(char *, struct aparams *, unsigned int, unsigned int,
 unsigned int, unsigned int, unsigned int, unsigned int);
 struct dev *dev_bynum(int);
Index: fdpass.c
===
RCS file: /cvs/src/usr.bin/sndiod/fdpass.c,v
retrieving revision 1.6
diff -u -p -u -p -r1.6 fdpass.c
--- fdpass.c28 Jun 2019 13:35:03 -  1.6
+++ fdpass.c20 Sep 2019 07:17:54 -
@@ -300,6 +300,7 @@ fdpass_in_helper(void *arg)
struct fdpass *f = arg;
struct dev *d;
struct port *p;
+   struct name *path;
 
if (!fdpass

Re: switching between audio devices with no playback interruption

2019-09-17 Thread Alexandre Ratchov 
On Mon, Sep 16, 2019 at 03:44:32PM -0300, Martin Pieuchot wrote:
> On 16/09/19(Mon) 19:31, Alexandre Ratchov wrote:
> > On Sun, Sep 15, 2019 at 03:01:36PM -0300, Martin Pieuchot wrote:
> > > On 29/08/19(Thu) 10:52, Alexandre Ratchov wrote:
> > > > The diff below allows to specifiy "an alternate device" to use in case
> > > > the audio device is disconnected. If so, programs continue playing
> > > > using the other device.
> > > > 
> > > > For instance, if dmesg contains:
> > > > 
> > > > audio0 at azalia0
> > > > audio1 at uaudio0
> > > > 
> > > > and /etc/rc.conf.local contains:
> > > > 
> > > > sndiod_flags=-f rsnd/0 -F rsnd/1
> > > > 
> > > > then sndiod will try to use the usb device by default. If it's
> > > > disconnected, programs continue using the internal one. Note that
> > > > there's no way to detect that the usb device is reconnected;
> > > 
> > > Instead of doing a open/reopen dance in userland, which is inherently
> > > racy, did you consider introducing a driver abstracting the various
> > > audio(4) drivers attached?  I'm thinking of something like wsmux(4) for
> > > audio.  This should allow us to transparently attach/detach usb audio
> > > devices.
> > 
> > I don't see any race conditions: once a device disappears, it is
> > closed and another one is opened in place. This only relies on calls
> > to open() and close() which are not racy. Unlike with hotplug(4),
> > there's no concurency between multiple code-paths; it's all
> > sequencial.
> 
> I understand the logic for closing an existing device, but what about
> plugging a new one?  I understand that this is not the scope of the
> diff, I wondering if that would fit in the model you're suggesting.
> 
> It is just a matter of sending SIGHUP, so I could fake that when
> uaudio(4) attaches for example?

Yes. That's the best we can do with "unix file open/close semantics"
only. Going further, if desired, would require some kind of kernel
support.

There's a full range of "clever" and complicated ways to handle
hot-plugging automatically (and to configure it!). I don't know what's
the best one, some experimenting is needed first.

> Regarding the race I was talking about, doesn't dev_reopen() close the
> current device before trying to open a new one, possibly finding an
> incompatible device or failing to open it?  What happens in this case?

After a SIGHUP, if the new device is incompatible, the old one is
reopened and sound continues. Now I understand your remark: oops!
close() and open() are called in the wrong order, so another process
could steal the device. I'll fix that.

> > I've considered the wsmux-style device for audio. Multiple audio
> > devices can't be combined, so the audio mux device would be a switch
> > between the devices, which is roughly what the diff does.
> > 
> > Switching between devices, when possible, is not trivial, because the
> > state of one device must be restored on another device. It may require
> > channel mapping and/or format conversions, in case the devices don't
> > have the same capabilities. We don't want such complicated code to run
> > with full kernel privileges and to have access to all the physical
> > memory. FWIW, this was one of the reasons for moving the audio
> > sub-system completely to user-space, leaving in the kernel only the
> > code to access the hardware.
> 
> I understand.
> 
> Another question, if a machine has multiple audio devices, including an
> USB one, can't we assume that we should try this one first?  Or could a
> choice mechanism like kern.timecounter with default value could be
> implemented to make this new feature work out of the box?

Almost. The reverse attaching order seems to cover most practical use
cases. If the user coonects a USB device, it's probably because he
wants to use it, so it would make sense for it to be tried first.

But a choice mechanism seems necessary in certain cases. For instance
"pro" audio interfaces need to be handled separately; certain USB
devices (ex. webcams) come with an unused audio interface that the
user may want to skip.

> 
> Finally, is there any downside of having a smaller DEFAULT_ROUND?

No. FWIW, the large default block and buffer sizes used to be
necessary 10+ years ago because MP kernels could delay interrupts for
more than a audio block which used to confuse azalia(4) and envy(4)
permanently. Now drivers are resilient to interrupt loss and any block
size could be used. Recent MP kernels behave better.

Re: switching between audio devices with no playback interruption

2019-09-16 Thread Martin Pieuchot 
On 16/09/19(Mon) 19:31, Alexandre Ratchov wrote:
> On Sun, Sep 15, 2019 at 03:01:36PM -0300, Martin Pieuchot wrote:
> > On 29/08/19(Thu) 10:52, Alexandre Ratchov wrote:
> > > The diff below allows to specifiy "an alternate device" to use in case
> > > the audio device is disconnected. If so, programs continue playing
> > > using the other device.
> > > 
> > > For instance, if dmesg contains:
> > > 
> > >   audio0 at azalia0
> > >   audio1 at uaudio0
> > > 
> > > and /etc/rc.conf.local contains:
> > > 
> > >   sndiod_flags=-f rsnd/0 -F rsnd/1
> > > 
> > > then sndiod will try to use the usb device by default. If it's
> > > disconnected, programs continue using the internal one. Note that
> > > there's no way to detect that the usb device is reconnected;
> > 
> > Instead of doing a open/reopen dance in userland, which is inherently
> > racy, did you consider introducing a driver abstracting the various
> > audio(4) drivers attached?  I'm thinking of something like wsmux(4) for
> > audio.  This should allow us to transparently attach/detach usb audio
> > devices.
> 
> I don't see any race conditions: once a device disappears, it is
> closed and another one is opened in place. This only relies on calls
> to open() and close() which are not racy. Unlike with hotplug(4),
> there's no concurency between multiple code-paths; it's all
> sequencial.

I understand the logic for closing an existing device, but what about
plugging a new one?  I understand that this is not the scope of the
diff, I wondering if that would fit in the model you're suggesting.  

It is just a matter of sending SIGHUP, so I could fake that when
uaudio(4) attaches for example?

Regarding the race I was talking about, doesn't dev_reopen() close the
current device before trying to open a new one, possibly finding an
incompatible device or failing to open it?  What happens in this case?

> I've considered the wsmux-style device for audio. Multiple audio
> devices can't be combined, so the audio mux device would be a switch
> between the devices, which is roughly what the diff does.
> 
> Switching between devices, when possible, is not trivial, because the
> state of one device must be restored on another device. It may require
> channel mapping and/or format conversions, in case the devices don't
> have the same capabilities. We don't want such complicated code to run
> with full kernel privileges and to have access to all the physical
> memory. FWIW, this was one of the reasons for moving the audio
> sub-system completely to user-space, leaving in the kernel only the
> code to access the hardware.

I understand.

Another question, if a machine has multiple audio devices, including an
USB one, can't we assume that we should try this one first?  Or could a
choice mechanism like kern.timecounter with default value could be
implemented to make this new feature work out of the box?

Finally, is there any downside of having a smaller DEFAULT_ROUND?

Re: switching between audio devices with no playback interruption

2019-09-16 Thread Alexandre Ratchov 
On Sun, Sep 15, 2019 at 03:01:36PM -0300, Martin Pieuchot wrote:
> On 29/08/19(Thu) 10:52, Alexandre Ratchov wrote:
> > The diff below allows to specifiy "an alternate device" to use in case
> > the audio device is disconnected. If so, programs continue playing
> > using the other device.
> > 
> > For instance, if dmesg contains:
> > 
> > audio0 at azalia0
> > audio1 at uaudio0
> > 
> > and /etc/rc.conf.local contains:
> > 
> > sndiod_flags=-f rsnd/0 -F rsnd/1
> > 
> > then sndiod will try to use the usb device by default. If it's
> > disconnected, programs continue using the internal one. Note that
> > there's no way to detect that the usb device is reconnected;
> 
> Instead of doing a open/reopen dance in userland, which is inherently
> racy, did you consider introducing a driver abstracting the various
> audio(4) drivers attached?  I'm thinking of something like wsmux(4) for
> audio.  This should allow us to transparently attach/detach usb audio
> devices.

I don't see any race conditions: once a device disappears, it is
closed and another one is opened in place. This only relies on calls
to open() and close() which are not racy. Unlike with hotplug(4),
there's no concurency between multiple code-paths; it's all
sequencial.

I've considered the wsmux-style device for audio. Multiple audio
devices can't be combined, so the audio mux device would be a switch
between the devices, which is roughly what the diff does.

Switching between devices, when possible, is not trivial, because the
state of one device must be restored on another device. It may require
channel mapping and/or format conversions, in case the devices don't
have the same capabilities. We don't want such complicated code to run
with full kernel privileges and to have access to all the physical
memory. FWIW, this was one of the reasons for moving the audio
sub-system completely to user-space, leaving in the kernel only the
code to access the hardware.

Furthermore, putting such code in the kernel would add an extra audio
software layer, while we've all the necessary bits in sndiod.

IMO, if in the future something ends up difficult to implement in
user-space I'm not against pulling parts of the code in the kernel,
but for now this doesn't seem necessary.


> > internal one will keep playing until all programs stop; then sndiod
> > will retry the usb one again. To force sndiod to retry the usb one,
> > send SIGHUP.
> 
> I'm not a fan of having such hotplug logic in userland because it is by
> default racy and opens a can of worm for being able to configure how / 
> when a driver becomes the default one.
> 

The configuration logic is simple and well-defined: there's an ordered
list of devices. Whenever a device disappears, it's closed and the
first working device of the list is opend. Configuring this is a
matter of providing the (oredered) device list; this would be
necessary in kernel implementation as well.

> > As we're at it, the same logic is applied to MIDI as well; it makes
> > possible to swap instuments without restarting programs.
> > 
> > For all this to work, both audio devices must support the same rate
> > (48kHz by default) and the same block size (now 10ms by default). This
> > requires the block size calculation to be fixed in the audio(9) layer,
> > so this diff is needed as well:
> 
> With the idea above you could attach all 'converted' audio drivers to
> the mux ;)

AFAIK, all drivers are now "fixed" (hacks remain, but they still
work). Now, whether switching between the devices is possible depends
only on the hardware and the device operating mode (which is not known
at attach time, btw).

Re: switching between audio devices with no playback interruption

2019-09-15 Thread Martin Pieuchot 
On 29/08/19(Thu) 10:52, Alexandre Ratchov wrote:
> The diff below allows to specifiy "an alternate device" to use in case
> the audio device is disconnected. If so, programs continue playing
> using the other device.
> 
> For instance, if dmesg contains:
> 
>   audio0 at azalia0
>   audio1 at uaudio0
> 
> and /etc/rc.conf.local contains:
> 
>   sndiod_flags=-f rsnd/0 -F rsnd/1
> 
> then sndiod will try to use the usb device by default. If it's
> disconnected, programs continue using the internal one. Note that
> there's no way to detect that the usb device is reconnected;

Instead of doing a open/reopen dance in userland, which is inherently
racy, did you consider introducing a driver abstracting the various
audio(4) drivers attached?  I'm thinking of something like wsmux(4) for
audio.  This should allow us to transparently attach/detach usb audio
devices.

>  the
> internal one will keep playing until all programs stop; then sndiod
> will retry the usb one again. To force sndiod to retry the usb one,
> send SIGHUP.

I'm not a fan of having such hotplug logic in userland because it is by
default racy and opens a can of worm for being able to configure how / 
when a driver becomes the default one.

> As we're at it, the same logic is applied to MIDI as well; it makes
> possible to swap instuments without restarting programs.
> 
> For all this to work, both audio devices must support the same rate
> (48kHz by default) and the same block size (now 10ms by default). This
> requires the block size calculation to be fixed in the audio(9) layer,
> so this diff is needed as well:

With the idea above you could attach all 'converted' audio drivers to
the mux ;)

switching between audio devices with no playback interruption

2019-08-29 Thread Alexandre Ratchov 
The diff below allows to specifiy "an alternate device" to use in case
the audio device is disconnected. If so, programs continue playing
using the other device.

For instance, if dmesg contains:

audio0 at azalia0
audio1 at uaudio0

and /etc/rc.conf.local contains:

sndiod_flags=-f rsnd/0 -F rsnd/1

then sndiod will try to use the usb device by default. If it's
disconnected, programs continue using the internal one. Note that
there's no way to detect that the usb device is reconnected; the
internal one will keep playing until all programs stop; then sndiod
will retry the usb one again. To force sndiod to retry the usb one,
send SIGHUP.

As we're at it, the same logic is applied to MIDI as well; it makes
possible to swap instuments without restarting programs.

For all this to work, both audio devices must support the same rate
(48kHz by default) and the same block size (now 10ms by default). This
requires the block size calculation to be fixed in the audio(9) layer,
so this diff is needed as well:

https://marc.info/?l=openbsd-tech&m=156610818325864

Please test even if you don't use this feature as this diff changes
the default audio block size (not all USB host controllers support
large transfers).

OK?

Index: dev.c
===
RCS file: /cvs/src/usr.bin/sndiod/dev.c,v
retrieving revision 1.58
diff -u -p -u -p -r1.58 dev.c
--- dev.c   29 Aug 2019 07:38:15 -  1.58
+++ dev.c   29 Aug 2019 07:52:08 -
@@ -968,7 +968,8 @@ dev_new(char *path, struct aparams *par,
return NULL;
}
d = xmalloc(sizeof(struct dev));
-   d->path = xstrdup(path);
+   d->path_list = NULL;
+   namelist_add(&d->path_list, path);
d->num = dev_sndnum++;
d->opt_list = NULL;
 
@@ -1173,6 +1174,94 @@ dev_close(struct dev *d)
dev_close_do(d);
 }
 
+/*
+ * Close the device, but attempt to migrate everything to a new sndio
+ * device.
+ */
+void
+dev_reopen(struct dev *d)
+{
+   struct slot *s;
+   long long pos;
+   unsigned int mode, round, bufsz, rate, pstate;
+   int delta;
+
+   /* not opened */
+   if (d->pstate == DEV_CFG)
+   return;
+
+   if (log_level >= 1) {
+   dev_log(d);
+   log_puts(": reopening device\n");
+   }
+
+   /* save state */
+   mode = d->mode;
+   round = d->round;
+   bufsz = d->bufsz;
+   rate = d->rate;
+   delta = d->delta;
+   pstate = d->pstate;
+
+   /* close device */
+   dev_close_do(d);
+
+   /* open device */
+   if (!dev_open_do(d)) {
+   if (log_level >= 1) {
+   dev_log(d);
+   log_puts(": found no working alternate device\n");
+   }
+   dev_exitall(d);
+   return;
+   }
+
+   /* check if new parameters are compatible with old ones */
+   if (d->mode != mode ||
+   d->round != round ||
+   d->bufsz != bufsz ||
+   d->rate != rate) {
+   if (log_level >= 1) {
+   dev_log(d);
+   log_puts(": alternate device not compatible\n");
+   }
+   dev_close(d);
+   return;
+   }
+
+   /*
+* adjust time positions, make anything go back delta ticks, so
+* that the new device can start at zero
+*/
+   for (s = d->slot_list; s != NULL; s = s->next) {
+   pos = (long long)(d->round - delta) * s->round + s->delta_rem;
+   s->delta_rem = pos % d->round;
+   s->delta += pos / (int)d->round;
+   s->delta -= s->round;
+   if (log_level >= 2) {
+   slot_log(s);
+   log_puts(": adjusted: delta -> ");
+   log_puti(s->delta);
+   log_puts(", delta_rem -> ");
+   log_puti(s->delta_rem);
+   log_puts("\n");
+   }
+   }
+   if (d->tstate == MMC_RUN) {
+   d->mtc.delta -= delta * MTC_SEC;
+   if (log_level >= 2) {
+   dev_log(d);
+   log_puts(": adjusted mtc: delta ->");
+   log_puti(d->mtc.delta);
+   log_puts("\n");
+   }
+   }
+
+   /* start the device if needed */
+   if (pstate == DEV_RUN)
+   dev_wakeup(d);
+}
+
 int
 dev_ref(struct dev *d)
 {
@@ -1279,7 +1368,7 @@ dev_del(struct dev *d)
}
midi_del(d->midi);
*p = d->next;
-   xfree(d->path);
+   namelist_clear(&d->path_list);
xfree(d);
 }
 
Index: dev.h
===
RCS file: /cvs/src/usr.bin/sndiod/dev.h,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 dev.h
--- dev.h   12 Jul 2019 06:30:55 -  1.21
+++ dev.h