A couple thoughts inline below.
On Sat, Oct 18, 2014 at 11:37:13AM -0500, Jeremy White wrote:
> 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@xxxxxxxxxxxxxxx>
> ---
> 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;
One of these seems redundant. Usually for a ring buffer you'd just
have a read offset and an valid data count.
I'm also not a fan of 'len'. I'd probably call it valid, or filled, or
something.
> + 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;
Moving the data around within the ring buffer seems to defeat the
purpose of a ring buffer... You'll need logic to do two reads (for
example), but you'll avoid a memcpy and it greatly simplifies the
increment/decrement logic.
> }
>
> - 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++) {
I'd use sizeof(int16_t) to make it obvious you're processing a number
of samples.
> + int16_t mix;
> +
> + fifo_remove_data(f, (unsigned char *) &mix, sizeof(mix));
This seems like an awful lot of work just to retrieve a single sample.
Much better to read the array directly to avoid copying, and do the
offset/length calculations in bulk (once per mix_in_one_fifo).
> +
> + /* 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;
The return value isn't actually used, so you could get rid of this
magic number.
> +}
>
> - 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;
This looks suspicious. What's the relationship between BUFFER_PERIODS
and the spice buffer chunk size? Are chunks always 10ms? This is why
I held spice buffers between period callbacks in the old code.
Seems like we should only increment 'ahead' when we have submitted
10ms worth of data (no relation to spice chunk size) OR we have hit an
underrun (in which case we've really submitted X ms of data and 10-X
ms of silence).
I remember this part being particularly hard to tune, though, so go
with what works...
> }
>
> - 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);
Would it make sense to only call condense_fifos() at the start of
handle_one_change()? Then we avoid the for-loop every period callback.
>
> - 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;
We leak 'data' here.
> }
> @@ -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;
> + }
Should the stuff remain initialized (e.g. wall_timer, dir_watch) if
this errors? I don't know. Maybe it doesn't matter.
> +
> + 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel