On Mon, Aug 24, 2020 at 08:19:18PM +0200, Omar Polo wrote:
>
> Hello ports@,
>
> Here's a second try at adding a working audio backend to Godot. It took
> me some time but now I think it's ready. It was fun, and even if I
> didn't knew anything about audio handling, I have learned something.
> The sio_open manpage in particular was incredibly helpful at explaining
> the various concept.
>
> You can find attached a patch to add a sndio driver. I'm also taking up
> the maintainership as previously discussed off-list with Thomas.
>
> Riccardo (cc'd) and me have been testing this for a week. We've tested
> with Oddventure, Hive Time and some hobby projects. Oddventure crashes
> regularly at some point (after ~ 15-20 minutes of gameplay), but it does
> so regardless of the sndio patch. As a test, I've been able to play for
> roughly 25-30 minutes at Hive Time with SIO_ERROR^1 set without having
> xruns, so I'm quite confident the driver isn't utterly broken.
>
> As Thomas suggested, it would be nice to upstream this patch but as I'm
> not able to compile and run the engine from the master branch I'm
> refraining to for the moment. In the meantime, it would be nice to have
> this tested by a wider audience and, possibly, also committed :)
>
> Some notes (and questions) about the patch:
>
Thanks for the code, few answers and suggestions below:
> - it's better to bundle the code in ${FILESDIR} and copy it in
> post-extract (what I'm currently doing) or have patches against
> /dev/null? I think the first option is more clear
>
> - the $OpenBSD$ cvs tag on the sources is correct? portcheck doesn't
> complain, but I've never done this before so I'm not sure.
>
> - the driver only implements the minimum necessary in order to play
> sounds: other audio driver also let the user change the device and
> some driver (e.g. pulseaudio) also support recording audio. Adding
> support for recording shouldn't be difficult, but I don't know a
> single game that requires this. Switching audio device at runtime
> doesn't seem too much useful to me, but I think I can implement it
> eventually
>
This is OK. Most ports don't provide device selection either. We're
slowly shifting toward pushing the device selection in sndiod using
sndioctl.
> - (I'm a noob when it comes to audio stuff so apologies) it's better
> SIO_SYNC or SIO_IGNORE for a game? SIO_IGNORE makes the device
> pauses during overruns and underruns, while SIO_SYNC makes the device
> discard the samples on overrun and then play the same amount of
> silence later.
The default is fine. FWIW, SIO_SYNC is when multiple audio programs
need to be synchronized; it's automatically forced by sndiod when
needed.
>
> - in the main loop in AudioDriverSndio::thread_func I'm not checking
> the return of sio_open/sio_close: this is deliberate. It's to keep
> the code simple, as if an error occurs during the open/close we'll
> notice it eventually some lines after on sio_write and exit there.
you mean sio_{start,stop}() ;-)
> - while other driver fills sample_in with zeroes when there is nothing
> to play, doing so with sndio would make the whole engine sluggish.
> The was_active + sio_open/close dance is due to that. I don't have
> an explanation for that, and I've got the idea from the sndio
> website[2]
This part is strange as AudioDriver has public "stop" method. Other
backends seem to use the flag for error recovery, which doesn't make
sense for sndio.
So, AFAICS, the "active" flag is only used to start calling
audio_server_process() and is never reset. If so, it would make sense
call sio_start() in the init() method and do the following the
thread_func(), pseudo-code:
thread_func()
{
memset(ad->samples_in, 0, ...);
while (!sio_eof()) {
if (active)
audio_server_process()
w = sio_write(...)
}
}
> - (please remember that I don't have any prior experience with audio
> stuff) every other audio driver has a loop like
> for ( ... ) {
> ad->samples_out.write[i] = ad->samples_in[i] >> 16;
> }
> I don't know why it's needed to shift every sample but, hey, everyone
> is doing it and so am I :)
>
This is because audio_server_process() produces 32-bit samples in in
samples_in[] while sio_write() expects 16-bit samples, as 16-bit
format was requested with sio_setpar(). The '>>' operator is used to
retain the 16 most significant bits.
You could avoid the need for the samples_out[] array and the
conversions by setting par.bits to 32 in the sio_setpar() call. sndiod
will do the necessary conversions.
> Index: files/sndio/audio_driver_sndio.cpp
> ===================================================================
> RCS file: files/sndio/audio_driver_sndio.cpp
> diff -N files/sndio/audio_driver_sndio.cpp
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ files/sndio/audio_driver_sndio.cpp 24 Aug 2020 17:59:45 -0000
> @@ -0,0 +1,181 @@
> +/* $OpenBSD$ */
> +
> +#include "audio_driver_sndio.h"
> +
> +#ifdef SNDIO_ENABLED
> +
> +#include "core/os/os.h"
> +#include "core/project_settings.h"
> +
> +#include <poll.h>
> +#include <stdio.h>
> +
> +Error AudioDriverSndio::init() {
> + active = false;
> + thread_exited = false;
> + exit_thread = false;
> + speaker_mode = SPEAKER_MODE_STEREO;
> + was_active = 0;
> +
> + handle = sio_open(SIO_DEVANY, SIO_PLAY, 0);
> + ERR_FAIL_COND_V(handle == NULL, ERR_CANT_OPEN);
> +
> + struct sio_par par;
> + sio_initpar(&par);
> +
> + par.bits = 16;
> + par.rate = GLOBAL_GET("audio/mix_rate");
> +
I'd set the par.appbufsz here to something sensible for games. For
instance 50ms-100ms is reasonable latency for games. Example:
par.appbufsz = 50 * par.rate / 1000;
> + /*
> + * XXX: require SIO_SYNC instead of SIO_IGNORE (the default) ?
> + * what is the most sensible choice for a game?
> + */
> + // par.xrun = SIO_ERROR;
> +
> + if (!sio_setpar(handle, &par))
> + ERR_FAIL_COND_V(1, ERR_CANT_OPEN);
> +
> + if (!sio_getpar(handle, &par))
> + ERR_FAIL_COND_V(1, ERR_CANT_OPEN);
> +
At this point, I'd check the format.
if (par.bits != 16 || par.bps != 2 || par.le != SIO_LE_NATIVE)
ERR_FAIL_COND_V(1, ....);
as sndiod is supposed to run, any format is supported. So this would
be mostly for the style or to catch setup errors
> + mix_rate = par.rate;
> + channels = par.pchan;
> + period_size = par.appbufsz;
> +
> + samples_in.resize(period_size * channels);
> + samples_out.resize(period_size * channels);
> +
> + mutex = Mutex::create();
> + thread = Thread::create(AudioDriverSndio::thread_func, this);
> +
> + return OK;
> +}
> +
> +void AudioDriverSndio::thread_func(void *p_udata) {
> + AudioDriverSndio *ad = (AudioDriverSndio*)p_udata;
> +
> + int nfds = sio_nfds(ad->handle);
> + struct pollfd *pfds;
> + if ((pfds = (struct pollfd*)calloc(sizeof(*pfds), nfds)) == NULL) {
> + ERR_PRINTS("cannot allocate memory");
> + ad->active = false;
> + ad->exit_thread = true;
> + ad->thread_exited = true;
> + return;
> + }
> +
> + while (!ad->exit_thread) {
> + if (ad->was_active) {
> + nfds = sio_pollfd(ad->handle, pfds, POLLOUT);
> + if (nfds > 0) {
> + if (poll(pfds, nfds, -1) < 0) {
> + ERR_PRINTS("sndio: poll failed");
> + ad->exit_thread = true;
> + break;
> + }
> + }
> + }
> +
sio_pollfd() is not needed here because sio_write() is blocking (BTW,
the corresponding sio_revents() call is missing).
> + ad->lock();
> + ad->start_counting_ticks();
> +
> + if (!ad->active) {
> + if (ad->was_active) {
> + ad->was_active = 0;
> + sio_stop(ad->handle);
> + }
> +
> + ad->stop_counting_ticks();
> + ad->unlock();
> + /* XXX: sleep a bit here? */
> + continue;
> + } else {
> + if (!ad->was_active) {
> + ad->was_active = 1;
> + sio_start(ad->handle);
> + }
> +
> + ad->audio_server_process(ad->period_size,
> ad->samples_in.ptrw());
> +
> + for (size_t i = 0; i < ad->period_size*ad->channels;
> ++i) {
> + ad->samples_out.write[i] = ad->samples_in[i] >>
> 16;
> + }
> + }
> +
> + size_t left = ad->period_size * ad->channels * sizeof(int16_t);
> + size_t wrote = 0;
> +
> + while (left != 0 && !ad->exit_thread) {
> + const uint8_t *src = (const
> uint8_t*)ad->samples_out.ptr();
> + size_t w = sio_write(ad->handle, (void*)(src + wrote),
> left);
> +
> + if (w == 0 && sio_eof(ad->handle)) {
> + ERR_PRINTS("sndio: fatal error");
> + ad->exit_thread = true;
> + } else {
> + wrote += w;
> + left -= w;
> + }
> + }
sio_write() writes the whole period (beacaue we use blocking mode), so
this could be simplified as follows:
bytes = ad->period_size * ad->channels * sizeof(int16_t);
w = sio_write(ad->handle, ad->samples_out.ptr(), bytes);
if (w != bytes) {
ERR_PRINTS(...)
ad->exit_thread = true;
}
> +
> + ad->stop_counting_ticks();
> + ad->unlock();
> + }
> +
> + free((void*)pfds);
> + ad->thread_exited = true;
> +}
> +
> +void AudioDriverSndio::start() {
> + active = true;
> +}
> +
> +int AudioDriverSndio::get_mix_rate() const {
> + return mix_rate;
> +}
> +
> +AudioDriver::SpeakerMode AudioDriverSndio::get_speaker_mode() const {
> + return speaker_mode;
> +}
> +
> +void AudioDriverSndio::lock() {
> + if (!thread || !mutex)
> + return;
> + mutex->lock();
> +}
> +
> +void AudioDriverSndio::unlock() {
> + if (!thread || !mutex)
> + return;
> + mutex->unlock();
> +}
> +
> +void AudioDriverSndio::finish() {
> + if (thread) {
> + exit_thread = true;
> + Thread::wait_to_finish(thread);
> +
> + memdelete(thread);
> + thread = NULL;
> + }
> +
> + if (mutex) {
> + memdelete(mutex);
> + mutex = NULL;
> + }
> +
> + if (handle) {
> + sio_close(handle);
> + handle = NULL;
> + }
> +}
> +
> +AudioDriverSndio::AudioDriverSndio() {
> + mutex = NULL;
> + thread = NULL;
> +}
> +
> +AudioDriverSndio::~AudioDriverSndio() {
> +}
> +
> +#endif
> Index: files/sndio/audio_driver_sndio.h
> ===================================================================
> RCS file: files/sndio/audio_driver_sndio.h
> diff -N files/sndio/audio_driver_sndio.h
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ files/sndio/audio_driver_sndio.h 24 Aug 2020 17:59:45 -0000
> @@ -0,0 +1,49 @@
> +/* $OpenBSD$ */
> +
> +#include "servers/audio_server.h"
> +
> +#ifdef SNDIO_ENABLED
> +
> +#include "core/os/mutex.h"
> +#include "core/os/thread.h"
> +
> +#include <sndio.h>
> +
> +class AudioDriverSndio : public AudioDriver {
> + Thread *thread;
> + Mutex *mutex;
> +
> + Vector<int32_t> samples_in;
> + Vector<int16_t> samples_out;
> + int was_active;
> +
> + struct sio_hdl *handle;
> +
> + static void thread_func(void*);
> + size_t period_size;
> +
> + unsigned int mix_rate;
> + int channels;
> + bool active;
> + bool thread_exited;
> + mutable bool exit_thread;
> + SpeakerMode speaker_mode;
> +
> +public:
> + const char *get_name() const {
> + return "sndio";
> + }
> +
> + virtual Error init();
> + virtual void start();
> + virtual int get_mix_rate() const;
> + virtual SpeakerMode get_speaker_mode() const;
> + virtual void lock();
> + virtual void unlock();
> + virtual void finish();
> +
> + AudioDriverSndio();
> + ~AudioDriverSndio();
> +};
> +
> +#endif
