[Spice-devel] [PATCH spice-server] stream-channel: Implements monitors_config

[Lukáš Hrázký]

On Tue, 2018-03-13 at 10:15 -0400, Frediano Ziglio wrote:
> > 
> > On Tue, 2018-03-13 at 06:21 +0000, Frediano Ziglio wrote:
> > > Although not necessary for a single monitor DisplayChannel implementation
> > > this make the DiisplayChannels more coherent from the client
> > > point of view.
> > 
> > Typo: "DiisplayChannels"
> > 
> > I've tested this, as expected, with the heads attribute of the qxl
> > device (in the xml) greater than 1, this causes an immediate round trip
> > of monitors_config making the server create a second monitor in the
> > guest and subsequently the streaming agent streaming two desktops next
> > to each other.
> > 
> > Small improvement, a lot of work left to do. I would add a similar
> > description of the current rather lacking state of things to the commit
> > message. With that,
> > 
> 
> This patch was not intended to fix this.

I know :)

> Do you have a suggestion for the comment I should add?

How about the following (is it clear enough?), and I think it wouldn't
hurt to put this in a comment for the marshall_monitors_config()
function too. Can help somebody in the future.

The monitors_config interaction is currently limited to work only if
the VM doesn't have more than one monitor configured (i.e. in the heads
attribute of the qxl device).

This is because when we send monitors_config to the client, it will
create a monitor for this display channel in it's list of monitors for
the display channel. The client (specifically remote_viewer here) will
then send a list of monitors to the server in a VDAgentMonitorsConfig
message on the main channel. In case the VM has more than one monitor
configured, this will cause the server to create that many monitors in
the guest, thus creating a second monitor for the streaming channel,
which is not desireable.

> 
> > Acked-by: Lukáš Hrázký <lhrazky at redhat.com>
> > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/stream-channel.c | 33 ++++++++++++++++++++++++++++++---
> > >  1 file changed, 30 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > > index 3a3b733f..a9882d9c 100644
> > > --- a/server/stream-channel.c
> > > +++ b/server/stream-channel.c
> > > @@ -102,6 +102,7 @@ enum {
> > >      RED_PIPE_ITEM_TYPE_STREAM_DATA,
> > >      RED_PIPE_ITEM_TYPE_STREAM_DESTROY,
> > >      RED_PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> > > +    RED_PIPE_ITEM_TYPE_MONITORS_CONFIG,
> > >  };
> > >  
> > >  typedef struct StreamCreateItem {
> > > @@ -204,6 +205,26 @@ fill_base(SpiceMarshaller *m, const StreamChannel
> > > *channel)
> > >      spice_marshall_DisplayBase(m, &base);
> > >  }
> > >  
> > > +static void
> > > +marshall_monitors_config(RedChannelClient *rcc, StreamChannel *channel,
> > > SpiceMarshaller *m)
> > > +{
> > > +    struct {
> > > +        SpiceMsgDisplayMonitorsConfig config;
> > > +        SpiceHead head;
> > > +    } msg = {
> > > +        { 1, 1, },
> > > +        {
> > > +            0, PRIMARY_SURFACE_ID,
> > > +            channel->width, channel->height,
> > > +            0, 0,
> > > +            0 // flags
> > > +        }
> > > +    };
> > > +
> > > +    red_channel_client_init_send_data(rcc,
> > > SPICE_MSG_DISPLAY_MONITORS_CONFIG);
> > > +    spice_marshall_msg_display_monitors_config(m, &msg.config);
> > > +}
> > > +
> > >  static void
> > >  stream_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_item)
> > >  {
> > > @@ -229,6 +250,12 @@ stream_channel_send_item(RedChannelClient *rcc,
> > > RedPipeItem *pipe_item)
> > >          spice_marshall_msg_display_surface_create(m, &surface_create);
> > >          break;
> > >      }
> > > +    case RED_PIPE_ITEM_TYPE_MONITORS_CONFIG:
> > > +        if (!red_channel_client_test_remote_cap(rcc,
> > > SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
> > > +            return;
> > > +        }
> > > +        marshall_monitors_config(rcc, channel, m);
> > > +        break;
> > >      case RED_PIPE_ITEM_TYPE_SURFACE_DESTROY: {
> > >          red_channel_client_init_send_data(rcc,
> > >          SPICE_MSG_DISPLAY_SURFACE_DESTROY);
> > >          SpiceMsgSurfaceDestroy surface_destroy = { PRIMARY_SURFACE_ID };
> > > @@ -397,7 +424,6 @@ stream_channel_connect(RedChannel *red_channel,
> > > RedClient *red_client, RedStream
> > >      request_new_stream(channel, start);
> > >  
> > >  
> > > -    // TODO set capabilities like  SPICE_DISPLAY_CAP_MONITORS_CONFIG
> > >      // see guest_set_client_capabilities
> > >      RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> > >      red_channel_client_push_set_ack(rcc);
> > > @@ -415,6 +441,7 @@ stream_channel_connect(RedChannel *red_channel,
> > > RedClient *red_client, RedStream
> > >  
> > >      // pass proper data
> > >      red_channel_client_pipe_add_type(rcc,
> > >      RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
> > > +    red_channel_client_pipe_add_type(rcc,
> > > RED_PIPE_ITEM_TYPE_MONITORS_CONFIG);
> > >      // surface data
> > >      red_channel_client_pipe_add_type(rcc,
> > >      RED_PIPE_ITEM_TYPE_FILL_SURFACE);
> > >      // TODO monitor configs ??
> > > @@ -433,8 +460,7 @@ stream_channel_constructed(GObject *object)
> > >      client_cbs.connect = stream_channel_connect;
> > >      red_channel_register_client_cbs(red_channel, &client_cbs, NULL);
> > >  
> > > -    // TODO, send monitor to support multiple monitors in the future
> > > -//    red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > > +    red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > >      red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> > >  
> > >      reds_register_channel(reds, red_channel);
> > > @@ -489,6 +515,7 @@ stream_channel_change_format(StreamChannel *channel,
> > > const StreamMsgFormat *fmt)
> > >          channel->width = fmt->width;
> > >          channel->height = fmt->height;
> > >          red_channel_pipes_add_type(red_channel,
> > >          RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
> > > +        red_channel_pipes_add_type(red_channel,
> > > RED_PIPE_ITEM_TYPE_MONITORS_CONFIG);
> > >          // TODO monitors config ??
> > >          red_channel_pipes_add_empty_msg(red_channel,
> > >          SPICE_MSG_DISPLAY_MARK);
> > >      }