>
> 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, by reading the whole message from the
> device (the streaming agent) and sending it over to the client.
>
> Signed-off-by: Uri Lublin <uril@xxxxxxxxxx>
Paranoia: MJPEG instead of jpeg?
Other paranoia: odd lines breaks.
> ---
>
> Since v1:
> - keep frame_mmtime when frame starts and use it
> - Added TODO for the removal of dev->msg reallocation.
> We better remove that realloc code either in this patch (v3)
> or a following one, since now it happens for each frame
> (and before it happened only for cursor).
>
> Note: I still see on the client side few messages from libjpeg saying:
> "Corrupt JPEG data: premature end of data segment"
> I suspect the spice-streaming-agent mjpeg-plugin.
>
I would not consider this as a stop-over, not a regression.
Maybe you could dump data when this is detected?
> ---
> server/red-stream-device.c | 49 ++++++++++++++++++++++++++++----------
> 1 file changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> index f280a089f..30787328d 100644
> --- a/server/red-stream-device.c
> +++ b/server/red-stream-device.c
> @@ -48,6 +48,7 @@ struct StreamDevice {
> StreamChannel *stream_channel;
> CursorChannel *cursor_channel;
> SpiceTimer *close_timer;
> + uint32_t frame_mmtime;
> };
>
> struct StreamDeviceClass {
> @@ -146,7 +147,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 +313,40 @@ 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;
> + /* make sure we have a large enough buffer for the whole frame */
> + /* ---
> + * TODO: Now that we copy partial data into the buffer, for each frame
> + * the buffer is allocated and freed (look for g_realloc in
> + * stream_device_partial_read).
> + * Probably better to just keep the larger buffer.
> + */
> + if (dev->msg_pos == 0) {
> + dev->frame_mmtime = reds_get_mm_time();
> + if (dev->msg_len < dev->hdr.size) {
> + g_free(dev->msg);
> + dev->msg = g_malloc(dev->hdr.size);
> + dev->msg_len = dev->hdr.size;
> }
> - // 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;
> }
>
> - 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,
> + dev->frame_mmtime);
> + dev->hdr.size = 0;
Why this line? Returning true already prepare for next message.
> +
> +
Why 2 empty lines?
> + return true;
> }
>
> /*
Apart these really minor issues patch seems fine.
OT: Would be great to have an additional test case in test-stream-device
but this is surely a follow up.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel