>
> 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 <fzig...@redhat.com>
> > ---
> > 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?
>
Is not so easy as it seems. First you have to handle the case
a new client is connecting and you start the new stream for it.
Also the case the resolution change. And even if size and format
are the same depending on the format is not guaranteed that you
can do it, you could be sending a partial data then start a new
one; this may work on h264 but not for mjpeg for instance.
> > +
> > + // 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?
>
If I remember laziness... sometimes these ids are used to allocate
arrays so you don't want big numbers, you don't want to have the
same number to avoid mixing the streams, I didn't remember if
DisplayChannel uses 64, 50 or other values but I was sure 40 would
be fine.
Now that I looks maybe NUM_STREAMS from display-limits.h is
a good candidate (and is 50).
> > +
> > + // 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?
>
The reference counting was introduced to avoid the edge cases of
the owner passing used before. Initially the PipeItem were supposed
to be part of the queue (having next/prev as ring item) so was
impossible to insert an item in 2 queues. I remember I suggested
to change the name RedPipeItem as this is now source of confusion.
Some specific case require still the items to be different because
for instance they contain pointers to clients which must be different
but in almost cases they are constant structures used just to store
values to be sent.
Yes, a red_channel_pipes_add would make sense.
>
> > + 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?
>
I think getting the time from guest was removed years ago as buggy
and complicated (think about migration and similar).
This behaviour is already used in all data (audio and video).
The idea is that we should handle data as soon as possible and
have a consistent timer. Or we could add the time spend to encode
so we could use this value to adjust reds_get_mm_time() value.
Considering this I would pass the mmtime to stream_channel_send_data
and move the time computation responsibility to the device.
About this code recently I realize that part of the code expect
the data in the stream to contain a single frame while this code
does not take this into account and sent chunks of data.
I think this is not going to work for mjpeg.
> > + 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) {
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
