[Spice-devel] [PATCH spice-gtk v2] main: Send monitor config only when it changes

Jonathon Jongsma jjongsma at redhat.com
Mon Oct 26 13:03:47 PDT 2015


While I fully support the effort to reduce unnecessary monitor config
messages, I have one potential concern about this patch. This patch
compares the current display configuration to the last sent
configuration. However, it's theoretically possible that the last sent
configuration may not have been successful. Should we compare instead
to the last *received* (from the server) configuration?

A couple typos below.

On Mon, 2015-10-26 at 17:36 +0100, Pavel Grunt wrote:
> When the guest recieves the monitor configuration message, it replies

typo: recieves -> receives

> (through spice-server) by destroying the primary surface, which makes
> the SpiceDisplay disabled if its "resize-guest" property is used.
> This change of the display state (disabled/enabled) leads to sending
> a new monitor config message (containing the disabled display) after
> a second. Then spice-server sends the monitor configuration message,
> spice-gtk replies back and we may end up with a loop of monitor
> configuration messages even if no real change of monitor
> configuration
> happens.
> 
> To avoid the loop, the new monitor configuration messages are only
> sent
> if they are different from the current monitor configuration.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1266484
> ---
> v2:
>  - rename display to display_current, display_to_be_sent to
> display_new
>  - use memcmp instead of for loop to check whether monitor
> configuration has changed 
> ---
>  src/channel-main.c | 75 ++++++++++++++++++++++++++++++++++----------
> ----------
>  1 file changed, 47 insertions(+), 28 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 0eb40b9..e0c5701 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -123,6 +123,14 @@ typedef enum {
>      DISPLAY_ENABLED,
>  } SpiceDisplayState;
>  
> +typedef struct SpiceMonitor {
> +    int                     x;
> +    int                     y;
> +    int                     width;
> +    int                     height;
> +    SpiceDisplayState       display_state;
> +} SpiceMonitor;
> +
>  struct _SpiceMainChannelPrivate  {
>      enum SpiceMouseMode         mouse_mode;
>      bool                        agent_connected;
> @@ -142,13 +150,8 @@ struct _SpiceMainChannelPrivate  {
>      guint                       agent_msg_pos;
>      uint8_t                     agent_msg_size;
>      uint32_t                    agent_caps[VD_AGENT_CAPS_SIZE];
> -    struct {
> -        int                     x;
> -        int                     y;
> -        int                     width;
> -        int                     height;
> -        SpiceDisplayState       display_state;
> -    } display[MAX_DISPLAY];
> +    SpiceMonitor                display_current[MAX_DISPLAY]; /*
> current configuration of spice-widgets */
> +    SpiceMonitor                display_new[MAX_DISPLAY]; /* new
> requested configuration */
>      gint                        timer_id;
>      GQueue                      *agent_msg_queue;
>      GHashTable                  *file_xfer_tasks;
> @@ -1145,11 +1148,11 @@ gboolean
> spice_main_send_monitor_config(SpiceMainChannel *channel)
>  
>      if (spice_main_agent_test_capability(channel,
>                                      
>  VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
> -        monitors = SPICE_N_ELEMENTS(c->display);
> +        monitors = SPICE_N_ELEMENTS(c->display_new);
>      } else {
>          monitors = 0;
> -        for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> -            if (c->display[i].display_state == DISPLAY_ENABLED)
> +        for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> +            if (c->display_new[i].display_state == DISPLAY_ENABLED)
>                  monitors += 1;
>          }
>      }
> @@ -1164,24 +1167,25 @@ gboolean
> spice_main_send_monitor_config(SpiceMainChannel *channel)
>  
>      CHANNEL_DEBUG(channel, "sending new monitors config to guest");
>      j = 0;
> -    for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> -        if (c->display[i].display_state != DISPLAY_ENABLED) {
> +    for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> +        if (c->display_new[i].display_state != DISPLAY_ENABLED) {
>              if (spice_main_agent_test_capability(channel,
>                                      
>  VD_AGENT_CAP_SPARSE_MONITORS_CONFIG))
>                  j++;
>              continue;
>          }
>          mon->monitors[j].depth  = c->display_color_depth ? c
> ->display_color_depth : 32;
> -        mon->monitors[j].width  = c->display[i].width;
> -        mon->monitors[j].height = c->display[i].height;
> -        mon->monitors[j].x = c->display[i].x;
> -        mon->monitors[j].y = c->display[i].y;
> +        mon->monitors[j].width  = c->display_new[i].width;
> +        mon->monitors[j].height = c->display_new[i].height;
> +        mon->monitors[j].x = c->display_new[i].x;
> +        mon->monitors[j].y = c->display_new[i].y;
>          CHANNEL_DEBUG(channel, "monitor #%d: %dx%d+%d+%d @ %d bpp",
> j,
>                        mon->monitors[j].width, mon
> ->monitors[j].height,
>                        mon->monitors[j].x, mon->monitors[j].y,
>                        mon->monitors[j].depth);
>          j++;
>      }
> +    memcpy(c->display_current, c->display_new, MAX_DISPLAY *
> sizeof(SpiceMonitor));
>  
>      if (c->disable_display_align == FALSE)
>          monitors_align(mon->monitors, mon->num_of_monitors);
> @@ -1513,13 +1517,23 @@ static gboolean
> any_display_has_dimensions(SpiceMainChannel *channel)
>      c = channel->priv;
>  
>      for (i = 0; i < MAX_DISPLAY; i++) {
> -        if (c->display[i].width > 0 && c->display[i].height > 0)
> +        if (c->display_new[i].width > 0 && c->display_new[i].height
> > 0)
>              return TRUE;
>      }
>  
>      return FALSE;
>  }
>  
> +static gboolean spice_monitor_configuration_changed(SpiceMainChannel
> *channel)
> +{
> +    SpiceMainChannelPrivate *c;
> +
> +    g_return_val_if_fail(SPICE_IS_MAIN_CHANNEL(channel), FALSE);
> +    c = channel->priv;
> +
> +    return memcmp(c->display_current, c->display_new, MAX_DISPLAY *
> sizeof(SpiceMonitor)) != 0;
> +}
> +
>  /* main context*/
>  static gboolean timer_set_display(gpointer data)
>  {
> @@ -1532,6 +1546,11 @@ static gboolean timer_set_display(gpointer
> data)
>      if (!c->agent_connected)
>          return FALSE;
>  
> +    if (!spice_monitor_configuration_changed(channel)) {
> +        SPICE_DEBUG("Not sending monitors config, configuration has
> not change");

typo: change -> changed

> +        return FALSE;
> +    }
> +
>      if (!any_display_has_dimensions(channel)) {
>          SPICE_DEBUG("Not sending monitors config, at least one
> monitor must have dimensions");
>          return FALSE;
> @@ -1543,7 +1562,7 @@ static gboolean timer_set_display(gpointer
> data)
>          /* ensure we have an explicit monitor configuration at least
> for
>             number of display channels */
>          for (i = 0; i <
> spice_session_get_n_display_channels(session); i++)
> -            if (c->display[i].display_state == DISPLAY_UNDEFINED) {
> +            if (c->display_new[i].display_state ==
> DISPLAY_UNDEFINED) {
>                  SPICE_DEBUG("Not sending monitors config, missing
> monitors");
>                  return FALSE;
>              }
> @@ -2695,12 +2714,12 @@ void
> spice_main_update_display(SpiceMainChannel *channel, int id,
>  
>      c = SPICE_MAIN_CHANNEL(channel)->priv;
>  
> -    g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
> +    g_return_if_fail(id < SPICE_N_ELEMENTS(c->display_new));
>  
> -    c->display[id].x      = x;
> -    c->display[id].y      = y;
> -    c->display[id].width  = width;
> -    c->display[id].height = height;
> +    c->display_new[id].x      = x;
> +    c->display_new[id].y      = y;
> +    c->display_new[id].width  = width;
> +    c->display_new[id].height = height;
>  
>      if (update)
>          update_display_timer(channel, 1);
> @@ -2902,14 +2921,14 @@ void
> spice_main_update_display_enabled(SpiceMainChannel *channel, int id,
> gboole
>  
>      if (id == -1) {
>          gint i;
> -        for (i = 0; i < G_N_ELEMENTS(c->display); i++) {
> -            c->display[i].display_state = display_state;
> +        for (i = 0; i < G_N_ELEMENTS(c->display_new); i++) {
> +            c->display_new[i].display_state = display_state;
>          }
>      } else {
> -        g_return_if_fail(id < G_N_ELEMENTS(c->display));
> -        if (c->display[id].display_state == display_state)
> +        g_return_if_fail(id < G_N_ELEMENTS(c->display_new));
> +        if (c->display_new[id].display_state == display_state)
>              return;
> -        c->display[id].display_state = display_state;
> +        c->display_new[id].display_state = display_state;
>      }
>  
>      if (update)


More information about the Spice-devel mailing list