Re: [RFC PATCH spice-server v3 08/20] stream-device: Handle streaming data from device to channel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> On Wed, 2017-08-23 at 10:14 +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.
> 
> This last sentence is a bit unclear. Perhaps
> 
> "The StreamChannel will forward the data to the clients using standard
> DisplayChannel messages, and will create and destroy streams as
> necessary".
> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> >  server/stream-channel.c | 108
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  server/stream-channel.h |   8 ++++
> >  server/stream-device.c  |   4 +-
> >  3 files changed, 118 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > index c57f7e89..4ab378a8 100644
> > --- a/server/stream-channel.c
> > +++ b/server/stream-channel.c
> > @@ -20,11 +20,13 @@
> >  #endif
> >  
> >  #include <common/generated_server_marshallers.h>
> > +#include <spice/stream-device.h>
> >  
> >  #include "red-channel-client.h"
> >  #include "stream-channel.h"
> >  #include "reds.h"
> >  #include "common-graphics-channel.h"
> > +#include "display-limits.h"
> >  
> >  #define TYPE_STREAM_CHANNEL_CLIENT stream_channel_client_get_type()
> >  
> > @@ -46,6 +48,8 @@ typedef struct StreamChannelClientClass
> > StreamChannelClientClass;
> >   * to get buffer handling */
> >  struct StreamChannelClient {
> >      CommonGraphicsChannelClient parent;
> > +
> > +    int stream_id;
> >  };
> >  
> >  struct StreamChannelClientClass {
> > @@ -58,6 +62,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 +77,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;
> > +
> >  #define PRIMARY_SURFACE_ID 0
> >  
> >  static void
> > @@ -81,6 +103,7 @@
> > stream_channel_client_class_init(StreamChannelClientClass *klass)
> >  static void
> >  stream_channel_client_init(StreamChannelClient *client)
> >  {
> > +    client->stream_id = -1;
> >  }
> >  
> >  static void
> > @@ -122,6 +145,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: {
> > @@ -147,6 +171,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");
> >      }
> > @@ -255,4 +305,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;
> > +}
> 
> How about a name like:
> 
> shared_pipe_item_ref()
> pipe_item_new_shared()
> 
> ?
> 

I liked the suggestion to add a function to RedChannel.
Too many red_channel_add_whatever :-)

> 
> > +
> > +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);
> > +
> > +    // TODO send new create surface if required
> > +
> > +    // allocate a new stream id
> > +    channel->stream_id = (channel->stream_id + 1) % NUM_STREAMS;
> > +
> > +    // 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);
> > +    red_pipe_item_unref(&item->base);
> > +}
> > +
> > +void
> > +stream_channel_send_data(StreamChannel *channel, const void *data,
> > size_t size, uint32_t mm_time)
> > +{
> > +    if (channel->stream_id < 0) {
> 
> print a warning here? Seems like sending stream data without creating a
> stream is potentially a signal that something went wrong.
> 

I put a comment now.
This is not really an error, can happen if we send a stop but
the guest didn't handle it or was still sending data.

> > +        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 = mm_time;
> > +    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 5691008c..6257f806 100644
> > --- a/server/stream-channel.h
> > +++ b/server/stream-channel.h
> > @@ -48,6 +48,14 @@ 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,
> > +                              uint32_t mm_time);
> > +
> >  G_END_DECLS
> >  
> >  #endif /* STREAM_CHANNEL_H_ */
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index 026f79c7..c1dd02d4 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,
> > reds_get_mm_time());
> >          dev->hdr.size -= n;
> >      }
> >      if (dev->hdr.size == 0) {
> 
> 
> Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> 
> 
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]