Hi Pavel, Couple of comments below. On 10/23/2015 01:25 PM, Pavel Grunt wrote: > When the guest recieves the monitor configuration message, it replies > (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 > --- > The problem can be seen in spicy, gnome-boxes and virt-manager w/ fedora23 wayland as the guest > --- > src/channel-main.c | 67 ++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 50 insertions(+), 17 deletions(-) > > diff --git a/src/channel-main.c b/src/channel-main.c > index 0eb40b9..64d4e4a 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[MAX_DISPLAY]; /* current configuration of spice-widgets */ > + SpiceMonitor display_to_be_sent[MAX_DISPLAY]; /* new requested configuration */ I just find that you could actually use current_display[] and new_display[] as the names for these variables, reflecting the comments you already left on the code. IMO it's easier to read. > gint timer_id; > GQueue *agent_msg_queue; > GHashTable *file_xfer_tasks; > @@ -1149,7 +1152,7 @@ gboolean spice_main_send_monitor_config(SpiceMainChannel *channel) > } else { > monitors = 0; > for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) { > - if (c->display[i].display_state == DISPLAY_ENABLED) > + if (c->display_to_be_sent[i].display_state == DISPLAY_ENABLED) > monitors += 1; > } > } > @@ -1165,6 +1168,7 @@ 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++) { > + c->display[i] = c->display_to_be_sent[i]; /* current display is now display_to_be_sent */ > if (c->display[i].display_state != DISPLAY_ENABLED) { > if (spice_main_agent_test_capability(channel, > VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) > @@ -1513,13 +1517,37 @@ 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_to_be_sent[i].width > 0 && c->display_to_be_sent[i].height > 0) > return TRUE; > } > > return FALSE; > } > > +static gboolean spice_monitor_equal(SpiceMonitor *mon_a, SpiceMonitor *mon_b) > +{ > + return mon_a->x == mon_b->x && > + mon_a->y == mon_b->y && > + mon_a->width == mon_b->width && > + mon_a->height == mon_b->height && > + mon_a->display_state == mon_b->display_state; > +} > + Maybe a memcmp() could be used instead of this function? It is also faster. > +static gboolean spice_monitor_configuration_changed(SpiceMainChannel *channel) > +{ > + SpiceMainChannelPrivate *c; > + guint i; > + > + g_return_val_if_fail(SPICE_IS_MAIN_CHANNEL(channel), FALSE); > + c = channel->priv; > + > + for (i = 0; i < MAX_DISPLAY; i++) { > + if (!spice_monitor_equal(&c->display[i], &c->display_to_be_sent[i])) > + return TRUE; > + } > + return FALSE; > +} > + > /* main context*/ > static gboolean timer_set_display(gpointer data) > { > @@ -1532,6 +1560,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"); > + 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 +1576,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_to_be_sent[i].display_state == DISPLAY_UNDEFINED) { > SPICE_DEBUG("Not sending monitors config, missing monitors"); > return FALSE; > } > @@ -2697,10 +2730,10 @@ void spice_main_update_display(SpiceMainChannel *channel, int id, > > g_return_if_fail(id < SPICE_N_ELEMENTS(c->display)); > > - c->display[id].x = x; > - c->display[id].y = y; > - c->display[id].width = width; > - c->display[id].height = height; > + c->display_to_be_sent[id].x = x; > + c->display_to_be_sent[id].y = y; > + c->display_to_be_sent[id].width = width; > + c->display_to_be_sent[id].height = height; > > if (update) > update_display_timer(channel, 1); > @@ -2903,13 +2936,13 @@ 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; > + c->display_to_be_sent[i].display_state = display_state; > } > } else { > g_return_if_fail(id < G_N_ELEMENTS(c->display)); > - if (c->display[id].display_state == display_state) > + if (c->display_to_be_sent[id].display_state == display_state) > return; > - c->display[id].display_state = display_state; > + c->display_to_be_sent[id].display_state = display_state; > } > > if (update) > -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etrunko@xxxxxxxxxx _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel