>
> 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@xxxxxxxxxx>
> >> ---
> >>
> >> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel