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

[Frediano Ziglio]

> > On 19 Mar 2018, at 11:39, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >> 
> >> On 03/13/2018 08:21 AM, Frediano Ziglio wrote:
> >>> Although not necessary for a single monitor DisplayChannel implementation
> >>> this make the DiisplayChannels more coherent from the client
> >>> point of view.
> >>> 
> >>> 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,
> >> 
> >> Hi Frediano,
> >> 
> >> Why do you send id=0 ?
> >> 
> >> Uri.
> >> 
> > 
> > Too much IDs :-)
> > These IDs are allocated starting from 0 and are per channel so as there
> > is only a monitor (currently) the ID should be 0.
> > So this id should not be confused with QXL ID or client/guest ID.
> 
> If Uri is confused, a comment may be in order.
> 
> Why so many IDs? Is it just to avoid an ID lookup during initialization?
> 

Not getting the lookup.

We have a global idea of the "monitor ID" which is quite confusing and
prone to different issues. The DisplayChannel (protocol) use channel IDs
(allocated per channel type starting from 0) and head IDs (allocated per
channel starting from 0). The client and vdagent use the global monitor ID.
The client craft these monitor IDs using basically the formula
"channel ID ? channel ID : head ID" and assuming the vdagent uses the same
IDs. Currently there are different problems of these assumption/schema.

> > 
> > Frediano
> > 
> >>> +            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);
> >>>      }
> >>> 

Frediano