[Spice-devel] [PATCH xf86-video-qxl] Revise the XSpice audio processing to avoid the use of pthreads.

[Jeremy White]

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