[Spice-devel] [RFC PATCH spice-server v2 09/19] stream-channel: Allows not fixed size

[Frediano Ziglio]

> 
> On Wed, 2017-06-14 at 16:40 +0100, Frediano Ziglio wrote:
> > Remove the fixed size stream and support any display size.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/stream-channel.c | 34 ++++++++++++++++++++++++++++------
> >  1 file changed, 28 insertions(+), 6 deletions(-)
> > 
> > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > index 119ef94..698a740 100644
> > --- a/server/stream-channel.c
> > +++ b/server/stream-channel.c
> > @@ -65,6 +65,8 @@ struct StreamChannel {
> >      /* current video stream id, <0 if not initialized or
> >       * we are not sending a stream */
> >      int stream_id;
> > +    /* size of the current video stream */
> > +    unsigned width, height;
> >  };
> >  
> >  struct StreamChannelClass {
> > @@ -75,6 +77,7 @@ 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_SURFACE_DESTROY,
> >      RED_PIPE_ITEM_TYPE_FILL_SURFACE,
> >      RED_PIPE_ITEM_TYPE_STREAM_CREATE,
> >      RED_PIPE_ITEM_TYPE_STREAM_DATA,
> > @@ -127,12 +130,12 @@ stream_channel_client_new(StreamChannel
> > *channel, RedClient *client, RedsStream
> >  }
> >  
> >  static void
> > -fill_base(SpiceMarshaller *m)
> > +fill_base(SpiceMarshaller *m, const StreamChannel *channel)
> >  {
> >      SpiceMsgDisplayBase base;
> >  
> >      base.surface_id = 0;
> > -    base.box = (SpiceRect) { 0, 0, 1024, 768 };
> > +    base.box = (SpiceRect) { 0, 0, channel->width, channel->height
> > };
> >      base.clip = (SpiceClip) { SPICE_CLIP_TYPE_NONE, NULL };
> >  
> >      spice_marshall_DisplayBase(m, &base);
> > @@ -143,18 +146,28 @@ stream_channel_send_item(RedChannelClient *rcc,
> > RedPipeItem *pipe_item)
> >  {
> >      SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> >      StreamChannelClient *client = STREAM_CHANNEL_CLIENT(rcc);
> > +    StreamChannel *channel =
> > STREAM_CHANNEL(red_channel_client_get_channel(rcc));
> >  
> >      switch (pipe_item->type) {
> >      case RED_PIPE_ITEM_TYPE_SURFACE_CREATE: {
> >          red_channel_client_init_send_data(rcc,
> > SPICE_MSG_DISPLAY_SURFACE_CREATE);
> > -        SpiceMsgSurfaceCreate surface_create = { 0, 1024, 768,
> > SPICE_SURFACE_FMT_32_xRGB, SPICE_SURFACE_FLAGS_PRIMARY };
> > +        SpiceMsgSurfaceCreate surface_create = {
> > +            0, channel->width, channel->height,
> > +            SPICE_SURFACE_FMT_32_xRGB, SPICE_SURFACE_FLAGS_PRIMARY
> > +        };
> >          spice_marshall_msg_display_surface_create(m,
> > &surface_create);
> >          break;
> >      }
> > +    case RED_PIPE_ITEM_TYPE_SURFACE_DESTROY: {
> > +        red_channel_client_init_send_data(rcc,
> > SPICE_MSG_DISPLAY_SURFACE_DESTROY);
> > +        SpiceMsgSurfaceDestroy surface_destroy = { 0 };
> 
> Maybe add a comment about how only surface #0 is supported?
> 

Maybe a mnemonic like PRIMARY_SURFACE_ID would help here.
Note that the same "0" is used in RED_PIPE_ITEM_TYPE_SURFACE_CREATE
introduced in an earlier patch.
Simply this surface is the only visible one and we want to send
visible data.

A bit OT on the primary surface is how to handle multimonitor.
Currently there are 2 way: Windows and Linux.
For Windows there's a different DisplayChannel (protocol
meaning) for each monitor so basically each primary
surface is related to a monitor.
For Linux there's a single DisplayChannel with an huge primary
surface and each monitor points to an area of this surface.
How do we want to handle multimonitor for vGPUs?
One issue I can see is that if an user want to add an additional
monitor we need to:
- (Windows way) create a new DisplayChannel. Is possible to
  create if if not expected at the beginning? Will the client
  connect to this new channel?
- (Linux way) enlarge the surface on the single DisplayChannel.
  Put all monitors vertically or horizontally? How these monitors
  will be shown (position) on the client? Will the old stream
  (that represent old monitor) keep working or will be required
  to start them again (with all bandwidth consumption)?
Should we add some feature to the client to support multimonitor?

> 
> > +        spice_marshall_msg_display_surface_destroy(m,
> > &surface_destroy);
> > +        break;
> > +    }
> >      case RED_PIPE_ITEM_TYPE_FILL_SURFACE: {
> >          red_channel_client_init_send_data(rcc,
> > SPICE_MSG_DISPLAY_DRAW_FILL);
> >  
> > -        fill_base(m);
> > +        fill_base(m, channel);
> >  
> >          SpiceFill fill;
> >          fill.brush = (SpiceBrush) { SPICE_BRUSH_TYPE_SOLID, { .color
> > = 0 } };
> > @@ -253,7 +266,7 @@ stream_channel_connect(RedChannel *red_channel,
> > RedClient *red_client, RedsStrea
> >      // "emulate" dcc_start
> >      // TODO only if "surface"
> >      red_channel_client_pipe_add_empty_msg(rcc,
> > SPICE_MSG_DISPLAY_INVAL_ALL_PALETTES);
> > -    // TODO pass proper data
> > +    // pass proper data
> >      red_channel_client_pipe_add_type(rcc,
> > RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
> >      // surface data
> >      red_channel_client_pipe_add_type(rcc,
> > RED_PIPE_ITEM_TYPE_FILL_SURFACE);
> > @@ -299,6 +312,8 @@ static void
> >  stream_channel_init(StreamChannel *channel)
> >  {
> >      channel->stream_id = -1;
> > +    channel->width = 1024;
> > +    channel->height = 768;
> >  }
> >  
> >  static RedPipeItem *
> > @@ -317,7 +332,14 @@ stream_channel_change_format(StreamChannel
> > *channel, const StreamMsgFormat *fmt)
> >      // send destroy old stream
> >      red_channel_pipes_add_type(red_channel,
> > RED_PIPE_ITEM_TYPE_STREAM_DESTROY);
> >  
> > -    // TODO send new create surface if required
> > +    // send new create surface if required
> > +    if (channel->width != fmt->width || channel->height != fmt-
> > >height) {
> > +        channel->width = fmt->width;
> > +        channel->height = fmt->height;
> > +        red_channel_pipes_add_type(red_channel,
> > RED_PIPE_ITEM_TYPE_SURFACE_DESTROY);
> > +        red_channel_pipes_add_type(red_channel,
> > RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
> > +        // TODO monitors config ??
> > +    }
> >  
> >      // allocate a new stream id
> >      channel->stream_id = (channel->stream_id + 1) % 40;
> 

Frediano