Hey,
For some reason, I was finding this quite hard to review until I split
it in some preparation patch with no behaviour change followed by
the actual changes, see the attachement for how I split things, though
some small details might be wrong.
On Wed, Mar 07, 2018 at 12:01:49PM +0000, Frediano Ziglio wrote:
> The current implementation of server and client assumes that a single
> data message contains an encoded frame.
> This is not a problem for most encoding but for MJPEG this causes
> the client to fail decoding.
> Collapse frame data into a single message before sending to the client.
> This is done in the channel as the channel code is responsible to take care
> of client protocol details. This allows for instance to support chunked
> transfer to client if implemented.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/stream-channel.c | 99 ++++++++++++++++++++++++++++++++++++++++++-------
> server/stream-channel.h | 12 ++++++
> server/stream-device.c | 10 +++--
> 3 files changed, 103 insertions(+), 18 deletions(-)
>
> Changes since v1:
> - explain why is imple,mented that way;
> - read multimedia time only once for first chunk of data.
>
> diff --git a/server/stream-channel.c b/server/stream-channel.c
> index 88f859f6..3a3b733f 100644
> --- a/server/stream-channel.c
> +++ b/server/stream-channel.c
> @@ -44,6 +44,7 @@
>
> typedef struct StreamChannelClient StreamChannelClient;
> typedef struct StreamChannelClientClass StreamChannelClientClass;
> +typedef struct StreamDataItem StreamDataItem;
>
> /* we need to inherit from CommonGraphicsChannelClient
> * to get buffer handling */
> @@ -74,6 +75,10 @@ struct StreamChannel {
>
> StreamQueueStat queue_stat;
>
> + /* pending partial data item */
> + StreamDataItem *data_item;
> + uint32_t data_item_pos;
> +
> /* callback to notify when a stream should be started or stopped */
> stream_channel_start_proc start_cb;
> void *start_opaque;
> @@ -104,12 +109,12 @@ typedef struct StreamCreateItem {
> SpiceMsgDisplayStreamCreate stream_create;
> } StreamCreateItem;
>
> -typedef struct StreamDataItem {
> +struct StreamDataItem {
> RedPipeItem base;
> StreamChannel *channel;
> // NOTE: this must be the last field in the structure
> SpiceMsgDisplayStreamData data;
> -} StreamDataItem;
> +};
>
> #define PRIMARY_SURFACE_ID 0
>
> @@ -129,6 +134,16 @@ stream_channel_client_init(StreamChannelClient *client)
> client->stream_id = -1;
> }
>
> +static void
> +stream_channel_unref_data_item(StreamChannel *channel)
> +{
> + if (channel->data_item) {
> + red_pipe_item_unref(&channel->data_item->base);
> + channel->data_item = NULL;
> + channel->data_item_pos = 0;
> + }
> +}
> +
> static void
> request_new_stream(StreamChannel *channel, StreamMsgStartStop *start)
> {
> @@ -152,6 +167,7 @@ stream_channel_client_on_disconnect(RedChannelClient *rcc)
> channel->stream_id = -1;
> channel->width = 0;
> channel->height = 0;
> + stream_channel_unref_data_item(channel);
>
> // send stream stop to device
> StreamMsgStartStop stop = { 0, };
> @@ -424,6 +440,16 @@ stream_channel_constructed(GObject *object)
> reds_register_channel(reds, red_channel);
> }
>
> +static void
> +stream_channel_finalize(GObject *object)
> +{
> + StreamChannel *channel = STREAM_CHANNEL(object);
> +
> + stream_channel_unref_data_item(channel);
> +
> + G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
> +}
> +
> static void
> stream_channel_class_init(StreamChannelClass *klass)
> {
> @@ -431,6 +457,7 @@ stream_channel_class_init(StreamChannelClass *klass)
> RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
>
> object_class->constructed = stream_channel_constructed;
> + object_class->finalize = stream_channel_finalize;
>
> channel_class->parser = spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
> channel_class->handle_message = handle_message;
> @@ -503,11 +530,46 @@ data_item_free(RedPipeItem *base)
> {
> StreamDataItem *pipe_item = SPICE_UPCAST(StreamDataItem, base);
>
> - stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
> + if (pipe_item->channel->data_item != pipe_item) {
> + stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
> + }
>
> g_free(pipe_item);
> }
>
> +static StreamDataItem*
> +stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_time)
My main comment about this patch will be regarding that method, which
returns a new item, but actually it's stored in the StreamChannel. I'd
suggest something like this instead (on top of your patch):
diff --git a/server/stream-channel.c b/server/stream-channel.c
index 75dd1f867..3c8904233 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -537,8 +537,8 @@ data_item_free(RedPipeItem *base)
g_free(pipe_item);
}
-static StreamDataItem*
-stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_time)
+static void
+stream_channel_init_data_item(StreamChannel *channel, size_t size, uint32_t mm_time)
{
stream_channel_unref_data_item(channel);
@@ -552,8 +552,6 @@ stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_ti
channel->data_item = item;
channel->data_item_pos = 0;
-
- return item;
}
void
@@ -567,7 +565,7 @@ stream_channel_start_data(StreamChannel *channel, size_t size, uint32_t mm_time)
// TODO this collects all chunks in a single message
// up: we send a single frame together (more compatible)
// down: guest can cause a crash due to DoS. As a safe measure we limit the maximum message
- stream_channel_new_data_item(channel, MIN(size, 32*1024*1024), mm_time);
+ stream_channel_init_data_item(channel, MIN(size, 32*1024*1024), mm_time);
}
void
@@ -583,12 +581,11 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
}
while (size) {
+ if (channel->data_item == NULL) {
+ stream_channel_init_data_item(channel, size, mm_time);
+ }
+
StreamDataItem *item = channel->data_item;
-
- if (!item) {
- item = stream_channel_new_data_item(channel, size, mm_time);
- }
-
size_t copy_size = item->data.data_size - channel->data_item_pos;
copy_size = MIN(copy_size, size);
// TODO try to optimize avoiding the copy
Christophe
> +{
> + stream_channel_unref_data_item(channel);
> +
> + StreamDataItem *item = g_malloc(sizeof(*item) + size);
> + red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_STREAM_DATA,
> + data_item_free);
> + item->data.base.id = channel->stream_id;
> + item->data.base.multi_media_time = mm_time;
> + item->data.data_size = size;
> + item->channel = channel;
> +
> + channel->data_item = item;
> + channel->data_item_pos = 0;
> +
> + return item;
> +}
> +
> +void
> +stream_channel_start_data(StreamChannel *channel, size_t size, uint32_t mm_time)
> +{
> + // see stream_channel_send_data comment
> + if (channel->stream_id < 0) {
> + return;
> + }
> +
> + // TODO this collects all chunks in a single message
> + // up: we send a single frame together (more compatible)
> + // down: guest can cause a crash due to DoS. As a safe measure we limit the maximum message
> + stream_channel_new_data_item(channel, MIN(size, 32*1024*1024), mm_time);
> +}
> +
> void
> stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
> {
> @@ -520,17 +582,25 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
>
> RedChannel *red_channel = RED_CHANNEL(channel);
>
> - StreamDataItem *item = g_malloc(sizeof(*item) + size);
> - red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_STREAM_DATA,
> - data_item_free);
> - item->data.base.id = channel->stream_id;
> - item->data.base.multi_media_time = mm_time;
> - item->data.data_size = size;
> - item->channel = channel;
> - stream_channel_update_queue_stat(channel, 1, size);
> - // TODO try to optimize avoiding the copy
> - memcpy(item->data.data, data, size);
> - red_channel_pipes_add(red_channel, &item->base);
> + while (size) {
> + StreamDataItem *item = channel->data_item;
> +
> + if (!item) {
> + item = stream_channel_new_data_item(channel, size, mm_time);
> + }
> +
> + size_t copy_size = item->data.data_size - channel->data_item_pos;
> + copy_size = MIN(copy_size, size);
> + // TODO try to optimize avoiding the copy
> + memcpy(item->data.data + channel->data_item_pos, data, copy_size);
> + size -= copy_size;
> + channel->data_item_pos += copy_size;
> + if (channel->data_item_pos == item->data.data_size) {
> + channel->data_item = NULL;
> + stream_channel_update_queue_stat(channel, 1, item->data.data_size);
> + red_channel_pipes_add(red_channel, &item->base);
> + }
> + }
> }
>
> void
> @@ -570,6 +640,7 @@ stream_channel_reset(StreamChannel *channel)
> channel->stream_id = -1;
> channel->width = 0;
> channel->height = 0;
> + stream_channel_unref_data_item(channel);
>
> if (!red_channel_is_connected(red_channel)) {
> return;
> diff --git a/server/stream-channel.h b/server/stream-channel.h
> index e8bec80b..18a1bdea 100644
> --- a/server/stream-channel.h
> +++ b/server/stream-channel.h
> @@ -60,6 +60,18 @@ struct StreamMsgStartStop;
>
> void stream_channel_change_format(StreamChannel *channel,
> const struct StreamMsgFormat *fmt);
> +
> +/**
> + * Tell the channel that a new data packet is starting.
> + * This can be used to group all chunks together.
> + */
> +void stream_channel_start_data(StreamChannel *channel,
> + size_t size,
> + uint32_t mm_time);
> +
> +/**
> + * Send to channel a chunk of data.
> + */
> void stream_channel_send_data(StreamChannel *channel,
> const void *data, size_t size,
> uint32_t mm_time);
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 6cf29d37..c84ebfac 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -273,11 +273,13 @@ handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> 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());
> + uint32_t mm_time = reds_get_mm_time();
> + if (dev->msg_pos == 0) {
> + stream_channel_start_data(dev->stream_channel, dev->hdr.size, mm_time);
> + }
> + stream_channel_send_data(dev->stream_channel, buf, n, mm_time);
> dev->hdr.size -= n;
> + dev->msg_pos += n;
> }
>
> return dev->hdr.size == 0;
> --
> 2.14.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
From f3b8d49752949c8b24efb3bc6515cb31cc81ebb0 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Fri, 16 Mar 2018 11:31:41 +0100
Subject: [spice-server 1/3] Preparation work
---
server/stream-channel.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/server/stream-channel.c b/server/stream-channel.c
index 88f859f6d..3fab28ae1 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -508,17 +508,10 @@ data_item_free(RedPipeItem *base)
g_free(pipe_item);
}
-void
-stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
+static StreamDataItem*
+stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_time)
{
- if (channel->stream_id < 0) {
- // this condition can happen if the guest didn't handle
- // the format stop that we send so think the stream is still
- // started
- return;
- }
-
- RedChannel *red_channel = RED_CHANNEL(channel);
+ stream_channel_unref_data_item(channel);
StreamDataItem *item = g_malloc(sizeof(*item) + size);
red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_STREAM_DATA,
@@ -527,9 +520,29 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
item->data.base.multi_media_time = mm_time;
item->data.data_size = size;
item->channel = channel;
- stream_channel_update_queue_stat(channel, 1, size);
+
+ return item;
+}
+
+void
+stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
+{
+ RedChannel *red_channel = RED_CHANNEL(channel);
+
+ if (channel->stream_id < 0) {
+ // this condition can happen if the guest didn't handle
+ // the format stop that we send so think the stream is still
+ // started
+ return;
+ }
+
+ StreamDataItem *item;
+
+ item = stream_channel_new_data_item(channel, size, mm_time);
+
// TODO try to optimize avoiding the copy
memcpy(item->data.data, data, size);
+ stream_channel_update_queue_stat(channel, 1, size);
red_channel_pipes_add(red_channel, &item->base);
}
--
2.14.3
From c8c204c5482e3bfa0038ea9829fb96923274ee29 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Fri, 16 Mar 2018 11:44:21 +0100
Subject: [spice-server 2/3] more preparation work
---
server/stream-channel.c | 57 +++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 9 deletions(-)
diff --git a/server/stream-channel.c b/server/stream-channel.c
index 3fab28ae1..b5652e5ff 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -44,6 +44,7 @@
typedef struct StreamChannelClient StreamChannelClient;
typedef struct StreamChannelClientClass StreamChannelClientClass;
+typedef struct StreamDataItem StreamDataItem;
/* we need to inherit from CommonGraphicsChannelClient
* to get buffer handling */
@@ -74,6 +75,10 @@ struct StreamChannel {
StreamQueueStat queue_stat;
+ /* pending partial data item */
+ StreamDataItem *data_item;
+ uint32_t data_item_pos;
+
/* callback to notify when a stream should be started or stopped */
stream_channel_start_proc start_cb;
void *start_opaque;
@@ -104,12 +109,12 @@ typedef struct StreamCreateItem {
SpiceMsgDisplayStreamCreate stream_create;
} StreamCreateItem;
-typedef struct StreamDataItem {
+struct StreamDataItem {
RedPipeItem base;
StreamChannel *channel;
// NOTE: this must be the last field in the structure
SpiceMsgDisplayStreamData data;
-} StreamDataItem;
+};
#define PRIMARY_SURFACE_ID 0
@@ -129,6 +134,16 @@ stream_channel_client_init(StreamChannelClient *client)
client->stream_id = -1;
}
+static void
+stream_channel_unref_data_item(StreamChannel *channel)
+{
+ if (channel->data_item) {
+ red_pipe_item_unref(&channel->data_item->base);
+ channel->data_item = NULL;
+ channel->data_item_pos = 0;
+ }
+}
+
static void
request_new_stream(StreamChannel *channel, StreamMsgStartStop *start)
{
@@ -152,6 +167,7 @@ stream_channel_client_on_disconnect(RedChannelClient *rcc)
channel->stream_id = -1;
channel->width = 0;
channel->height = 0;
+ stream_channel_unref_data_item(channel);
// send stream stop to device
StreamMsgStartStop stop = { 0, };
@@ -424,6 +440,16 @@ stream_channel_constructed(GObject *object)
reds_register_channel(reds, red_channel);
}
+static void
+stream_channel_finalize(GObject *object)
+{
+ StreamChannel *channel = STREAM_CHANNEL(object);
+
+ stream_channel_unref_data_item(channel);
+
+ G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
+}
+
static void
stream_channel_class_init(StreamChannelClass *klass)
{
@@ -431,6 +457,7 @@ stream_channel_class_init(StreamChannelClass *klass)
RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
object_class->constructed = stream_channel_constructed;
+ object_class->finalize = stream_channel_finalize;
channel_class->parser = spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
channel_class->handle_message = handle_message;
@@ -503,7 +530,9 @@ data_item_free(RedPipeItem *base)
{
StreamDataItem *pipe_item = SPICE_UPCAST(StreamDataItem, base);
- stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
+ if (pipe_item->channel->data_item != pipe_item) {
+ stream_channel_update_queue_stat(pipe_item->channel, -1, -pipe_item->data.data_size);
+ }
g_free(pipe_item);
}
@@ -521,6 +550,9 @@ stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_ti
item->data.data_size = size;
item->channel = channel;
+ channel->data_item = item;
+ channel->data_item_pos = 0;
+
return item;
}
@@ -536,14 +568,20 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
return;
}
- StreamDataItem *item;
+ while (size) {
+ StreamDataItem *item = channel->data_item;
- item = stream_channel_new_data_item(channel, size, mm_time);
+ if (!item) {
+ item = stream_channel_new_data_item(channel, size, mm_time);
+ }
- // TODO try to optimize avoiding the copy
- memcpy(item->data.data, data, size);
- stream_channel_update_queue_stat(channel, 1, size);
- red_channel_pipes_add(red_channel, &item->base);
+ size_t copy_size = size;
+ // TODO try to optimize avoiding the copy
+ memcpy(item->data.data, data, copy_size);
+ size -= copy_size;
+ stream_channel_update_queue_stat(channel, 1, size);
+ red_channel_pipes_add(red_channel, &item->base);
+ }
}
void
@@ -583,6 +621,7 @@ stream_channel_reset(StreamChannel *channel)
channel->stream_id = -1;
channel->width = 0;
channel->height = 0;
+ stream_channel_unref_data_item(channel);
if (!red_channel_is_connected(red_channel)) {
return;
--
2.14.3
From 43b50f11428313d395c7c01bb3c93356c76f9ac8 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Wed, 7 Mar 2018 12:01:49 +0000
Subject: [spice-server 3/3] stream-channel: Send the full frame in a single
message
The current implementation of server and client assumes that a single
data message contains an encoded frame.
This is not a problem for most encoding but for MJPEG this causes
the client to fail decoding.
Collapse frame data into a single message before sending to the client.
This is done in the channel as the channel code is responsible to take care
of client protocol details. This allows for instance to support chunked
transfer to client if implemented.
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
---
server/red-stream-device.c | 10 ++++++----
server/stream-channel.c | 27 +++++++++++++++++++++++----
server/stream-channel.h | 12 ++++++++++++
3 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index c87c4899d..81336e549 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -271,11 +271,13 @@ handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
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());
+ uint32_t mm_time = reds_get_mm_time();
+ if (dev->msg_pos == 0) {
+ stream_channel_start_data(dev->stream_channel, dev->hdr.size, mm_time);
+ }
+ stream_channel_send_data(dev->stream_channel, buf, n, mm_time);
dev->hdr.size -= n;
+ dev->msg_pos += n;
}
return dev->hdr.size == 0;
diff --git a/server/stream-channel.c b/server/stream-channel.c
index b5652e5ff..75dd1f867 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -556,6 +556,20 @@ stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t mm_ti
return item;
}
+void
+stream_channel_start_data(StreamChannel *channel, size_t size, uint32_t mm_time)
+{
+ // see stream_channel_send_data comment
+ if (channel->stream_id < 0) {
+ return;
+ }
+
+ // TODO this collects all chunks in a single message
+ // up: we send a single frame together (more compatible)
+ // down: guest can cause a crash due to DoS. As a safe measure we limit the maximum message
+ stream_channel_new_data_item(channel, MIN(size, 32*1024*1024), mm_time);
+}
+
void
stream_channel_send_data(StreamChannel *channel, const void *data, size_t size, uint32_t mm_time)
{
@@ -575,12 +589,17 @@ stream_channel_send_data(StreamChannel *channel, const void *data, size_t size,
item = stream_channel_new_data_item(channel, size, mm_time);
}
- size_t copy_size = size;
+ size_t copy_size = item->data.data_size - channel->data_item_pos;
+ copy_size = MIN(copy_size, size);
// TODO try to optimize avoiding the copy
- memcpy(item->data.data, data, copy_size);
+ memcpy(item->data.data + channel->data_item_pos, data, copy_size);
size -= copy_size;
- stream_channel_update_queue_stat(channel, 1, size);
- red_channel_pipes_add(red_channel, &item->base);
+ channel->data_item_pos += copy_size;
+ if (channel->data_item_pos == item->data.data_size) {
+ channel->data_item = NULL;
+ stream_channel_update_queue_stat(channel, 1, item->data.data_size);
+ red_channel_pipes_add(red_channel, &item->base);
+ }
}
}
diff --git a/server/stream-channel.h b/server/stream-channel.h
index e8bec80bd..18a1bdead 100644
--- a/server/stream-channel.h
+++ b/server/stream-channel.h
@@ -60,6 +60,18 @@ struct StreamMsgStartStop;
void stream_channel_change_format(StreamChannel *channel,
const struct StreamMsgFormat *fmt);
+
+/**
+ * Tell the channel that a new data packet is starting.
+ * This can be used to group all chunks together.
+ */
+void stream_channel_start_data(StreamChannel *channel,
+ size_t size,
+ uint32_t mm_time);
+
+/**
+ * Send to channel a chunk of data.
+ */
void stream_channel_send_data(StreamChannel *channel,
const void *data, size_t size,
uint32_t mm_time);
--
2.14.3
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel