[Spice-devel] [PATCH] stream-device: handle_data: send whole message

[Frediano Ziglio]

> 
> On 06/14/2018 12:56 PM, Frediano Ziglio wrote:
> >>
> >> SPICE expect to have each frame in a single message.
> >> So far the stream-device did not do that.
> >> That works fine for H264 streams but not for mjpeg.
> >> The client handles by itself mjpeg streams, and not via
> >> gstreamer, and is assuming that a message contains the
> >> whole frame. Since it currently not, using spice-streaming-agent
> >> with mjpeg plugin, it confuses the client which burns CPU
> >> till it fails.
> >>
> >> This patch fixes that, but reading the whole message from the
> >> device (the streaming agent) and sending it over to the client.
> >>
> >> BTW, why realloc up and realloc down for every message ?
> >>
> > 
> > I don't know, before this patch it was done only for cursor shape which
> > should be not that often. So the question is why you want to do it
> > for every message?
> 
> Yes, before it was done only for cursor shape.
> 
> I do not want to do it for every message.
> I think we better leave the "big" buffer as is, possibly
> shrink it upon reset/disconnect.
> 
> > 
> >> Signed-off-by: Uri Lublin <uril at redhat.com>
> >> ---
> >>
> >> This is an alternative solution to Frediano's patch
> >> "stream-channel: Send the full frame in a single message" (v3)
> >>
> >> The 32MB limitation is the same as in Frediano's patch.
> >> Perhaps it's too big (?)
> >>
> > 
> > That's slightly different, this patch hard limit to 32mb considering
> > more than 32mb a protocol error, on my patch was falling back to
> > split.
> 
> That's indeed a difference between the two patches.
> I'm not sure what's the right thing to do.
> If you split the message, the client loses its mind
> (when an mjpeg/no-gstreamer codec is used).
> 
> > 
> >> The BTW above should have appeared here, next time ...
> >> Also why use realloc at all, and not free + alloc (no memcpy) ?
> >>
> > 
> > For the shrink case, considered not so often and considering
> > the buffer so small a single function call can be quicker, the
> > allocator is free to reuse the same memory buffer.
> > For expanding with same considerations (not so often and small
> > starting buffer) more or less the same.
> 
> But that also adds a memcpy of memory we do not care about.
> 
> > 
> >> ---
> >>   server/red-stream-device.c | 40 +++++++++++++++++++++++++-------------
> >>   1 file changed, 27 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> >> index df6a366f4..432632f14 100644
> >> --- a/server/red-stream-device.c
> >> +++ b/server/red-stream-device.c
> >> @@ -146,7 +146,11 @@ stream_device_partial_read(StreamDevice *dev,
> >> SpiceCharDeviceInstance *sin)
> >>           }
> >>           break;
> >>       case STREAM_TYPE_DATA:
> >> -        handled = handle_msg_data(dev, sin);
> >> +        if (dev->hdr.size > 32*1024*1024) {
> >> +            handled = handle_msg_invalid(dev, sin, "STREAM_DATA too
> >> large");
> >> +        } else {
> >> +            handled = handle_msg_data(dev, sin);
> >> +        }
> >>           break;
> >>       case STREAM_TYPE_CURSOR_SET:
> >>           handled = handle_msg_cursor_set(dev, sin);
> >> @@ -308,20 +312,30 @@ handle_msg_data(StreamDevice *dev,
> >> SpiceCharDeviceInstance *sin)
> >>           spice_assert(dev->hdr.type == STREAM_TYPE_DATA);
> >>       }
> >>   
> >> -    while (1) {
> >> -        uint8_t buf[16 * 1024];
> >> -        n = sif->read(sin, buf, MIN(sizeof(buf), dev->hdr.size));
> >> -        if (n <= 0) {
> >> -            break;
> >> -        }
> >> -        // TODO collect all message ??
> >> -        // up: we send a single frame together
> >> -        // down: guest can cause a crash
> >> -        stream_channel_send_data(dev->stream_channel, buf, n,
> >> reds_get_mm_time());
> >> -        dev->hdr.size -= n;
> >> +    /* make sure we have a large enough buffer for the whole frame */
> >> +    if (dev->msg_pos == 0 && dev->msg_len < dev->hdr.size) {
> >> +        g_free(dev->msg);
> >> +        dev->msg = g_malloc(dev->hdr.size);
> >> +        dev->msg_len = dev->hdr.size;
> >>       }
> >>   
> >> -    return dev->hdr.size == 0;
> >> +    /* read from device */
> >> +    n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
> >> dev->msg_pos);
> >> +    if (n <= 0) {
> >> +        return false;
> >> +    }
> >> +
> >> +    dev->msg_pos += n;
> >> +    if (dev->msg_pos != dev->hdr.size) { /* some bytes are still missing
> >> */
> >> +        return false;
> >> +    }
> >> +
> >> +    /* The whole frame was read from the device, send it */
> >> +    stream_channel_send_data(dev->stream_channel, dev->msg->buf,
> >> dev->hdr.size, reds_get_mm_time());
> > 
> > OT, not a regression, the time should be taken when the guest starts
> > sending the message, not at the end.
> 
> Yes.
> 
> > 
> >> +    dev->hdr.size = 0;
> >> +
> >> +
> >> +    return true;
> >>   }
> >>   
> >>   /*
> > 
> > Note that this patch is increasing processing latency (as probably
> > mine).
> 
> I know. The solution of reading the whole frame before sending it
> (in other words waiting for the last byte of the frame to arrive),
> is adding latency. We can think of other solutions to this problem.
> 
> Uri.
> 

That's why I prefer the code to collapse reading being in StreamChannel,
StreamChannel knows the clients, which messages they support and how to
send the information.

Maybe a patch mixing the two? Kind of:
- defining a message limit (we agree 32Mb is sensible, is about 20 Mpixel
  of uncompressed jpeg);
- collapse message in stream-channel.c and send. Later support different
  way (either multiple partial messages or pipe through).

Frediano