[Spice-devel] [PATCH spice-server v3 2/2] stream-channel: Send the full frame in a single message

[Lukáš Hrázký]

On Tue, 2018-06-12 at 13:30 +0200, Christophe Fergeau wrote:
> Hey,
> 
> On Wed, May 09, 2018 at 01:20:19PM +0200, Lukáš Hrázký wrote:
> > On Wed, 2018-05-09 at 05:18 -0400, Frediano Ziglio wrote:
> > > The agent send a single message, but reads/writes to the device are
> > > not atomic. Note that the current protocol introduce additional
> > > delays as the frames cannot be partially decoded but must wait for the
> > > full message (maybe the client can change its read code to handle this,
> > > at the moment it does nothing about, on the server is less of a problem
> > > as the message is build quickly from device to memory so not much delay
> > > is added).
> > 
> > This is the part I don't understand... AFAICS, you read the whole
> > message in red-stream-device.c:handle_msg_data(). That should be the
> > whole frame? Then you send the whole frame with
> > stream_channel_send_data(). So it should never be partial?
> 
> I would guess the data you get in handle_msg_data() got fragmented
> somewhere on the agent -> kernel -> virtio-serial -> qemu -> spice way.
> So one mjpeg frame will correspond to multiple calls to
>         n = sif->read(sin, buf, MIN(sizeof(buf), dev->hdr.size));
> Then with this patch, stream_channel_send_data() will coalesce all these
> reads in a single StreamDataItem and send it when its size corresponds
> to dev->hdr.size.

Yah, figured that out when I revisited this few days ago after a
discussion with Uri. He mentioned he's got another patch for it, though
I'm not sure where it is (I may have missed it).

> > > > >  void
> > > > > @@ -563,11 +609,25 @@ stream_channel_send_data(StreamChannel *channel,
> > > > > const void *data, size_t size,
> > > > >  
> > > > >      RedChannel *red_channel = RED_CHANNEL(channel);
> > > > >  
> > > > > -    StreamDataItem *item = stream_data_item_new(channel, size, mm_time);
> > > > > -    stream_channel_update_queue_stat(channel, 1, size);
> > > > > -    // TODO try to optimize avoiding the copy
> > > > > -    memcpy(item->data.data, data, size);
> > > > > -    red_channel_pipes_add(red_channel, &item->base);
> > > > > +    while (size) {
> > > > > +        if (channel->data_item == NULL) {
> > > > > +            stream_channel_init_data_item(channel, size, mm_time);
> > > > > +        }
> > > > > +
> > > > > +        StreamDataItem *item = channel->data_item;
> > > > > +
> > > > > +        size_t copy_size = item->data.data_size - channel->data_item_pos;
> > > > > +        copy_size = MIN(copy_size, size);
> > > > > +        // TODO try to optimize avoiding the copy
> > > > > +        memcpy(item->data.data + channel->data_item_pos, data, copy_size);
> > > > > +        size -= copy_size;
> > > > > +        channel->data_item_pos += copy_size;
> > > > > +        if (channel->data_item_pos == item->data.data_size) {
> > > > > +            channel->data_item = NULL;
> > > > > +            stream_channel_update_queue_stat(channel, 1,
> > > > > item->data.data_size);
> > > > > +            red_channel_pipes_add(red_channel, &item->base);
> > > > > +        }
> > > > > +    }
> > 
> > What does the while (size) loop do here? It will do more than one
> > iteration only if copy_size < size, which means there is not enough
> > space in the item buffer and in that case it seems to me it will loop
> > forever? Am I missing something?
> 
> I don't think it will loop forever (channel->data_item will be set to
> NULL, and then a new one will be created on the next iteration, and this
> one will be sent, and at that point, size will be 0).

Same as above... figured it out, not sure what I was thinking at the
time :)

> However, I'm also
> slightly confused as for the intent of that loop. Maybe to handle the
> case when we receive more data than we expect?

Yeah, come look at it once again (unless I'm wrong again) seems you're
right :) I don't think we can actually receive more data for the
message than what was the size in the header? Because the size defines
the end of the message? So:

1. The loop should never do more than one iteration.

2. If it actually does, I think it will fragment the message once again
(sending two messages for a single frame), so the bug with the client
persists.

I was also thinking the ideal way of handling this would be to "pipe
through" the data, i.e. send a header with the full data message size
to the streaming channel once a data message header arrives on the
streaming device and then send whatever fragments of the message arrive
on the device, until the message is complete. But I think that's not
possible with the channel interface?

Cheers,
Lukas

> Christophe