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

[Uri Lublin]

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.