On Wed, 2017-06-14 at 16:40 +0100, Frediano Ziglio wrote:
> Handle stream data from device sending to the channel.
> Channel will forward to clients as proper DisplayChannel
> messaging creating and destroying the channel stream as
> needed.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/stream-channel.c | 107
> ++++++++++++++++++++++++++++++++++++++++++++++++
> server/stream-channel.h | 6 +++
> server/stream-device.c | 4 +-
> 3 files changed, 115 insertions(+), 2 deletions(-)
>
> diff --git a/server/stream-channel.c b/server/stream-channel.c
> index 7f4dad7..119ef94 100644
> --- a/server/stream-channel.c
> +++ b/server/stream-channel.c
> @@ -20,6 +20,7 @@
> #endif
>
> #include <common/generated_server_marshallers.h>
> +#include <spice/stream-device.h>
>
> #include "red-channel-client.h"
> #include "stream-channel.h"
> @@ -46,6 +47,8 @@ typedef struct StreamChannelClientClass
> StreamChannelClientClass;
> * to get buffer handling */
> struct StreamChannelClient {
> CommonGraphicsChannelClient parent;
> +
> + int stream_id;
> };
>
> struct StreamChannelClientClass {
> @@ -58,6 +61,10 @@ G_DEFINE_TYPE(StreamChannelClient,
> stream_channel_client, TYPE_COMMON_GRAPHICS_C
>
> struct StreamChannel {
> RedChannel parent;
> +
> + /* current video stream id, <0 if not initialized or
> + * we are not sending a stream */
> + int stream_id;
> };
>
> struct StreamChannelClass {
> @@ -69,8 +76,22 @@ G_DEFINE_TYPE(StreamChannel, stream_channel,
> RED_TYPE_CHANNEL)
> enum {
> RED_PIPE_ITEM_TYPE_SURFACE_CREATE =
> RED_PIPE_ITEM_TYPE_COMMON_LAST,
> RED_PIPE_ITEM_TYPE_FILL_SURFACE,
> + RED_PIPE_ITEM_TYPE_STREAM_CREATE,
> + RED_PIPE_ITEM_TYPE_STREAM_DATA,
> + RED_PIPE_ITEM_TYPE_STREAM_DESTROY,
> };
>
> +typedef struct StreamCreateItem {
> + RedPipeItem base;
> + SpiceMsgDisplayStreamCreate stream_create;
> +} StreamCreateItem;
> +
> +typedef struct StreamDataItem {
> + RedPipeItem base;
> + // NOTE: this must be the last field in the structure
> + SpiceMsgDisplayStreamData data;
> +} StreamDataItem;
> +
> static void
> stream_channel_client_class_init(StreamChannelClientClass *klass)
> {
> @@ -79,6 +100,7 @@
> stream_channel_client_class_init(StreamChannelClientClass *klass)
> static void
> stream_channel_client_init(StreamChannelClient *client)
> {
> + client->stream_id = -1;
> }
>
> static void
> @@ -120,6 +142,7 @@ static void
> stream_channel_send_item(RedChannelClient *rcc, RedPipeItem
> *pipe_item)
> {
> SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> + StreamChannelClient *client = STREAM_CHANNEL_CLIENT(rcc);
>
> switch (pipe_item->type) {
> case RED_PIPE_ITEM_TYPE_SURFACE_CREATE: {
> @@ -141,6 +164,32 @@ stream_channel_send_item(RedChannelClient *rcc,
> RedPipeItem *pipe_item)
> spice_marshall_Fill(m, &fill, &brush_pat_out,
> &mask_bitmap_out);
> break;
> }
> + case RED_PIPE_ITEM_TYPE_STREAM_CREATE: {
> + StreamCreateItem *item = SPICE_UPCAST(StreamCreateItem,
> pipe_item);
> + client->stream_id = item->stream_create.id;
> + red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_CREATE);
> + spice_marshall_msg_display_stream_create(m, &item-
> >stream_create);
> + break;
> + }
> + case RED_PIPE_ITEM_TYPE_STREAM_DATA: {
> + StreamDataItem *item = SPICE_UPCAST(StreamDataItem,
> pipe_item);
> + red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DATA);
> + spice_marshall_msg_display_stream_data(m, &item->data);
> + red_pipe_item_ref(pipe_item);
> + spice_marshaller_add_by_ref_full(m, item->data.data, item-
> >data.data_size,
> + marshaller_unref_pipe_item,
> pipe_item);
> + break;
> + }
> + case RED_PIPE_ITEM_TYPE_STREAM_DESTROY: {
> + if (client->stream_id < 0) {
> + return;
> + }
> + SpiceMsgDisplayStreamDestroy stream_destroy = { client-
> >stream_id };
> + red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DESTROY);
> + spice_marshall_msg_display_stream_destroy(m,
> &stream_destroy);
> + client->stream_id = -1;
> + break;
> + }
> default:
> spice_error("invalid pipe item type");
> }
> @@ -249,4 +298,62 @@ stream_channel_class_init(StreamChannelClass
> *klass)
> static void
> stream_channel_init(StreamChannel *channel)
> {
> + channel->stream_id = -1;
> +}
> +
> +static RedPipeItem *
> +pipe_item_new_ref(G_GNUC_UNUSED RedChannelClient *rcc, void *data,
> G_GNUC_UNUSED int num)
> +{
> + RedPipeItem *item = data;
> + red_pipe_item_ref(item);
> + return item;
> +}
> +
> +void
> +stream_channel_change_format(StreamChannel *channel, const
> StreamMsgFormat *fmt)
> +{
> + RedChannel *red_channel = RED_CHANNEL(channel);
> +
> + // send destroy old stream
> + red_channel_pipes_add_type(red_channel,
> RED_PIPE_ITEM_TYPE_STREAM_DESTROY);
Is there any way to check whether the new format is (for whatever
reason) the same as the old format so that we can avoid destroying and
re-creating a stream in this case? Or is it not worth it?
> +
> + // TODO send new create surface if required
> +
> + // allocate a new stream id
> + channel->stream_id = (channel->stream_id + 1) % 40;
Any particular reason for using 40 here?
> +
> + // send create stream
> + StreamCreateItem *item = spice_new0(StreamCreateItem, 1);
> + red_pipe_item_init(&item->base,
> RED_PIPE_ITEM_TYPE_STREAM_CREATE);
> + item->stream_create.id = channel->stream_id;
> + item->stream_create.flags = SPICE_STREAM_FLAGS_TOP_DOWN;
> + item->stream_create.codec_type = fmt->codec;
> + item->stream_create.stream_width = fmt->width;
> + item->stream_create.stream_height = fmt->height;
> + item->stream_create.src_width = fmt->width;
> + item->stream_create.src_height = fmt->height;
> + item->stream_create.dest = (SpiceRect) { 0, 0, fmt->width, fmt-
> >height };
> + item->stream_create.clip = (SpiceClip) { SPICE_CLIP_TYPE_NONE,
> NULL };
> + red_channel_pipes_new_add(red_channel, pipe_item_new_ref, item);
This is a little bit odd and probably requires some comment. The
intention of the _pipes_new_add() function is that you pass a generator
function to it and it will create a new pipe item for each channel
client. But here you're creating your own pipe item and instead of
passing a generator function, you pass a function that simply takes a
reference. This results in the same pipe item being shared among all
clients rather than a separate pipe item for each client. I don't
anticipate any problems in doing it this way, but it is a little bit
unexpected.
Presumably the reason that this function assumes that we need to create
a separate pipe item for each channel client is because in the past the
pipe items were not refcounted. Now that they are refcounted, perhaps
this API should be changed everywhere. so instead of
void red_channel_pipes_new_add(RedChannel *channel, new_pipe_item_t
creator, void *data);
We could change it to something more like:
void red_channel_pipes_add(RedChannel *channel, RedPipeItem *item);
Where the function would simply take a reference for each channel
client. This is outside of the scope of this patch, but it might be
-worth thinking about in the future?
> + red_pipe_item_unref(&item->base);
> +}
> +
> +void
> +stream_channel_send_data(StreamChannel *channel, const void *data,
> size_t size)
> +{
> + if (channel->stream_id < 0) {
> + return;
> + }
> +
> + RedChannel *red_channel = RED_CHANNEL(channel);
> +
> + StreamDataItem *item = spice_malloc(sizeof(*item) + size);
> + red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_STREAM_DATA);
> + item->data.base.id = channel->stream_id;
> + item->data.base.multi_media_time = reds_get_mm_time();
So, here we are sending a time based on when the server recieved the
data from the guest. It seems that it would be better to use the time
that the data was encoded. Or is this not possible?
> + item->data.data_size = size;
> + // TODO try to optimize avoiding the copy
> + memcpy(item->data.data, data, size);
> + red_channel_pipes_new_add(red_channel, pipe_item_new_ref, item);
> + red_pipe_item_unref(&item->base);
> }
> diff --git a/server/stream-channel.h b/server/stream-channel.h
> index 5691008..b771082 100644
> --- a/server/stream-channel.h
> +++ b/server/stream-channel.h
> @@ -48,6 +48,12 @@ GType stream_channel_get_type(void) G_GNUC_CONST;
> */
> StreamChannel* stream_channel_new(RedsState *server);
>
> +struct StreamMsgFormat;
> +
> +void stream_channel_change_format(StreamChannel *channel,
> + const struct StreamMsgFormat
> *fmt);
> +void stream_channel_send_data(StreamChannel *channel, const void
> *data, size_t size);
> +
> G_END_DECLS
>
> #endif /* STREAM_CHANNEL_H_ */
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 6c4eccb..7b1e00d 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -149,6 +149,7 @@ handle_msg_format(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
> }
> fmt.width = GUINT32_FROM_LE(fmt.width);
> fmt.height = GUINT32_FROM_LE(fmt.height);
> + stream_channel_change_format(dev->channel, &fmt);
> dev->hdr_pos = 0;
> }
>
> @@ -160,11 +161,10 @@ handle_msg_data(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
> while (1) {
> uint8_t buf[16 * 1024];
> n = sif->read(sin, buf, sizeof(buf));
> - /* TODO */
> - spice_debug("readed %d bytes from device", n);
> if (n <= 0) {
> break;
> }
> + stream_channel_send_data(dev->channel, buf, n);
> dev->hdr.size -= n;
> }
> if (dev->hdr.size == 0) {
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel