[Spice-devel] [PATCH xf86-video-qxl] Revise the XSpice audio processing to avoid the use of pthreads.
Jeremy White
jwhite at codeweavers.com
Sat Oct 18 09:37:13 PDT 2014
The initial implementation used a separate thread to drive the audio
playback channel. But if you have adaptive streaming turned on,
you will eventually get a update_client_playback_latency message on the
display channel (which in the Xspice case is being driven by the main,
Xorg, thread).
After enough time you would get a thread collision and bad things
would result. I saw this manifest as infinite spin loops in snd_send_data.
This patch eliminates the use of threading altogether, making everything
run in the main Xorg thread using watches and timers, eliminating the
possibility of thread collision.
Signed-off-by: Jeremy White <jwhite at codeweavers.com>
---
src/qxl.h | 3 +-
src/spiceqxl_audio.c | 545 +++++++++++++++++++++++++++++++-------------------
2 files changed, 336 insertions(+), 212 deletions(-)
diff --git a/src/qxl.h b/src/qxl.h
index fa9b13f..603faca 100644
--- a/src/qxl.h
+++ b/src/qxl.h
@@ -334,8 +334,6 @@ struct _qxl_screen_t
/* XSpice specific, dragged from the Device */
QXLReleaseInfo *last_release;
- pthread_t audio_thread;
-
uint32_t cmdflags;
uint32_t oom_running;
uint32_t num_free_res; /* is having a release ring effective
@@ -353,6 +351,7 @@ struct _qxl_screen_t
} guest_primary;
char playback_fifo_dir[PATH_MAX];
+ void *playback_opaque;
#endif /* XSPICE */
uint32_t deferred_fps;
diff --git a/src/spiceqxl_audio.c b/src/spiceqxl_audio.c
index 02859ee..3a62236 100644
--- a/src/spiceqxl_audio.c
+++ b/src/spiceqxl_audio.c
@@ -20,6 +20,10 @@
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
+/* XSpice based audio feed; reads from files (presumably fifos) in a configured directory,
+ and mixes their raw data on to the spice playback channel. */
+
+
#ifdef HAVE_CONFIG_H
#include "config.h"
#endif
@@ -32,283 +36,362 @@
#include <sys/time.h>
#include <unistd.h>
#include <dirent.h>
+#include <sys/inotify.h>
+
#define BUFFER_PERIODS 10
#define PERIOD_MS 10
#define MAX_FIFOS 16
+struct fifo_data {
+ char *buffer;
+ int size;
+ int len;
+ int add_to;
+ int remove_from;
+ int fd;
+ SpiceWatch *watch;
+};
+
struct audio_data {
- int fifo_fds[MAX_FIFOS];
- ino_t inodes[MAX_FIFOS];
- uint32_t valid_bytes, write_offs;
- char *buffer, *spice_buffer;
- int period_frames;
- uint32_t spice_write_offs, spice_buffer_bytes;
- uint32_t frame_bytes, period_bytes, fed, buffer_bytes;
+ struct fifo_data fifos[MAX_FIFOS];
+ uint32_t *spice_buffer;
+ int spice_buffer_bytes;
+ int ahead;
struct timeval last_read_time;
+ int fifo_count;
+ SpiceTimer *wall_timer;
+ int wall_timer_live;
+ int dir_watch;
+ int fifo_dir_watch;
+ SpiceWatch *fifo_dir_qxl_watch;
};
-static ssize_t
-read_from_fifos (struct audio_data *data)
+/* We maintain a ring buffer for each file we are reading from;
+ these helper functions faciliate adding data to the buffer,
+ and removing it. */
+static inline void * fifo_add_ptr(struct fifo_data *f)
{
- size_t to_read_bytes = min(data->period_bytes, data->buffer_bytes - data->write_offs);
- int i;
- ssize_t max_readed = 0;
- int16_t *out_buf = (int16_t*)(data->buffer + data->write_offs), *buf;
-
- buf = malloc(to_read_bytes);
- if (!buf)
- {
- ErrorF("playback: malloc failed: %s\n", strerror(errno));
- return 0;
+ if (f->len == f->size)
+ return NULL;
+
+ if (f->remove_from > 0 && f->remove_from <= f->add_to) {
+ if (f->len > 0)
+ memmove(f->buffer, f->buffer + f->remove_from, f->len);
+ f->add_to -= f->remove_from;
+ f->remove_from = 0;
}
- memset(out_buf, 0, to_read_bytes);
+ return f->buffer + f->add_to;
+}
- for (i = 0; i < MAX_FIFOS; ++i)
- {
- unsigned int s;
- ssize_t readed;
+static inline void fifo_data_added(struct fifo_data *f, int n)
+{
+ f->add_to = (f->add_to + n) % f->size;
+ f->len += n;
+}
- if (data->fifo_fds[i] < 0)
- continue;
+static inline void fifo_remove_data(struct fifo_data *f, unsigned char *dest, int len)
+{
+ int remain = f->size - f->remove_from;
+
+ if (remain < len) {
+ memcpy(dest, f->buffer + f->remove_from, remain);
+ dest += remain;
+ len -= remain;
+ f->len -= remain;
+ f->remove_from = 0;
+ }
- readed = read(data->fifo_fds[i], buf, to_read_bytes);
- if (readed < 0)
- {
- if (errno != EAGAIN && errno != EINTR)
- ErrorF("playback: read from FIFO %d failed: %s\n", data->fifo_fds[i], strerror(errno));
- continue;
- }
+ memcpy(dest, f->buffer + f->remove_from, len);
+ f->len -= len;
+ f->remove_from = (f->remove_from + len) % f->size;
+}
- if (readed == 0)
- {
- ErrorF("playback: FIFO %d gave EOF\n", data->fifo_fds[i]);
- close(data->fifo_fds[i]);
- data->fifo_fds[i] = -1;
- data->inodes[i] = 0;
- continue;
- }
+static int mix_in_one_fifo(struct fifo_data *f, int16_t *out, int len)
+{
+ int s;
+ for (s = 0; f->len > 0 && s < (len / 2); s++) {
+ int16_t mix;
+
+ fifo_remove_data(f, (unsigned char *) &mix, sizeof(mix));
+
+ /* FIXME: Ehhh, this'd be better as floats. With this algorithm,
+ * samples mixed after being clipped will have undue weight. But
+ * if we're clipping, then we're distorted anyway, so whatever. */
+ if (out[s] + mix > INT16_MAX)
+ out[s] = INT16_MAX;
+ else if (out[s] + mix < -INT16_MAX)
+ out[s] = -INT16_MAX;
+ else
+ out[s] += mix;
- if (readed > max_readed)
- max_readed = readed;
-
- for (s = 0; s < readed / sizeof(int16_t); ++s)
- {
- /* FIXME: Ehhh, this'd be better as floats. With this algorithm,
- * samples mixed after being clipped will have undue weight. But
- * if we're clipping, then we're distorted anyway, so whatever. */
- if (out_buf[s] + buf[s] > INT16_MAX)
- out_buf[s] = INT16_MAX;
- else if (out_buf[s] + buf[s] < -INT16_MAX)
- out_buf[s] = -INT16_MAX;
- else
- out_buf[s] += buf[s];
- }
}
- free(buf);
+ return s * 2;
+}
- if (!max_readed)
- return 0;
+static void mix_in_fifos(qxl_screen_t *qxl)
+{
+ int i;
+ struct audio_data *data = qxl->playback_opaque;
+ struct fifo_data *f;
- data->valid_bytes = min(data->valid_bytes + max_readed,
- data->buffer_bytes);
+ memset(data->spice_buffer, 0, data->spice_buffer_bytes);
- data->write_offs += max_readed;
- data->write_offs %= data->buffer_bytes;
+ if (data->fifo_count == 0)
+ return;
- ++data->fed;
+ /* First fifo can just be copied */
+ f = &data->fifos[0];
+ fifo_remove_data(f, (unsigned char *) data->spice_buffer, min(data->spice_buffer_bytes, f->len));
- return max_readed;
+ /* Extra fifos need to be mixed in */
+ for (i = 1; i < data->fifo_count; i++) {
+ f = &data->fifos[i];
+ if (f->len > 0)
+ mix_in_one_fifo(f, (int16_t *) data->spice_buffer, data->spice_buffer_bytes);
+ }
}
-static int
-scan_fifos (struct audio_data *data, const char *dirname)
+/* Pulseaudio file sinks will generate output as fast as we'll
+ take it (e.g. use mplayer to play a song). This logic causes
+ us to only take more data when enough wall time has passed. */
+static void see_how_much_time_passed(struct audio_data *data)
{
- DIR *dir;
- struct dirent *ent;
- int i;
+ struct timeval end, diff, period_tv;
- dir = opendir(dirname);
- if (!dir)
- {
- ErrorF("playback: failed to open FIFO directory '%s': %s\n", dirname, strerror(errno));
- return 1;
+ if (! data->ahead) {
+ gettimeofday(&data->last_read_time, NULL);
+ return;
}
- while ((ent = readdir(dir)))
- {
- char path[PATH_MAX];
+ period_tv.tv_sec = 0;
+ period_tv.tv_usec = PERIOD_MS * 1000;
- if (ent->d_name[0] == '.')
- /* skip dot-files */
- continue;
+ gettimeofday(&end, NULL);
- for (i = 0; i < MAX_FIFOS; ++i)
- if (ent->d_ino == data->inodes[i])
- break;
- if (i < MAX_FIFOS)
- /* file already open */
- continue;
+ timersub(&end, &data->last_read_time, &diff);
- for (i = 0; i < MAX_FIFOS; ++i)
- if (data->fifo_fds[i] < 0)
- break;
- if (i == MAX_FIFOS)
- {
- static int once = 0;
- if (!once)
- {
- ErrorF("playback: Too many FIFOs already open\n");
- ++once;
+ while (data->ahead &&
+ (diff.tv_sec > 0 || diff.tv_usec >= period_tv.tv_usec)) {
+ timersub(&diff, &period_tv, &diff);
+
+ --data->ahead;
+
+ timeradd(&data->last_read_time, &period_tv, &data->last_read_time);
+ }
+
+ if (data->ahead == 0)
+ data->last_read_time = end;
+}
+
+static void condense_fifos(struct audio_data *data)
+{
+ int i;
+ struct fifo_data tmp;
+
+ for (i = 0; i < data->fifo_count; i++) {
+ struct fifo_data *f = &data->fifos[i];
+ if (f->fd == -1 && f->len == 0) {
+ if ((i + 1) < data->fifo_count) {
+ tmp = *f;
+ *f = data->fifos[data->fifo_count - 1];
+ data->fifos[data->fifo_count - 1] = tmp;
}
- closedir(dir);
- return 0;
+ data->fifo_count--;
+ i--;
}
+ }
+}
- if (snprintf(path, sizeof(path), "%s/%s", dirname, ent->d_name) >= sizeof(path)) {
- ErrorF("playback: FIFO filename is too long - truncated into %s", path);
+static void watch_or_wait(qxl_screen_t *qxl);
+static void process_fifos(qxl_screen_t *qxl, struct audio_data *data, int maxlen)
+{
+ /* mplayer + pulse will write data to the fifo as fast as we can read it.
+ So we need to pace how quickly we consume the data.
+ So feed no more than BUFFER_PERIODS of data ahead; after that, we wait
+ for the clock to elapse.
+ */
+ see_how_much_time_passed(data);
+
+ while (data->ahead < BUFFER_PERIODS && maxlen > 0) {
+ if (! data->spice_buffer) {
+ uint32_t chunk_frames;
+ spice_server_playback_get_buffer(&qxl->playback_sin, &data->spice_buffer, &chunk_frames);
+ data->spice_buffer_bytes = chunk_frames * sizeof(int16_t) * SPICE_INTERFACE_PLAYBACK_CHAN;
}
- data->fifo_fds[i] = open(path, O_RDONLY | O_RSYNC | O_NONBLOCK);
- if (data->fifo_fds[i] < 0)
- {
- ErrorF("playback: open FIFO '%s' failed: %s\n", path, strerror(errno));
- continue;
- }
- ErrorF("playback: opened FIFO '%s' as %d\n", path, data->fifo_fds[i]);
+ if (! data->spice_buffer)
+ break;
+
+ mix_in_fifos(qxl);
+ maxlen -= data->spice_buffer_bytes;
+
+ spice_server_playback_put_samples(&qxl->playback_sin, data->spice_buffer);
+ data->spice_buffer = NULL;
- data->inodes[i] = ent->d_ino;
+ data->ahead++;
}
- closedir(dir);
+ condense_fifos(data);
- return 0;
+ watch_or_wait(qxl);
}
-static void *
-audio_thread_main (void *p)
+static void read_from_fifos(int fd, int event, void *opaque)
{
- qxl_screen_t *qxl = p;
+ qxl_screen_t *qxl = opaque;
+ struct audio_data *data = qxl->playback_opaque;
int i;
- struct audio_data data;
- int freq = SPICE_INTERFACE_PLAYBACK_FREQ;
-
- memset(&data, 0, sizeof(data));
- for (i = 0; i < MAX_FIFOS; ++i)
- data.fifo_fds[i] = -1;
-
-#if SPICE_INTERFACE_PLAYBACK_MAJOR > 1 || SPICE_INTERFACE_PLAYBACK_MINOR >= 3
- freq = spice_server_get_best_playback_rate(&qxl->playback_sin);
-#endif
- data.period_frames = freq * PERIOD_MS / 1000;
+ int maxlen = 0;
+ for (i = 0; i < data->fifo_count; i++) {
+ struct fifo_data *f = &data->fifos[i];
+
+ if (f->size - f->len > 0 && f->fd >= 0) {
+ int rc;
+ rc = read(f->fd, fifo_add_ptr(f), f->size - f->len);
+ if (rc > 0)
+ fifo_data_added(f, rc);
+ else if (rc == -1 && (errno == EAGAIN || errno == EINTR))
+ /* no new data to read */;
+ else {
+ if (rc == 0)
+ ErrorF("fifo %d closed\n", f->fd);
+ else
+ ErrorF("fifo %d error %d: %s\n", f->fd, errno, strerror(errno));
+ if (f->watch)
+ qxl->core->watch_remove(f->watch);
+ f->watch = NULL;
+ close(f->fd);
+ f->fd = -1;
+ }
- data.frame_bytes = sizeof(int16_t) * SPICE_INTERFACE_PLAYBACK_CHAN;
+ if (f->size == f->len) {
+ if (f->watch)
+ qxl->core->watch_remove(f->watch);
+ f->watch = NULL;
+ }
+ }
- data.period_bytes = data.period_frames * data.frame_bytes;
- data.buffer_bytes = data.period_bytes * BUFFER_PERIODS;
- data.buffer = malloc(data.buffer_bytes);
- memset(data.buffer, 0, data.buffer_bytes);
+ if (f->len > maxlen)
+ maxlen = f->len;
+ }
- spice_server_playback_start(&qxl->playback_sin);
+ process_fifos(qxl, data, maxlen);
+}
- gettimeofday(&data.last_read_time, NULL);
+static void start_watching(qxl_screen_t *qxl)
+{
+ struct audio_data *data = qxl->playback_opaque;
+ int i;
- while (1)
- {
- struct timeval end, diff, period_tv;
-
- if (scan_fifos(&data, qxl->playback_fifo_dir))
- goto cleanup;
-
- while (data.fed < BUFFER_PERIODS)
- {
- if (!read_from_fifos(&data))
- break;
-
- while (data.valid_bytes)
- {
- int to_copy_bytes;
- uint32_t read_offs;
-
- if (!data.spice_buffer)
- {
- uint32_t chunk_frames;
- spice_server_playback_get_buffer(&qxl->playback_sin, (uint32_t**)&data.spice_buffer, &chunk_frames);
- data.spice_buffer_bytes = chunk_frames * data.frame_bytes;
- }
- if (!data.spice_buffer)
- break;
-
- if (data.valid_bytes > data.write_offs)
- {
- read_offs = data.buffer_bytes + data.write_offs - data.valid_bytes;
- to_copy_bytes = min(data.buffer_bytes - read_offs,
- data.spice_buffer_bytes - data.spice_write_offs);
- }
- else
- {
- read_offs = data.write_offs - data.valid_bytes;
- to_copy_bytes = min(data.valid_bytes,
- data.spice_buffer_bytes - data.spice_write_offs);
- }
+ for (i = 0; i < data->fifo_count; i++) {
+ struct fifo_data *f = &data->fifos[i];
+ if (f->watch || f->size == f->len)
+ continue;
- memcpy(data.spice_buffer + data.spice_write_offs,
- data.buffer + read_offs, to_copy_bytes);
+ f->watch = qxl->core->watch_add(f->fd, SPICE_WATCH_EVENT_READ, read_from_fifos, qxl);
+ }
+}
- data.valid_bytes -= to_copy_bytes;
+static void watch_or_wait(qxl_screen_t *qxl)
+{
+ struct audio_data *data = qxl->playback_opaque;
- data.spice_write_offs += to_copy_bytes;
+ if (data->ahead < BUFFER_PERIODS)
+ start_watching(qxl);
- if (data.spice_write_offs >= data.spice_buffer_bytes)
- {
- spice_server_playback_put_samples(&qxl->playback_sin, (uint32_t*)data.spice_buffer);
- data.spice_buffer = NULL;
- data.spice_buffer_bytes = data.spice_write_offs = 0;
- }
- }
+ if (data->ahead) {
+ if (! data->wall_timer_live) {
+ qxl->core->timer_start(data->wall_timer, PERIOD_MS);
+ data->wall_timer_live++;
}
+ }
+ else {
+ if (data->wall_timer_live)
+ qxl->core->timer_cancel(data->wall_timer);
+ data->wall_timer_live = 0;
+ }
+
+}
- period_tv.tv_sec = 0;
- period_tv.tv_usec = PERIOD_MS * 1000;
+static void wall_ticker(void *opaque)
+{
+ qxl_screen_t *qxl = opaque;
+ struct audio_data *data = qxl->playback_opaque;
- usleep(period_tv.tv_usec);
+ data->wall_timer_live = 0;
- gettimeofday(&end, NULL);
+ read_from_fifos(-1, 0, qxl);
+}
- timersub(&end, &data.last_read_time, &diff);
+static void handle_one_change(qxl_screen_t *qxl, struct inotify_event *e)
+{
+ if (e->mask & (IN_CREATE | IN_MOVED_TO)) {
+ struct audio_data *data = qxl->playback_opaque;
+ struct fifo_data *f = &data->fifos[data->fifo_count];
+ char *fname;
- while (data.fed &&
- (diff.tv_sec > 0 || diff.tv_usec >= period_tv.tv_usec))
- {
- timersub(&diff, &period_tv, &diff);
+ if (data->fifo_count == MAX_FIFOS) {
+ static int once = 0;
+ if (!once) {
+ ErrorF("playback: Too many FIFOs already open\n");
+ ++once;
+ }
+ return;
+ }
- --data.fed;
+ fname = malloc(strlen(e->name) + strlen(qxl->playback_fifo_dir) + 1 + 1);
+ strcpy(fname, qxl->playback_fifo_dir);
+ strcat(fname, "/");
+ strcat(fname, e->name);
- timeradd(&data.last_read_time, &period_tv, &data.last_read_time);
+ f->fd = open(fname, O_RDONLY | O_RSYNC | O_NONBLOCK);
+ free(fname);
+ if (f->fd < 0) {
+ ErrorF("playback: open FIFO '%s' failed: %s\n", e->name, strerror(errno));
+ return;
}
- if (!data.fed)
- data.last_read_time = end;
- }
+ ErrorF("playback: opened FIFO '%s' as %d\n", e->name, f->fd);
-cleanup:
- if (data.spice_buffer)
- {
- memset(data.spice_buffer, 0, data.spice_buffer_bytes - data.spice_write_offs);
- spice_server_playback_put_samples(&qxl->playback_sin, (uint32_t*)data.spice_buffer);
- data.spice_buffer = NULL;
- data.spice_buffer_bytes = data.spice_write_offs = 0;
+ data->fifo_count++;
+
+ f->watch = qxl->core->watch_add(f->fd, SPICE_WATCH_EVENT_READ, read_from_fifos, qxl);
}
+}
- free(data.buffer);
+static void playback_dir_changed(int fd, int event, void *opaque)
+{
+ qxl_screen_t *qxl = opaque;
+ static unsigned char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
+ static int index = 0;
+ struct inotify_event *e;
+ int rc;
+
+ do {
+ rc = read(fd, buf + index, sizeof(buf) - index);
+ if (rc > 0) {
+ index += rc;
+ if (index >= sizeof(*e)) {
+ int len;
+ e = (struct inotify_event *) buf;
+ len = sizeof(*e) + e->len;
+ if (index >= len) {
+ handle_one_change(qxl, e);
+ if (index > len)
+ memmove(buf, buf + index, index - len);
+ index -= len;
+ }
+ }
+ }
+ }
+ while (rc > 0);
+}
- spice_server_playback_stop(&qxl->playback_sin);
- return NULL;
-}
static const SpicePlaybackInterface playback_sif = {
{
@@ -319,13 +402,40 @@ static const SpicePlaybackInterface playback_sif = {
}
};
+static void audio_initialize (qxl_screen_t *qxl)
+{
+ int i;
+ struct audio_data *data = qxl->playback_opaque;
+ int freq = SPICE_INTERFACE_PLAYBACK_FREQ;
+ int period_frames;
+ int period_bytes;
+ int frame_bytes;
+
+#if SPICE_INTERFACE_PLAYBACK_MAJOR > 1 || SPICE_INTERFACE_PLAYBACK_MINOR >= 3
+ freq = spice_server_get_best_playback_rate(&qxl->playback_sin);
+#endif
+
+ period_frames = freq * PERIOD_MS / 1000;
+ frame_bytes = sizeof(int16_t) * SPICE_INTERFACE_PLAYBACK_CHAN;
+ period_bytes = period_frames * frame_bytes;
+
+ for (i = 0; i < MAX_FIFOS; ++i) {
+ data->fifos[i].fd = -1;
+ data->fifos[i].size = period_bytes * BUFFER_PERIODS;
+ data->fifos[i].buffer = calloc(1, data->fifos[i].size);
+ }
+
+ spice_server_playback_start(&qxl->playback_sin);
+}
+
+
int
qxl_add_spice_playback_interface (qxl_screen_t *qxl)
{
int ret;
+ struct audio_data *data = calloc(1, sizeof(*data));
- if (qxl->playback_fifo_dir[0] == 0)
- {
+ if (qxl->playback_fifo_dir[0] == 0) {
ErrorF("playback: no audio FIFO directory, audio is disabled\n");
return 0;
}
@@ -346,9 +456,24 @@ qxl_add_spice_playback_interface (qxl_screen_t *qxl)
#endif
- ret = pthread_create(&qxl->audio_thread, NULL, &audio_thread_main, qxl);
- if (ret < 0)
+
+ qxl->playback_opaque = data;
+ audio_initialize(qxl);
+
+ data->wall_timer = qxl->core->timer_add(wall_ticker, qxl);
+
+ data->dir_watch = inotify_init1(IN_NONBLOCK);
+ data->fifo_dir_watch = -1;
+ if (data->dir_watch >= 0)
+ data->fifo_dir_watch = inotify_add_watch(data->dir_watch, qxl->playback_fifo_dir, IN_CREATE | IN_MOVE);
+
+ if (data->fifo_dir_watch == -1) {
+ ErrorF("Error %s(%d) watching the fifo dir\n", strerror(errno), errno);
return errno;
+ }
+
+ data->fifo_dir_qxl_watch = qxl->core->watch_add(data->dir_watch,
+ SPICE_WATCH_EVENT_READ, playback_dir_changed, qxl);
return 0;
}
--
1.7.10.4
More information about the Spice-devel
mailing list