> On 14 Jun 2018, at 15:03, Uri Lublin <uril@xxxxxxxxxx> 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@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.
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel