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

[Frediano Ziglio]

> 
> SPICE expects 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, confuses the client which burns CPU
> till it fails and keeps complaining:
>   "GSpice-CRITICAL **: 15:53:36.984: need more input data"
> 
> 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 at redhat.com>

Acked-by: Frediano Ziglio <fziglio at redhat.com>

> ---
> 
> Since v2: (addressing Frediano's comments)
>   - s/mjpeg/MJPEG/
>   - removed "dev->hdr.size = 0;" as the function returns true
>   - removed an extra empty line
>   - reword a bit commit log
> 
> 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.
> 
> ---
>  server/red-stream-device.c | 47 ++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> index f280a089f..6a2b6a73a 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,38 @@ 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);
> +
> +    return true;
>  }
>  
>  /*