On Mon, 2015-10-26 at 15:03 -0500, Jonathon Jongsma wrote: > 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. if it was not successful once, can it be successful another time? > Should we compare instead > to the last *received* (from the server) configuration? But I agree with you that it would be more correct. > > A couple typos below. fixed, thank you Pavel > > 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) _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel