Alexandre Ratchov <alex@caoua.org> 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 <thfr@openbsd.org>
+MAINTAINER = Omar Polo <op@omarpolo.com>
# 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() {
+}
+
+
No comments:
Post a Comment