Hey Andrew,
Thanks for the careful review. Comments in line, and a new patch on the
way.
>> +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.
I've eliminated the 'remove_from', but I really didn't feel a better
synonym for len.
+ 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.
*blush*. That was part of some other logic that no longer makes sense.
Revised.
+ 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.
Sure.
+ 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).
I don't think the copy penalty is so stiff, but I can (and have)
simplified the retrieval process. Note that this will only trigger when
we're mixing more than 1 fifo; ie, it is not an expected mainline case.
+ return s * 2;
The return value isn't actually used, so you could get rid of this
magic number.
Good point; removed.
+ 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.
After our discussion, I've completely reworked this. I believe it is
more clear in the new logic, and it continues to pass my tests.
- 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.
Yep, changed.
+ 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.
Erm. Yeah, I don't know either. There won't be any active code; it'll
just be memory that is leaked. I don't think that's a substantive
change (we already leaked some ram as it was), so I'm going to pass on
this one.
Cheers,
Jeremy
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel