Re: [PATCH] audio: Add sndio backend
Hi Brad,
I tested the sndio backend on my Linux system and I found a bug in the
sndio backend. The problem is that the function audio_run() can call the
function sndio_enable_out() to disable audio playback.
In the sndio_poll_event() function, audio_run() is called, which removes
the poll handlers when the playback stream is stopped by calling
sndio_enable_out(). Next, sndio_poll_wait() is called in
sndio_poll_event(), which can reinstall the poll handlers of the stopped
stream again. After a subsequent call to sndio_fini_out(), the pindex
pointer of the still installed poll handlers points to a memory area
that has already been freed. This can lead to a segmentation fault or a
QEMU lockup because of a blocking read.
I suggest to use a flag to prevent that sndio_poll_event() reinstalls
the poll handlers.
+typedef struct SndioVoice {
+union {
+HWVoiceOut out;
+HWVoiceIn in;
+} hw;
+struct sio_par par;
+struct sio_hdl *hdl;
+struct pollfd *pfds;
+struct pollindex {
+struct SndioVoice *self;
+int index;
+} *pindexes;
+unsigned char *buf;
+size_t buf_size;
+size_t sndio_pos;
+size_t qemu_pos;
+unsigned int mode;
+unsigned int nfds;
+ bool enabled;
+} SndioVoice;
+/*
+ * call-back called when one of the descriptors
+ * became readable or writable
+ */
+static void sndio_poll_event(SndioVoice *self, int index, int event)
+{
+int revents;
+
+/*
+ * ensure we're not called twice this cycle
+ */
+sndio_poll_clear(self);
+
+/*
+ * make self->pfds[] look as we're returning from poll syscal,
+ * this is how sio_revents expects events to be.
+ */
+self->pfds[index].revents = event;
+
+/*
+ * tell sndio to handle events and return whether we can read or
+ * write without blocking.
+ */
+revents = sio_revents(self->hdl, self->pfds);
+if (self->mode == SIO_PLAY) {
+if (revents & POLLOUT) {
+sndio_write(self);
+}
+
+if (self->qemu_pos < self->buf_size) {
+audio_run(self->hw.out.s, "sndio_out");
+}
+} else {
+if (revents & POLLIN) {
+sndio_read(self);
+}
+
+if (self->qemu_pos < self->sndio_pos) {
+audio_run(self->hw.in.s, "sndio_in");
+}
+}
+
+sndio_poll_wait(self);
- sndio_poll_wait(self);
+ if (self->enabled) {
+ sndio_poll_wait(self);
+ }
+}
+/*
+ * return a buffer where data to play can be stored
+ */
+static size_t sndio_put_buffer_out(HWVoiceOut *hw, void *buf, size_t size)
+{
+SndioVoice *self = (SndioVoice *) hw;
+
+self->qemu_pos += size;
+sndio_poll_wait(self);
+return size;
+}
+/*
+ * discard the given amount of recorded data
+ */
+static void sndio_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size)
+{
+SndioVoice *self = (SndioVoice *) hw;
+
+self->qemu_pos += size;
+if (self->qemu_pos == self->buf_size) {
+self->qemu_pos = 0;
+self->sndio_pos = 0;
+}
+sndio_poll_wait(self);
+}
It's not necessary to guard sndio_poll_wait() in sndio_put_buffer_out()
and sndio_put_buffer_in() because audio_run() will never call those
functions for a disabled stream.
+static void sndio_enable(SndioVoice *self, bool enable)
+{
+if (enable) {
+sio_start(self->hdl);
+ self->enabled = true;
+sndio_poll_wait(self);
+} else {
+ self->enabled = false;
+sndio_poll_clear(self);
+sio_stop(self->hdl);
+}
+}
With best regards,
Volker
Re: [PATCH] audio: Add sndio backend
Hi Brad,
just a few white space and coding style issues.
+/*
+ * stop polling descriptors
+ */
+static void sndio_poll_clear(SndioVoice *self)
+{
+struct pollfd *pfd;
+int i;
+
+for (i = 0; i < self->nfds; i++) {
+pfd = >pfds[i];
+qemu_set_fd_handler (pfd->fd, NULL, NULL, NULL);
No space between function name and opening brace.
+/*
+ * Set handlers for all descriptors libsndio needs to
+ * poll
+ */
+static void sndio_poll_wait(SndioVoice *self)
+{
+struct pollfd *pfd;
+int events, i;
+
+events = 0;
+if (self->mode == SIO_PLAY) {
+if (self->sndio_pos < self->qemu_pos) {
+events |= POLLOUT;
+}
+} else {
+if (self->sndio_pos < self->buf_size) {
+events |= POLLIN;
+}
+}
+
+/*
+ * fill the given array of descriptors with the events sndio
+ * wants, they are different from our 'event' variable because
+ * sndio may use descriptors internally.
+ */
+self->nfds = sio_pollfd(self->hdl, self->pfds, events);
+
+for (i = 0; i < self->nfds; i++) {
+pfd = >pfds[i];
+if (pfd->fd < 0) {
+continue;
Indent with 4 spaces.
+}
+qemu_set_fd_handler(pfd->fd,
+(pfd->events & POLLIN) ? sndio_poll_in : NULL,
+(pfd->events & POLLOUT) ? sndio_poll_out : NULL,
+>pindexes[i]);
Same here.
+static int sndio_init(SndioVoice *self,
+ struct audsettings *as, int mode, Audiodev *dev)
+{
+AudiodevSndioOptions *opts = >u.sndio;
+unsigned long long latency;
+const char *dev_name;
+struct sio_par req;
+unsigned int nch;
+int i, nfds;
+
+dev_name = opts->has_dev ? opts->dev : SIO_DEVANY;
+latency = opts->has_latency ? opts->latency : SNDIO_LATENCY_US;
+
+/* open the device in non-blocking mode */
+self->hdl = sio_open(dev_name, mode, 1);
+if (self->hdl == NULL) {
+dolog("failed to open device\n");
+return -1;
+}
+
+self->mode = mode;
+
+sio_initpar();
+
+switch (as->fmt) {
+case AUDIO_FORMAT_S8:
+req.bits = 8;
+req.sig = 1;
+break;
+case AUDIO_FORMAT_U8:
+req.bits = 8;
+req.sig = 0;
+break;
+case AUDIO_FORMAT_S16:
+req.bits = 16;
+req.sig = 1;
+break;
+case AUDIO_FORMAT_U16:
+req.bits = 16;
+req.sig = 0;
+break;
+case AUDIO_FORMAT_S32:
+req.bits = 32;
+req.sig = 1;
+break;
+case AUDIO_FORMAT_U32:
+req.bits = 32;
+req.sig = 0;
+default:
+dolog("unknown audio sample format\n");
+return -1;
+}
+
+if (req.bits > 8) {
+req.le = as->endianness ? 0 : 1;
+}
+
+req.rate = as->freq;
+if (mode == SIO_PLAY) {
+req.pchan = as->nchannels;
Indent with 4 spaces.
+} else {
+req.rchan = as->nchannels;
Here too.
+static void sndio_enable_out(HWVoiceOut *hw, bool enable)
+{
+SndioVoice *self = (SndioVoice *) hw;
+
+return sndio_enable(self, enable);
+}
Unnecessary return statement.
+
+static void sndio_enable_in(HWVoiceIn *hw, bool enable)
+{
+SndioVoice *self = (SndioVoice *) hw;
+
+return sndio_enable(self, enable);
+}
The same here
+static void sndio_fini_out(HWVoiceOut *hw)
+{
+SndioVoice *self = (SndioVoice *) hw;
+
+return sndio_fini(self);
+}
and here
+
+static void sndio_fini_in(HWVoiceIn *hw)
+{
+SndioVoice *self = (SndioVoice *) hw;
+
+return sndio_fini(self);
+}
and here.
With best regards,
Volker
Re: [PATCH] audio: Add sndio backend
On 11/14/2021 8:18 AM, Christian Schoenebeck wrote:
On Samstag, 13. November 2021 21:40:39 CET Brad Smith wrote:
On 11/8/2021 8:03 AM, Christian Schoenebeck wrote:
On Sonntag, 7. November 2021 06:19:26 CET Brad Smith wrote:
audio: Add sndio backend
Add a sndio backend.
Hi Brad!
sndio is the native API used by OpenBSD, although it has been ported to
other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
The C code is from Alexandre Ratchov and the rest of
the bits are from me.
A Signed-off-by: line is mandatory for all QEMU patches:
https://wiki.qemu.org/Contribute/SubmitAPatch
Ah, I was not aware of that. I usually include it but it was an
oversight this time.
Also, it should be clear from the patches who did what exactly, either by
splitting the patches up and assigning the respective authors accordingly,
or by making the person with the most relevant work the patch author and
describing in the commit log additional authors and what they have added/
changed, along with their Signed-off-by: line:
Signed-off-by: Alexandre Ratchov
[Brad Smith: - Added foo
- Some other change]
Signed-off-by: Brad Smith
I think I'll go with this.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/
Documentation/SubmittingPatches?
id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
Please CC those involved authors.
Will do.
I added Alexandre Ratchov on CC as he seems to be the primary author of this
patch series.
---
audio/audio.c | 1 +
audio/audio_template.h | 2 +
audio/meson.build | 1 +
audio/sndioaudio.c | 555 +
meson.build| 7 +
meson_options.txt | 4 +-
qapi/audio.json| 25 +-
qemu-options.hx| 8 +
tests/vm/freebsd | 3 +
9 files changed, 604 insertions(+), 2 deletions(-)
An additional subsection for this backend should be added to MAINTAINERS.
I did not add anything here as I figured it implies a certain level of
obligation. His time
available varies quite a bit (especially at the current time) and I
wasn't sure if it's
appropriate listing him.
Yes, that's an unpleasant but legitimate question: will there be anybody
caring for this sndio backend in QEMU or would it go orphaned right from the
start?
Orphaned from the start makes it sound like we're dumping code and
walking away.
When I say obligation I mean say responding withing say 3 - 4 days for
some sort of
an issue vs say maybe taking 1.5 - 2 weeks to respond.
It would be good to have at least somebody familiar with this code to
volunteer as reviewer(s) ("R:" line(s) in MAINTAINERS file). Reviewers are
automatically CCed, so that they can (optionally) give their feedback on
future changes to the sndio backend, i.e. when somebody sends sndio patches to
qemu-devel. This is voluntary and can be revoked at any time, and I do not
expect that you would frequently get emailed for this either.
That sounds reasonable. I'll prod Alexandre further about responding.
As this is a BSD-specific audio backend, it is not likely that an active QEMU
developer would be able to care for it.
I would not say it is BSD-specific. sndio is also packaged and available
on a good
number of Linux OS's (Alpine, Arch, Gentoo, Magia, Manjaro and some
others). I
know I have seen some Gentoo users around testing and contributing to sndio
backends. Some use it as their default sound API (Void Linux for
example). There
are older packages for Debian / Ubuntu that unfortunately don't have the
pkg-config file.
create mode 100644 audio/sndioaudio.c
diff --git a/audio/audio.c b/audio/audio.c
index 54a153c0ef..bad1ceb69e 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2005,6 +2005,7 @@ void audio_create_pdos(Audiodev *dev)
CASE(OSS, oss, Oss);
CASE(PA, pa, Pa);
CASE(SDL, sdl, Sdl);
+CASE(SNDIO, sndio, );
CASE(SPICE, spice, );
CASE(WAV, wav, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index c6714946aa..ecc5a0bc6d 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -337,6 +337,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev) return
qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE); case
AUDIODEV_DRIVER_SDL:
return
qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
+case AUDIODEV_DRIVER_SNDIO:
+return dev->u.sndio.TYPE;
case AUDIODEV_DRIVER_SPICE:
return dev->u.spice.TYPE;
case AUDIODEV_DRIVER_WAV:
diff --git a/audio/meson.build b/audio/meson.build
index 462533bb8c..e24c86e7e6 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -17,6 +17,7 @@ foreach m : [
['pa', pulse, files('paaudio.c')],
['sdl', sdl, files('sdlaudio.c')],
['jack', jack, files('jackaudio.c')],
+ ['sndio', sndio, files('sndioaudio.c')],
['spice', spice,
Re: [PATCH] audio: Add sndio backend
On Samstag, 13. November 2021 21:40:39 CET Brad Smith wrote:
> On 11/8/2021 8:03 AM, Christian Schoenebeck wrote:
> > On Sonntag, 7. November 2021 06:19:26 CET Brad Smith wrote:
> >> audio: Add sndio backend
> >>
> >> Add a sndio backend.
> >
> > Hi Brad!
> >
> >> sndio is the native API used by OpenBSD, although it has been ported to
> >> other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
> >>
> >> The C code is from Alexandre Ratchov and the rest of
> >> the bits are from me.
> >
> > A Signed-off-by: line is mandatory for all QEMU patches:
> > https://wiki.qemu.org/Contribute/SubmitAPatch
>
> Ah, I was not aware of that. I usually include it but it was an
> oversight this time.
>
> > Also, it should be clear from the patches who did what exactly, either by
> > splitting the patches up and assigning the respective authors accordingly,
> > or by making the person with the most relevant work the patch author and
> > describing in the commit log additional authors and what they have added/
> > changed, along with their Signed-off-by: line:
> >
> > Signed-off-by: Alexandre Ratchov
> > [Brad Smith: - Added foo
> >
> > - Some other change]
> >
> > Signed-off-by: Brad Smith
>
> I think I'll go with this.
>
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/
> > Documentation/SubmittingPatches?
> > id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
> >
> > Please CC those involved authors.
>
> Will do.
I added Alexandre Ratchov on CC as he seems to be the primary author of this
patch series.
> >> ---
> >>
> >> audio/audio.c | 1 +
> >> audio/audio_template.h | 2 +
> >> audio/meson.build | 1 +
> >> audio/sndioaudio.c | 555 +
> >> meson.build| 7 +
> >> meson_options.txt | 4 +-
> >> qapi/audio.json| 25 +-
> >> qemu-options.hx| 8 +
> >> tests/vm/freebsd | 3 +
> >> 9 files changed, 604 insertions(+), 2 deletions(-)
> >
> > An additional subsection for this backend should be added to MAINTAINERS.
>
> I did not add anything here as I figured it implies a certain level of
> obligation. His time
> available varies quite a bit (especially at the current time) and I
> wasn't sure if it's
> appropriate listing him.
Yes, that's an unpleasant but legitimate question: will there be anybody
caring for this sndio backend in QEMU or would it go orphaned right from the
start?
It would be good to have at least somebody familiar with this code to
volunteer as reviewer(s) ("R:" line(s) in MAINTAINERS file). Reviewers are
automatically CCed, so that they can (optionally) give their feedback on
future changes to the sndio backend, i.e. when somebody sends sndio patches to
qemu-devel. This is voluntary and can be revoked at any time, and I do not
expect that you would frequently get emailed for this either.
As this is a BSD-specific audio backend, it is not likely that an active QEMU
developer would be able to care for it.
> >> create mode 100644 audio/sndioaudio.c
> >>
> >> diff --git a/audio/audio.c b/audio/audio.c
> >> index 54a153c0ef..bad1ceb69e 100644
> >> --- a/audio/audio.c
> >> +++ b/audio/audio.c
> >> @@ -2005,6 +2005,7 @@ void audio_create_pdos(Audiodev *dev)
> >>
> >> CASE(OSS, oss, Oss);
> >> CASE(PA, pa, Pa);
> >> CASE(SDL, sdl, Sdl);
> >>
> >> +CASE(SNDIO, sndio, );
> >>
> >> CASE(SPICE, spice, );
> >> CASE(WAV, wav, );
> >>
> >> diff --git a/audio/audio_template.h b/audio/audio_template.h
> >> index c6714946aa..ecc5a0bc6d 100644
> >> --- a/audio/audio_template.h
> >> +++ b/audio/audio_template.h
> >> @@ -337,6 +337,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
> >> TYPE)(Audiodev *dev) return
> >> qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE); case
> >>
> >> AUDIODEV_DRIVER_SDL:
> >> return
> >> qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> >>
> >> +case AUDIODEV_DRIVER_SNDIO:
> >> +return dev->u.sndio.TYPE;
> >>
> >> case AUDIODEV_DRIVER_SPICE:
> >> return dev->u.spice.TYPE;
> >>
> >> case AUDIODEV_DRIVER_WAV:
> >> diff --git a/audio/meson.build b/audio/meson.build
> >> index 462533bb8c..e24c86e7e6 100644
> >> --- a/audio/meson.build
> >> +++ b/audio/meson.build
> >> @@ -17,6 +17,7 @@ foreach m : [
> >>
> >> ['pa', pulse, files('paaudio.c')],
> >> ['sdl', sdl, files('sdlaudio.c')],
> >> ['jack', jack, files('jackaudio.c')],
> >>
> >> + ['sndio', sndio, files('sndioaudio.c')],
> >>
> >> ['spice', spice, files('spiceaudio.c')]
> >>
> >> ]
> >>
> >> if m[1].found()
> >>
> >> diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
> >> new file mode 100644
> >> index 00..204af07781
> >> --- /dev/null
> >> +++ b/audio/sndioaudio.c
> >> @@ -0,0 +1,555 @@
> >> +/*
> >> + * Copyright (c) 2019 Alexandre Ratchov
>
Re: [PATCH] audio: Add sndio backend
On 11/10/2021 1:22 AM, WANG Xuerui wrote: On 2021/11/7 13:19, Brad Smith wrote: audio: Add sndio backend Add a sndio backend. sndio is the native API used by OpenBSD, although it has been ported to other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.). The C code is from Alexandre Ratchov and the rest of the bits are from me. As pointed out by others, this is lacking Signed-off-by lines; IIUC you may contact Alexandre to get theirs, and then add yours. I'm not familiar with this part of qemu, so what follows is only a somewhat brief review. That said... --- audio/audio.c | 1 + audio/audio_template.h | 2 + audio/meson.build | 1 + audio/sndioaudio.c | 555 + meson.build| 7 + meson_options.txt | 4 +- qapi/audio.json| 25 +- qemu-options.hx| 8 + tests/vm/freebsd | 3 + 9 files changed, 604 insertions(+), 2 deletions(-) create mode 100644 audio/sndioaudio.c diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c new file mode 100644 index 00..204af07781 --- /dev/null +++ b/audio/sndioaudio.c @@ -0,0 +1,555 @@ +/* + * Copyright (c) 2019 Alexandre Ratchov + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ Perhaps using an SPDX license identifier would be better? Have done so. + +/* + * TODO : + * + * Use a single device and open it in full-duplex rather than + * opening it twice (once for playback once for recording). + * + * This is the only way to ensure that playback doesn't drift with respect + * to recording, which is what guest systems expect. + */ + +#include +#include +#include "qemu/osdep.h" +#include "qemu-common.h" +#include "qemu/main-loop.h" +#include "audio.h" +#include "trace.h" + +#define AUDIO_CAP "sndio" +#include "audio_int.h" + +/* default latency in ms if no option is set */ +#define SNDIO_LATENCY_US 5 Maybe you mean "microseconds" in the comment? 50 *seconds* seems a bit long ;) I have corrected the comment.
Re: [PATCH] audio: Add sndio backend
On 11/8/2021 9:58 AM, Paolo Bonzini wrote:
On 11/7/21 06:19, Brad Smith wrote:
if not get_option('spice_protocol').auto() or have_system
@@ -1301,6 +1306,7 @@ if have_system
'oss': oss.found(),
'pa': pulse.found(),
'sdl': sdl.found(),
+ 'sndio': sndio.found(),
}
foreach k, v: audio_drivers_available
config_host_data.set('CONFIG_AUDIO_' + k.to_upper(), v)
Maybe you want to add sndio to the audio_drivers_priority array if
targetos == 'openbsd'?
Paolo
I see what to do there now. When I first came across it I wasn't sure.
Re: [PATCH] audio: Add sndio backend
On 11/8/2021 8:03 AM, Christian Schoenebeck wrote:
On Sonntag, 7. November 2021 06:19:26 CET Brad Smith wrote:
audio: Add sndio backend
Add a sndio backend.
Hi Brad!
sndio is the native API used by OpenBSD, although it has been ported to
other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
The C code is from Alexandre Ratchov and the rest of
the bits are from me.
A Signed-off-by: line is mandatory for all QEMU patches:
https://wiki.qemu.org/Contribute/SubmitAPatch
Ah, I was not aware of that. I usually include it but it was an
oversight this time.
Also, it should be clear from the patches who did what exactly, either by
splitting the patches up and assigning the respective authors accordingly, or
by making the person with the most relevant work the patch author and
describing in the commit log additional authors and what they have added/
changed, along with their Signed-off-by: line:
Signed-off-by: Alexandre Ratchov
[Brad Smith: - Added foo
- Some other change]
Signed-off-by: Brad Smith
I think I'll go with this.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/
Documentation/SubmittingPatches?
id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
Please CC those involved authors.
Will do.
---
audio/audio.c | 1 +
audio/audio_template.h | 2 +
audio/meson.build | 1 +
audio/sndioaudio.c | 555 +
meson.build| 7 +
meson_options.txt | 4 +-
qapi/audio.json| 25 +-
qemu-options.hx| 8 +
tests/vm/freebsd | 3 +
9 files changed, 604 insertions(+), 2 deletions(-)
An additional subsection for this backend should be added to MAINTAINERS.
I did not add anything here as I figured it implies a certain level of
obligation. His time
available varies quite a bit (especially at the current time) and I
wasn't sure if it's
appropriate listing him.
create mode 100644 audio/sndioaudio.c
diff --git a/audio/audio.c b/audio/audio.c
index 54a153c0ef..bad1ceb69e 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2005,6 +2005,7 @@ void audio_create_pdos(Audiodev *dev)
CASE(OSS, oss, Oss);
CASE(PA, pa, Pa);
CASE(SDL, sdl, Sdl);
+CASE(SNDIO, sndio, );
CASE(SPICE, spice, );
CASE(WAV, wav, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index c6714946aa..ecc5a0bc6d 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -337,6 +337,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev) return
qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE); case
AUDIODEV_DRIVER_SDL:
return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
+case AUDIODEV_DRIVER_SNDIO:
+return dev->u.sndio.TYPE;
case AUDIODEV_DRIVER_SPICE:
return dev->u.spice.TYPE;
case AUDIODEV_DRIVER_WAV:
diff --git a/audio/meson.build b/audio/meson.build
index 462533bb8c..e24c86e7e6 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -17,6 +17,7 @@ foreach m : [
['pa', pulse, files('paaudio.c')],
['sdl', sdl, files('sdlaudio.c')],
['jack', jack, files('jackaudio.c')],
+ ['sndio', sndio, files('sndioaudio.c')],
['spice', spice, files('spiceaudio.c')]
]
if m[1].found()
diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
new file mode 100644
index 00..204af07781
--- /dev/null
+++ b/audio/sndioaudio.c
@@ -0,0 +1,555 @@
+/*
+ * Copyright (c) 2019 Alexandre Ratchov
+ *
It is quite common for new source files in QEMU to have an authors list
section in the header here like:
* Autors:
* Alexandre Ratchov
I was looking through the tree and all of the examples I came across
were using this
with a Copyright for a company as opposed to an individual. What would
be the
format?
That way scripts/get_maintainer.pl can suggest those people as well in case
they are not explicitly listed in MAINTAINERS. Does not seem to be mandatory
for QEMU though. Just saying.
Re: [PATCH] audio: Add sndio backend
Hi Brad,
audio: Add sndio backend
Add a sndio backend.
sndio is the native API used by OpenBSD, although it has been ported to
other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
The C code is from Alexandre Ratchov and the rest of
the bits are from me.
---
audio/audio.c | 1 +
audio/audio_template.h | 2 +
audio/meson.build | 1 +
audio/sndioaudio.c | 555 +
meson.build| 7 +
meson_options.txt | 4 +-
qapi/audio.json| 25 +-
qemu-options.hx| 8 +
tests/vm/freebsd | 3 +
9 files changed, 604 insertions(+), 2 deletions(-)
create mode 100644 audio/sndioaudio.c
diff --git a/audio/audio.c b/audio/audio.c
index 54a153c0ef..bad1ceb69e 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2005,6 +2005,7 @@ void audio_create_pdos(Audiodev *dev)
CASE(OSS, oss, Oss);
CASE(PA, pa, Pa);
CASE(SDL, sdl, Sdl);
+CASE(SNDIO, sndio, );
CASE(SPICE, spice, );
CASE(WAV, wav, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index c6714946aa..ecc5a0bc6d 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -337,6 +337,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev)
return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
case AUDIODEV_DRIVER_SDL:
return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
+case AUDIODEV_DRIVER_SNDIO:
+return dev->u.sndio.TYPE;
case AUDIODEV_DRIVER_SPICE:
return dev->u.spice.TYPE;
case AUDIODEV_DRIVER_WAV:
diff --git a/audio/meson.build b/audio/meson.build
index 462533bb8c..e24c86e7e6 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -17,6 +17,7 @@ foreach m : [
['pa', pulse, files('paaudio.c')],
['sdl', sdl, files('sdlaudio.c')],
['jack', jack, files('jackaudio.c')],
+ ['sndio', sndio, files('sndioaudio.c')],
['spice', spice, files('spiceaudio.c')]
]
if m[1].found()
diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
new file mode 100644
index 00..204af07781
--- /dev/null
+++ b/audio/sndioaudio.c
@@ -0,0 +1,555 @@
+/*
+ * Copyright (c) 2019 Alexandre Ratchov
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+/*
+ * TODO :
+ *
+ * Use a single device and open it in full-duplex rather than
+ * opening it twice (once for playback once for recording).
+ *
+ * This is the only way to ensure that playback doesn't drift with respect
+ * to recording, which is what guest systems expect.
+ */
+
+#include
+#include
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/main-loop.h"
+#include "audio.h"
+#include "trace.h"
+
+#define AUDIO_CAP "sndio"
+#include "audio_int.h"
+
+/* default latency in ms if no option is set */
+#define SNDIO_LATENCY_US 5
+
+typedef struct SndioVoice {
+union {
+HWVoiceOut out;
+HWVoiceIn in;
+} hw;
+struct sio_par par;
+struct sio_hdl *hdl;
+struct pollfd *pfds;
+struct pollindex {
+struct SndioVoice *self;
+int index;
+} *pindexes;
+unsigned char *buf;
+size_t buf_size;
+size_t sndio_pos;
+size_t qemu_pos;
+unsigned int mode;
+unsigned int nfds;
+} SndioVoice;
+
+typedef struct SndioConf {
+const char *devname;
+unsigned int latency;
+} SndioConf;
+
+/* needed for forward reference */
+static void sndio_poll_in(void *arg);
+static void sndio_poll_out(void *arg);
+
+/*
+ * stop polling descriptors
+ */
+static void sndio_poll_clear(SndioVoice *self)
+{
+struct pollfd *pfd;
+int i;
+
+for (i = 0; i < self->nfds; i++) {
+pfd = >pfds[i];
+qemu_set_fd_handler (pfd->fd, NULL, NULL, NULL);
+}
+
+self->nfds = 0;
+}
+
+/*
+ * write data to the device until it blocks or
+ * all of our buffered data is written
+ */
+static void sndio_write(SndioVoice *self)
+{
+size_t todo, n;
+
+todo = self->qemu_pos - self->sndio_pos;
+
+/*
+ * transfer data to device, until it blocks
+ */
+while (todo > 0) {
+n = sio_write(self->hdl, self->buf + self->sndio_pos, todo);
+if (n == 0) {
+break;
+}
+self->sndio_pos += n;
+
Re: [PATCH] audio: Add sndio backend
El mar., 9 nov. 2021 22:53, Brad Smith escribió: > On 11/8/2021 9:58 AM, Paolo Bonzini wrote: > > > Maybe you want to add sndio to the audio_drivers_priority array if > > targetos == 'openbsd'? > > That part I was not 100% sure of. > > Am I to understand with the current Meson code it will try to use > PulseAudio instead of > ALSA on a Linux system unless overrriden? > Yes, correct. Paolo > >
Re: [PATCH] audio: Add sndio backend
On 2021/11/7 13:19, Brad Smith wrote: > audio: Add sndio backend > > Add a sndio backend. > > sndio is the native API used by OpenBSD, although it has been ported to > other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.). > > The C code is from Alexandre Ratchov and the rest of > the bits are from me. As pointed out by others, this is lacking Signed-off-by lines; IIUC you may contact Alexandre to get theirs, and then add yours. I'm not familiar with this part of qemu, so what follows is only a somewhat brief review. That said... > --- > audio/audio.c | 1 + > audio/audio_template.h | 2 + > audio/meson.build | 1 + > audio/sndioaudio.c | 555 + > meson.build| 7 + > meson_options.txt | 4 +- > qapi/audio.json| 25 +- > qemu-options.hx| 8 + > tests/vm/freebsd | 3 + > 9 files changed, 604 insertions(+), 2 deletions(-) > create mode 100644 audio/sndioaudio.c > > diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c > new file mode 100644 > index 00..204af07781 > --- /dev/null > +++ b/audio/sndioaudio.c > @@ -0,0 +1,555 @@ > +/* > + * Copyright (c) 2019 Alexandre Ratchov > + * > + * Permission to use, copy, modify, and distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ Perhaps using an SPDX license identifier would be better? > + > +/* > + * TODO : > + * > + * Use a single device and open it in full-duplex rather than > + * opening it twice (once for playback once for recording). > + * > + * This is the only way to ensure that playback doesn't drift with respect > + * to recording, which is what guest systems expect. > + */ > + > +#include > +#include > +#include "qemu/osdep.h" > +#include "qemu-common.h" > +#include "qemu/main-loop.h" > +#include "audio.h" > +#include "trace.h" > + > +#define AUDIO_CAP "sndio" > +#include "audio_int.h" > + > +/* default latency in ms if no option is set */ > +#define SNDIO_LATENCY_US 5 Maybe you mean "microseconds" in the comment? 50 *seconds* seems a bit long ;)
Re: [PATCH] audio: Add sndio backend
On 11/8/2021 9:58 AM, Paolo Bonzini wrote:
On 11/7/21 06:19, Brad Smith wrote:
if not get_option('spice_protocol').auto() or have_system
@@ -1301,6 +1306,7 @@ if have_system
'oss': oss.found(),
'pa': pulse.found(),
'sdl': sdl.found(),
+ 'sndio': sndio.found(),
}
foreach k, v: audio_drivers_available
config_host_data.set('CONFIG_AUDIO_' + k.to_upper(), v)
Maybe you want to add sndio to the audio_drivers_priority array if
targetos == 'openbsd'?
That part I was not 100% sure of.
Am I to understand with the current Meson code it will try to use
PulseAudio instead of
ALSA on a Linux system unless overrriden?
Re: [PATCH] audio: Add sndio backend
On 11/7/21 06:19, Brad Smith wrote:
if not get_option('spice_protocol').auto() or have_system
@@ -1301,6 +1306,7 @@ if have_system
'oss': oss.found(),
'pa': pulse.found(),
'sdl': sdl.found(),
+'sndio': sndio.found(),
}
foreach k, v: audio_drivers_available
config_host_data.set('CONFIG_AUDIO_' + k.to_upper(), v)
Maybe you want to add sndio to the audio_drivers_priority array if
targetos == 'openbsd'?
Paolo
Re: [PATCH] audio: Add sndio backend
On Sonntag, 7. November 2021 06:19:26 CET Brad Smith wrote:
> audio: Add sndio backend
>
> Add a sndio backend.
Hi Brad!
> sndio is the native API used by OpenBSD, although it has been ported to
> other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
>
> The C code is from Alexandre Ratchov and the rest of
> the bits are from me.
A Signed-off-by: line is mandatory for all QEMU patches:
https://wiki.qemu.org/Contribute/SubmitAPatch
Also, it should be clear from the patches who did what exactly, either by
splitting the patches up and assigning the respective authors accordingly, or
by making the person with the most relevant work the patch author and
describing in the commit log additional authors and what they have added/
changed, along with their Signed-off-by: line:
Signed-off-by: Alexandre Ratchov
[Brad Smith: - Added foo
- Some other change]
Signed-off-by: Brad Smith
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/
Documentation/SubmittingPatches?
id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297
Please CC those involved authors.
> ---
> audio/audio.c | 1 +
> audio/audio_template.h | 2 +
> audio/meson.build | 1 +
> audio/sndioaudio.c | 555 +
> meson.build| 7 +
> meson_options.txt | 4 +-
> qapi/audio.json| 25 +-
> qemu-options.hx| 8 +
> tests/vm/freebsd | 3 +
> 9 files changed, 604 insertions(+), 2 deletions(-)
An additional subsection for this backend should be added to MAINTAINERS.
> create mode 100644 audio/sndioaudio.c
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 54a153c0ef..bad1ceb69e 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2005,6 +2005,7 @@ void audio_create_pdos(Audiodev *dev)
> CASE(OSS, oss, Oss);
> CASE(PA, pa, Pa);
> CASE(SDL, sdl, Sdl);
> +CASE(SNDIO, sndio, );
> CASE(SPICE, spice, );
> CASE(WAV, wav, );
>
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index c6714946aa..ecc5a0bc6d 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -337,6 +337,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
> TYPE)(Audiodev *dev) return
> qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE); case
> AUDIODEV_DRIVER_SDL:
> return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> +case AUDIODEV_DRIVER_SNDIO:
> +return dev->u.sndio.TYPE;
> case AUDIODEV_DRIVER_SPICE:
> return dev->u.spice.TYPE;
> case AUDIODEV_DRIVER_WAV:
> diff --git a/audio/meson.build b/audio/meson.build
> index 462533bb8c..e24c86e7e6 100644
> --- a/audio/meson.build
> +++ b/audio/meson.build
> @@ -17,6 +17,7 @@ foreach m : [
>['pa', pulse, files('paaudio.c')],
>['sdl', sdl, files('sdlaudio.c')],
>['jack', jack, files('jackaudio.c')],
> + ['sndio', sndio, files('sndioaudio.c')],
>['spice', spice, files('spiceaudio.c')]
> ]
>if m[1].found()
> diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
> new file mode 100644
> index 00..204af07781
> --- /dev/null
> +++ b/audio/sndioaudio.c
> @@ -0,0 +1,555 @@
> +/*
> + * Copyright (c) 2019 Alexandre Ratchov
> + *
It is quite common for new source files in QEMU to have an authors list
section in the header here like:
* Autors:
* Alexandre Ratchov
That way scripts/get_maintainer.pl can suggest those people as well in case
they are not explicitly listed in MAINTAINERS. Does not seem to be mandatory
for QEMU though. Just saying.
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + *
> MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + *
> ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + *
> WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + *
> ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + *
> OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */
> +
> +/*
> + * TODO :
> + *
> + * Use a single device and open it in full-duplex rather than
> + * opening it twice (once for playback once for recording).
> + *
> + * This is the only way to ensure that playback doesn't drift with respect
> + * to recording, which is what guest systems expect.
> + */
> +
> +#include
> +#include
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qemu/main-loop.h"
> +#include "audio.h"
> +#include "trace.h"
> +
> +#define AUDIO_CAP "sndio"
> +#include "audio_int.h"
> +
> +/* default latency in ms if no option is set */
> +#define SNDIO_LATENCY_US 5
> +
> +typedef
[PATCH] audio: Add sndio backend
audio: Add sndio backend
Add a sndio backend.
sndio is the native API used by OpenBSD, although it has been ported to
other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
The C code is from Alexandre Ratchov and the rest of
the bits are from me.
---
audio/audio.c | 1 +
audio/audio_template.h | 2 +
audio/meson.build | 1 +
audio/sndioaudio.c | 555 +
meson.build| 7 +
meson_options.txt | 4 +-
qapi/audio.json| 25 +-
qemu-options.hx| 8 +
tests/vm/freebsd | 3 +
9 files changed, 604 insertions(+), 2 deletions(-)
create mode 100644 audio/sndioaudio.c
diff --git a/audio/audio.c b/audio/audio.c
index 54a153c0ef..bad1ceb69e 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2005,6 +2005,7 @@ void audio_create_pdos(Audiodev *dev)
CASE(OSS, oss, Oss);
CASE(PA, pa, Pa);
CASE(SDL, sdl, Sdl);
+CASE(SNDIO, sndio, );
CASE(SPICE, spice, );
CASE(WAV, wav, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index c6714946aa..ecc5a0bc6d 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -337,6 +337,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev)
return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
case AUDIODEV_DRIVER_SDL:
return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
+case AUDIODEV_DRIVER_SNDIO:
+return dev->u.sndio.TYPE;
case AUDIODEV_DRIVER_SPICE:
return dev->u.spice.TYPE;
case AUDIODEV_DRIVER_WAV:
diff --git a/audio/meson.build b/audio/meson.build
index 462533bb8c..e24c86e7e6 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -17,6 +17,7 @@ foreach m : [
['pa', pulse, files('paaudio.c')],
['sdl', sdl, files('sdlaudio.c')],
['jack', jack, files('jackaudio.c')],
+ ['sndio', sndio, files('sndioaudio.c')],
['spice', spice, files('spiceaudio.c')]
]
if m[1].found()
diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
new file mode 100644
index 00..204af07781
--- /dev/null
+++ b/audio/sndioaudio.c
@@ -0,0 +1,555 @@
+/*
+ * Copyright (c) 2019 Alexandre Ratchov
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+/*
+ * TODO :
+ *
+ * Use a single device and open it in full-duplex rather than
+ * opening it twice (once for playback once for recording).
+ *
+ * This is the only way to ensure that playback doesn't drift with respect
+ * to recording, which is what guest systems expect.
+ */
+
+#include
+#include
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/main-loop.h"
+#include "audio.h"
+#include "trace.h"
+
+#define AUDIO_CAP "sndio"
+#include "audio_int.h"
+
+/* default latency in ms if no option is set */
+#define SNDIO_LATENCY_US 5
+
+typedef struct SndioVoice {
+union {
+HWVoiceOut out;
+HWVoiceIn in;
+} hw;
+struct sio_par par;
+struct sio_hdl *hdl;
+struct pollfd *pfds;
+struct pollindex {
+struct SndioVoice *self;
+int index;
+} *pindexes;
+unsigned char *buf;
+size_t buf_size;
+size_t sndio_pos;
+size_t qemu_pos;
+unsigned int mode;
+unsigned int nfds;
+} SndioVoice;
+
+typedef struct SndioConf {
+const char *devname;
+unsigned int latency;
+} SndioConf;
+
+/* needed for forward reference */
+static void sndio_poll_in(void *arg);
+static void sndio_poll_out(void *arg);
+
+/*
+ * stop polling descriptors
+ */
+static void sndio_poll_clear(SndioVoice *self)
+{
+struct pollfd *pfd;
+int i;
+
+for (i = 0; i < self->nfds; i++) {
+pfd = >pfds[i];
+qemu_set_fd_handler (pfd->fd, NULL, NULL, NULL);
+}
+
+self->nfds = 0;
+}
+
+/*
+ * write data to the device until it blocks or
+ * all of our buffered data is written
+ */
+static void sndio_write(SndioVoice *self)
+{
+size_t todo, n;
+
+todo = self->qemu_pos - self->sndio_pos;
+
+/*
+ * transfer data to device, until it blocks
+ */
+while (todo > 0) {
+n = sio_write(self->hdl, self->buf + self->sndio_pos, todo);
+if (n == 0) {
+break;
+}
+self->sndio_pos += n;
+todo -= n;
+}
+
+if
Re: [PATCH] audio: Add sndio backend
> > > ERROR: g_free(NULL) is safe this check is probably not required
> > > #381: FILE: audio/sndioaudio.c:318:
> > > +if (self->pfds) {
> > > +g_free(self->pfds);
> > Reasonable too.
>
> Not clear to me. Leave as is or needs a change?
Just use "g_free(self->pfds)".
cheers,
Gerd
Re: [PATCH] audio: Add sndio backend
On 3/4/20 8:50 AM, Brad Smith wrote:
Add a sndio backend.
sndio is the native API used by OpenBSD, although it has been ported to
other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
The C code is from Alexandre Ratchov and the rest of
the bits are from me.
Signed-off-by: Alexandre Ratchov
Signed-off-by: Brad Smith
diff --git a/audio/Makefile.objs b/audio/Makefile.objs
Your patch lacks the usual --- separator and diffstat that 'git
format-patch' (and therefore 'git send-email') uses. This makes it
harder to see at a glance the approximate impact of your patch, and
whether it touches a file I'm interested in.
+++ b/qapi/audio.json
@@ -248,6 +248,28 @@
'*out':'AudiodevPaPerDirectionOptions',
'*server': 'str' } }
+##
+# @AudiodevSndioOptions:
+#
+# Options of the sndio audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @dev: the name of the sndio device to use (default 'default')
+#
+# @latency: play buffer size (in microseconds)
+#
+# Since: 4.0
You've missed 4.0 by a long shot; the next release is 5.0.
+##
+{ 'struct': 'AudiodevSndioOptions',
+ 'data': {
+'*in':'AudiodevPerDirectionOptions',
+'*out': 'AudiodevPerDirectionOptions',
+'*dev': 'str',
+'*latency': 'uint32'} }
+
##
# @AudiodevWavOptions:
#
@@ -287,7 +309,7 @@
##
{ 'enum': 'AudiodevDriver',
'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'oss', 'pa', 'sdl',
-'spice', 'wav' ] }
+'sndio', 'spice', 'wav' ] }
It's also customary to document enum options, something like:
# @sndio (since 5.0)
Furthermore, since:
+++ b/qemu-options.hx
@@ -566,6 +566,9 @@ DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
#ifdef CONFIG_AUDIO_SDL
"-audiodev sdl,id=id[,prop[=value][,...]]\n"
#endif
+#ifdef CONFIG_AUDIO_SNDIO
+"-audiodev sndio,id=id[,prop[=value][,...]]\n"
+#endif
the option is only useful based on configure-time decisions, it seems
like the new enum element should be:
{ 'name': 'sndio', 'if': 'defined(CONFIG_AUDIO_SNDIO)' }
to make it introspectible whether support is compiled in to a given
binary. True, there are existing enum members that should do likewise,
so maybe it's worth a cleanup patch first.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH] audio: Add sndio backend
On 3/5/2020 3:50 AM, Gerd Hoffmann wrote:
On Wed, Mar 04, 2020 at 07:04:07AM -0800, no-re...@patchew.org wrote:
Patchew URL:
https://patchew.org/QEMU/20200304145003.gb15...@humpty.home.comstyle.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PATCH] audio: Add sndio backend
Message-id: 20200304145003.gb15...@humpty.home.comstyle.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
From https://github.com/patchew-project/qemu
* [new tag] patchew/20200304145003.gb15...@humpty.home.comstyle.com
-> patchew/20200304145003.gb15...@humpty.home.comstyle.com
Switched to a new branch 'test'
421ab62 audio: Add sndio backend
=== OUTPUT BEGIN ===
ERROR: space prohibited before that close parenthesis ')'
#41: FILE: audio/audio.c:1977:
+CASE(SNDIO, sndio, );
False positive I'd say.
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
Adding an entry to the MAINTAINERS file is a good idea though.
Ok. Will do.
ERROR: g_free(NULL) is safe this check is probably not required
#381: FILE: audio/sndioaudio.c:318:
+if (self->pfds) {
+g_free(self->pfds);
Reasonable too.
Not clear to me. Leave as is or needs a change?
Re: [PATCH] audio: Add sndio backend
On Wed, Mar 04, 2020 at 07:04:07AM -0800, no-re...@patchew.org wrote:
> Patchew URL:
> https://patchew.org/QEMU/20200304145003.gb15...@humpty.home.comstyle.com/
>
>
>
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Subject: [PATCH] audio: Add sndio backend
> Message-id: 20200304145003.gb15...@humpty.home.comstyle.com
> Type: series
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
>
> From https://github.com/patchew-project/qemu
> * [new tag] patchew/20200304145003.gb15...@humpty.home.comstyle.com
> -> patchew/20200304145003.gb15...@humpty.home.comstyle.com
> Switched to a new branch 'test'
> 421ab62 audio: Add sndio backend
>
> === OUTPUT BEGIN ===
> ERROR: space prohibited before that close parenthesis ')'
> #41: FILE: audio/audio.c:1977:
> +CASE(SNDIO, sndio, );
False positive I'd say.
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
Adding an entry to the MAINTAINERS file is a good idea though.
> ERROR: g_free(NULL) is safe this check is probably not required
> #381: FILE: audio/sndioaudio.c:318:
> +if (self->pfds) {
> +g_free(self->pfds);
Reasonable too.
cheers,
Gerd
Re: [PATCH] audio: Add sndio backend
Patchew URL:
https://patchew.org/QEMU/20200304145003.gb15...@humpty.home.comstyle.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PATCH] audio: Add sndio backend
Message-id: 20200304145003.gb15...@humpty.home.comstyle.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
From https://github.com/patchew-project/qemu
* [new tag] patchew/20200304145003.gb15...@humpty.home.comstyle.com ->
patchew/20200304145003.gb15...@humpty.home.comstyle.com
Switched to a new branch 'test'
421ab62 audio: Add sndio backend
=== OUTPUT BEGIN ===
ERROR: space prohibited before that close parenthesis ')'
#41: FILE: audio/audio.c:1977:
+CASE(SNDIO, sndio, );
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#59:
new file mode 100644
ERROR: g_free(NULL) is safe this check is probably not required
#381: FILE: audio/sndioaudio.c:318:
+if (self->pfds) {
+g_free(self->pfds);
ERROR: g_free(NULL) is safe this check is probably not required
#386: FILE: audio/sndioaudio.c:323:
+if (self->pindexes) {
+g_free(self->pindexes);
ERROR: g_free(NULL) is safe this check is probably not required
#391: FILE: audio/sndioaudio.c:328:
+if (self->buf) {
+g_free(self->buf);
total: 4 errors, 1 warnings, 780 lines checked
Commit 421ab62c169b (audio: Add sndio backend) has style problems, please
review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200304145003.gb15...@humpty.home.comstyle.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
[PATCH] audio: Add sndio backend
Add a sndio backend.
sndio is the native API used by OpenBSD, although it has been ported to
other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
The C code is from Alexandre Ratchov and the rest of
the bits are from me.
Signed-off-by: Alexandre Ratchov
Signed-off-by: Brad Smith
diff --git a/audio/Makefile.objs b/audio/Makefile.objs
index d7490a379f..e6b86aacc7 100644
--- a/audio/Makefile.objs
+++ b/audio/Makefile.objs
@@ -28,3 +28,8 @@ common-obj-$(CONFIG_AUDIO_SDL) += sdl.mo
sdl.mo-objs = sdlaudio.o
sdl.mo-cflags := $(SDL_CFLAGS)
sdl.mo-libs := $(SDL_LIBS)
+
+# sndio module
+common-obj-$(CONFIG_AUDIO_SNDIO) += sndio.mo
+sndio.mo-objs = sndioaudio.o
+sndio.mo-libs := $(SNDIO_LIBS)
diff --git a/audio/audio.c b/audio/audio.c
index 9ac9a20c41..6eeaaece5a 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1974,6 +1974,7 @@ void audio_create_pdos(Audiodev *dev)
CASE(OSS, oss, Oss);
CASE(PA, pa, Pa);
CASE(SDL, sdl, );
+CASE(SNDIO, sndio, );
CASE(SPICE, spice, );
CASE(WAV, wav, );
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7013d3041f..b516bae41b 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -336,6 +336,8 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev)
return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
case AUDIODEV_DRIVER_SDL:
return dev->u.sdl.TYPE;
+case AUDIODEV_DRIVER_SNDIO:
+return dev->u.sndio.TYPE;
case AUDIODEV_DRIVER_SPICE:
return dev->u.spice.TYPE;
case AUDIODEV_DRIVER_WAV:
diff --git a/audio/sndioaudio.c b/audio/sndioaudio.c
new file mode 100644
index 00..2dbc0ddaee
--- /dev/null
+++ b/audio/sndioaudio.c
@@ -0,0 +1,566 @@
+/*
+ * Copyright (c) 2019 Alexandre Ratchov
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+/*
+ * TODO :
+ *
+ * Use a single device and open it in full-duplex rather than
+ * opening it twice (once for playback once for recording).
+ *
+ * This is the only way to ensure that playback doesn't drift with respect
+ * to recording, which is what guest systems expect.
+ */
+
+#include
+#include
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qemu/main-loop.h"
+#include "audio.h"
+#include "trace.h"
+
+#define AUDIO_CAP "sndio"
+#include "audio_int.h"
+
+/* default latency in ms if no option is set */
+#define SNDIO_LATENCY_US 5
+
+typedef struct SndioVoice {
+union {
+HWVoiceOut out;
+HWVoiceIn in;
+} hw;
+struct sio_par par;
+struct sio_hdl *hdl;
+struct pollfd *pfds;
+struct pollindex {
+struct SndioVoice *self;
+int index;
+} *pindexes;
+unsigned char *buf;
+size_t buf_size;
+size_t sndio_pos;
+size_t qemu_pos;
+unsigned int mode;
+unsigned int nfds;
+} SndioVoice;
+
+typedef struct SndioConf {
+const char *devname;
+unsigned int latency;
+} SndioConf;
+
+/* needed for forward reference */
+static void sndio_poll_in(void *arg);
+static void sndio_poll_out(void *arg);
+
+/*
+ * stop polling descriptors
+ */
+static void sndio_poll_clear(SndioVoice *self)
+{
+struct pollfd *pfd;
+int i;
+
+for (i = 0; i < self->nfds; i++) {
+pfd = >pfds[i];
+qemu_set_fd_handler (pfd->fd, NULL, NULL, NULL);
+}
+
+self->nfds = 0;
+}
+
+/*
+ * write data to the device until it blocks or
+ * all of our buffered data is written
+ */
+static void sndio_write(SndioVoice *self)
+{
+size_t todo, n;
+
+todo = self->qemu_pos - self->sndio_pos;
+
+/*
+ * transfer data to device, until it blocks
+ */
+while (todo > 0) {
+n = sio_write(self->hdl, self->buf + self->sndio_pos, todo);
+if (n == 0) {
+break;
+}
+self->sndio_pos += n;
+todo -= n;
+}
+
+if (self->sndio_pos == self->buf_size) {
+/*
+ * we complete the block
+ */
+self->sndio_pos = 0;
+self->qemu_pos = 0;
+}
+}
+
+/*
+ * read data from the device until it blocks or
+ * there no room any longer
+ */
+static void sndio_read(SndioVoice *self)
+{
+size_t todo, n;
+
+todo = self->buf_size - self->sndio_pos;
+
+/*
+ * transfer data from the device, until
