Alexandre Ratchov <[email protected]> writes:
> Thanks for the code, few answers and suggestions below:
thank you for reviewing this
> [...]
>
>> - 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(...)
> }
> }
Yup, you're right. I didn't noticed that the active flag is never
reset. I've rewrote the thread_func following your pseudo code.
>> - (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.
I've also followed this advice, now there is a single samples array. It
makes the code easier to follow IMHO
>>
>> [...]
>>
>> + 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;
I trust you, I've added this as-is :)
>> + /*
>> + * 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
Ops, fixed in the attached patch
>>
>> [...]
>> + 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).
I used sio_pollfd to hold the mutex for as little time as possible, as
calling sio_write with the mutex locked could take too much time. With
sio_pollfd I am sure that I don't have to wait (too much at least)
during the write.
But all of this isn't required. The samples array is owned by this
class and it's not exposed, nor touched anywhere else, so I could simply
sio_write after unlocking the mutex. I'm doing this in the updated
patch.
>> + 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;
> }
I supposed it was the case, but I wasn't sure. I'm now doing as you
suggest.
The driver is like half of the code now :)
P.S. I've also done some minor style edits (upstream requires braces
everywhere) and I've dropped some redundant ERR_FAIL_COND_V.
Index: Makefile
===================================================================
RCS file: /cvs/ports/games/godot/Makefile,v
retrieving revision 1.14
diff -u -p -r1.14 Makefile
--- Makefile 18 Aug 2020 17:39:30 -0000 1.14
+++ Makefile 4 Sep 2020 11:15:03 -0000
@@ -6,9 +6,9 @@ V = 3.2.2
DISTNAME = godot-${V}-stable
PKGNAME = godot-${V}
CATEGORIES = games
-REVISION = 0
+REVISION = 1
HOMEPAGE = https://godotengine.org/
-MAINTAINER = Thomas Frohwein <[email protected]>
+MAINTAINER = Omar Polo <[email protected]>
# MIT
PERMIT_PACKAGE = Yes
@@ -16,7 +16,7 @@ PERMIT_PACKAGE = Yes
WANTLIB += ${COMPILER_LIBCXX}
WANTLIB += GL X11 Xau Xcursor Xdmcp Xext Xfixes Xi Xinerama Xrandr
WANTLIB += Xrender c enet execinfo freetype intl m mbedtls mbedcrypto
-WANTLIB += mbedx509 mpcdec ogg opus opusfile png theora theoradec
+WANTLIB += mbedx509 mpcdec ogg opus opusfile png sndio theora theoradec
WANTLIB += vorbis vorbisfile webp xcb z pcre2-32 vpx zstd
COMPILER = base-clang ports-gcc base-gcc
@@ -83,6 +83,9 @@ CFLAGS += -mlongcall
CXXFLAGS += -mlongcall
LDFLAGS += -Wl,--relax
.endif
+
+post-extract:
+ cp -R ${FILESDIR}/sndio ${WRKDIST}/drivers
pre-configure:
${SUBST_CMD} ${WRKSRC}/drivers/unix/os_unix.cpp
Index: files/sndio/SCsub
===================================================================
RCS file: files/sndio/SCsub
diff -N files/sndio/SCsub
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ files/sndio/SCsub 4 Sep 2020 11:15:03 -0000
@@ -0,0 +1,6 @@
+#!/usr/bin/env python
+# $OpenBSD$
+
+Import('env')
+
+env.add_source_files(env.drivers_sources, "*.cpp")
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 4 Sep 2020 11:15:03 -0000
@@ -0,0 +1,134 @@
+/* $OpenBSD$ */
+
+#include "audio_driver_sndio.h"
+
+#ifdef SNDIO_ENABLED
+
+#include "core/os/os.h"
+#include "core/project_settings.h"
+
+Error AudioDriverSndio::init() {
+ active = false;
+ thread_exited = false;
+ exit_thread = false;
+ speaker_mode = SPEAKER_MODE_STEREO;
+
+ 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 = 32;
+ par.bps = 4;
+ par.rate = GLOBAL_GET("audio/mix_rate");
+ par.appbufsz = 50 * par.rate / 1000;
+
+ if (!sio_setpar(handle, &par)) {
+ return ERR_CANT_OPEN;
+ }
+
+ if (!sio_getpar(handle, &par)) {
+ return ERR_CANT_OPEN;
+ }
+
+ if (par.bits != 32 || par.bps != 4 || par.le != SIO_LE_NATIVE) {
+ return ERR_CANT_OPEN;
+ }
+
+ if (!sio_start(handle)) {
+ return ERR_CANT_OPEN;
+ }
+
+ mix_rate = par.rate;
+ channels = par.pchan;
+ period_size = par.appbufsz;
+
+ samples.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;
+
+ for (size_t i = 0; i < ad->period_size * ad->channels; ++i)
+ ad->samples.write[i] = 0;
+
+ while (!ad->exit_thread) {
+ ad->lock();
+ ad->start_counting_ticks();
+
+ if (ad->active) {
+ ad->audio_server_process(ad->period_size,
ad->samples.ptrw());
+ }
+
+ ad->stop_counting_ticks();
+ ad->unlock();
+
+ size_t bytes = ad->period_size * ad->channels * sizeof(int32_t);
+ if (sio_write(ad->handle, ad->samples.ptr(), bytes) != bytes) {
+ ERR_PRINTS("sndio: fatal error");
+ ad->exit_thread = true;
+ }
+ }
+
+ 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 4 Sep 2020 11:15:03 -0000
@@ -0,0 +1,47 @@
+/* $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;
+
+ 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
Index: patches/patch-drivers_SCsub
===================================================================
RCS file: patches/patch-drivers_SCsub
diff -N patches/patch-drivers_SCsub
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-drivers_SCsub 4 Sep 2020 11:15:03 -0000
@@ -0,0 +1,15 @@
+$OpenBSD$
+
+add sndio
+
+Index: drivers/SCsub
+--- drivers/SCsub.orig
++++ drivers/SCsub
+@@ -12,6 +12,7 @@ SConscript("windows/SCsub")
+ SConscript("alsa/SCsub")
+ SConscript("coreaudio/SCsub")
+ SConscript("pulseaudio/SCsub")
++SConscript("sndio/SCsub")
+ if env["platform"] == "windows":
+ SConscript("wasapi/SCsub")
+ if env["xaudio2"]:
Index: patches/patch-platform_x11_detect_py
===================================================================
RCS file: /cvs/ports/games/godot/patches/patch-platform_x11_detect_py,v
retrieving revision 1.2
diff -u -p -r1.2 patch-platform_x11_detect_py
--- patches/patch-platform_x11_detect_py 19 Jul 2020 13:02:38 -0000
1.2
+++ patches/patch-platform_x11_detect_py 4 Sep 2020 11:15:03 -0000
@@ -1,6 +1,6 @@
$OpenBSD: patch-platform_x11_detect_py,v 1.2 2020/07/19 13:02:38 thfr Exp $
-remove hardcoded -O2, found by bcallah@
+remove hardcoded -O2, found by bcallah@. Add sndio
Index: platform/x11/detect.py
--- platform/x11/detect.py.orig
@@ -27,3 +27,14 @@ Index: platform/x11/detect.py
env.Prepend(CPPDEFINES=["DEBUG_ENABLED"])
if env["debug_symbols"] == "yes":
+@@ -302,6 +293,10 @@ def configure(env):
+ env.ParseConfig("pkg-config alsa --libs")
+ else:
+ print("ALSA libraries not found, disabling driver")
++
++ print("Enabling sndio")
++ env.Append(CPPDEFINES=["SNDIO_ENABLED"])
++ env.Append(LINKFLAGS=["-lsndio"])
+
+ if env["pulseaudio"]:
+ if os.system("pkg-config --exists libpulse") == 0: # 0 means found
Index: patches/patch-platform_x11_os_x11_cpp
===================================================================
RCS file: /cvs/ports/games/godot/patches/patch-platform_x11_os_x11_cpp,v
retrieving revision 1.2
diff -u -p -r1.2 patch-platform_x11_os_x11_cpp
--- patches/patch-platform_x11_os_x11_cpp 19 Jul 2020 13:02:38 -0000
1.2
+++ patches/patch-platform_x11_os_x11_cpp 4 Sep 2020 11:15:03 -0000
@@ -1,6 +1,6 @@
$OpenBSD: patch-platform_x11_os_x11_cpp,v 1.2 2020/07/19 13:02:38 thfr Exp $
-fix libXrandr library name
+fix libXrandr library name and load sndio
Index: platform/x11/os_x11.cpp
--- platform/x11/os_x11.cpp.orig
@@ -18,3 +18,14 @@ Index: platform/x11/os_x11.cpp
} else {
XRRQueryVersion(x11_display, &xrandr_major, &xrandr_minor);
if (((xrandr_major << 8) | xrandr_minor) >= 0x0105) {
+@@ -3596,6 +3596,10 @@ void OS_X11::update_real_mouse_position() {
+ }
+
+ OS_X11::OS_X11() {
++
++#ifdef SNDIO_ENABLED
++ AudioDriverManager::add_driver(&driver_sndio);
++#endif
+
+ #ifdef PULSEAUDIO_ENABLED
+ AudioDriverManager::add_driver(&driver_pulseaudio);
Index: patches/patch-platform_x11_os_x11_h
===================================================================
RCS file: patches/patch-platform_x11_os_x11_h
diff -N patches/patch-platform_x11_os_x11_h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-platform_x11_os_x11_h 4 Sep 2020 11:15:03 -0000
@@ -0,0 +1,26 @@
+$OpenBSD$
+
+load sndio
+
+Index: platform/x11/os_x11.h
+--- platform/x11/os_x11.h.orig
++++ platform/x11/os_x11.h
+@@ -36,6 +36,7 @@
+ #include "crash_handler_x11.h"
+ #include "drivers/alsa/audio_driver_alsa.h"
+ #include "drivers/alsamidi/midi_driver_alsamidi.h"
++#include "drivers/sndio/audio_driver_sndio.h"
+ #include "drivers/pulseaudio/audio_driver_pulseaudio.h"
+ #include "drivers/unix/os_unix.h"
+ #include "joypad_linux.h"
+@@ -185,6 +186,10 @@ class OS_X11 : public OS_Unix {
+
+ #ifdef ALSAMIDI_ENABLED
+ MIDIDriverALSAMidi driver_alsamidi;
++#endif
++
++#ifdef SNDIO_ENABLED
++ AudioDriverSndio driver_sndio;
+ #endif
+
+ #ifdef PULSEAUDIO_ENABLED
