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

[Uri Lublin]

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>
---

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;
 }
 
 /*
-- 
2.17.1