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

Wed Jun 13 12:20:47 UTC 2018

On Wed, 2018-06-13 at 07:13 -0400, Frediano Ziglio wrote:
> > 
> > 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.
> > 
> 
> Does in case the guest wants to send very large message, in this case
> is limited to 32Mb.
> 
> > 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.
> > 
> 
> Simply not expected to be a regression in this case.

I'm not following you here. Do you mean that you do not expect this to
happen? If so, and if the code actually doesn't work for that case, the
code doesn't need to be there...

> We need a single message for frame only with Mjpeg.

I don't understand this either. AFAIK the frame data messages are
handled the same regardless of the codec? My understanding was that the
Mjpeg messages are a bit longer so that they actually trigger the bug?

I think this bug is actually present for all the messages received by
the server, it's just that most of them are too short to trigger it.
I'm not sure we should rely on it though.

> > 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?
> > 
> 
> Yes, though so and would be great. Currently the RedChannelClient requires
> each item to return a marshaller with all data and will send data from
> the marshaller. RCC does not allow to "pause" with data not available.
> 
> > Cheers,
> > Lukas
> > 
> > > Christophe
> 
> Frediano