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

[Christophe Fergeau]

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.

> > > >  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). 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?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180612/5ee58d42/attachment.sig>