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

[Christophe de Dinechin]

> On 14 Jun 2018, at 15:03, Uri Lublin <uril at redhat.com> wrote:
> 
> 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.

I prefer splitting, or else having a very visible error out so that someone notices and fixes. An uncompressed 4K frame is roughly 32MB. Not that we’d ever want to send that directly, but this is a limit we may hit in the lifetime of the protocol.

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