----- Original Message ----- > Hi, > > On Mon, 2016-10-10 at 08:37 -0400, Marc-André Lureau wrote: > > Hi > > > > ----- Original Message ----- > > > When the guest receives 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. > > > > It's hard follow, could you explain step by step what you mean. > > sorry, I overlooked your question > > I did more investigation and following is happening from clients > perspective: > 1. let window width be 401 > 2. client sends request to resize > 3. client receives PRIMARY_DESTROY > 4. client marks the display as disabled (config in the main channel) > 5. client receives PRIMARY_CREATE + dimensions of display - width is > 400 > 6. client sends a new resize request since resize-guest is on, and > display width != window width > > Flickering is PRIMARY_DESTROY & PRIMARY_CREATE
Is there a way to prevent DESTROY & CREATE of same size on server/guest side? why not? > > Pavel > > > > To avoid the loop, the new monitor configuration messages are only > > > sent > > > if they are different from the current monitor configuration. > > > > > > All the issues are visible only with Wayland & QXL driver on the > > > guest > > > > > > Fixes: > > > https://bugzilla.redhat.com/show_bug.cgi?id=1266484 > > > https://bugs.freedesktop.org/show_bug.cgi?id=94950 > > > --- > > > v3: rebased, fixed typos > > > > In v2, Jonathon raised a valid concern: "However, it's theoretically > > possible that the last sent configuration may not have been > > successful." For ex, vdagent wasn't yet started (whatever handles > > the monitor config change). > > > > Imho, it should be fine for the client to send the same monitor > > config, the server should however not notify of changes if none > > happened. > > > > > v2: > > > https://lists.freedesktop.org/archives/spice-devel/2015-October/02 > > > 2784.html > > > --- > > > src/channel-main.c | 59 > > > +++++++++++++++++++++++++++++++++++------------------- > > > 1 file changed, 38 insertions(+), 21 deletions(-) > > > > > > diff --git a/src/channel-main.c b/src/channel-main.c > > > index 990a06a..1a46819 100644 > > > --- a/src/channel-main.c > > > +++ b/src/channel-main.c > > > @@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate { > > > guint agent_msg_pos; > > > uint8_t agent_msg_size; > > > uint32_t agent_caps[VD_AGENT_CAPS_SIZE]; > > > - SpiceDisplayConfig display[MAX_DISPLAY]; > > > + SpiceDisplayConfig display_current[MAX_DISPLAY]; /* > > > current > > > configuration of spice-widgets */ > > > + SpiceDisplayConfig display_new[MAX_DISPLAY]; /* new > > > requested > > > configuration */ > > > gint timer_id; > > > GQueue *agent_msg_queue; > > > GHashTable *file_xfer_tasks; > > > @@ -1098,11 +1099,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; > > > } > > > } > > > @@ -1117,24 +1118,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: %ux%u+%d+%d @ %u > > > 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(SpiceDisplayConfig)); > > > > > > if (c->disable_display_align == FALSE) > > > monitors_align(mon->monitors, mon->num_of_monitors); > > > @@ -1469,13 +1471,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(SpiceDisplayConfig)) != 0; > > > +} > > > + > > > /* main context*/ > > > static gboolean timer_set_display(gpointer data) > > > { > > > @@ -1488,6 +1500,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 > > > changed"); > > > + return FALSE; > > > + } > > > + > > > if (!any_display_has_dimensions(channel)) { > > > SPICE_DEBUG("Not sending monitors config, at least one > > > monitor must > > > have dimensions"); > > > return FALSE; > > > @@ -1499,7 +1516,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; > > > } > > > @@ -2573,17 +2590,17 @@ 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)); > > > > > > SpiceDisplayConfig display = { > > > .x = x, .y = y, .width = width, .height = height, > > > - .display_state = c->display[id].display_state > > > + .display_state = c->display_new[id].display_state > > > }; > > > > > > - if (memcmp(&display, &c->display[id], > > > sizeof(SpiceDisplayConfig)) == 0) > > > + if (memcmp(&display, &c->display_new[id], > > > sizeof(SpiceDisplayConfig)) == > > > 0) > > > return; > > > > > > - c->display[id] = display; > > > + c->display_new[id] = display; > > > > > > if (update) > > > update_display_timer(channel, 1); > > > @@ -2785,14 +2802,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) > > > -- > > > 2.10.1 > > > > > > _______________________________________________ > > > Spice-devel mailing list > > > Spice-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > _______________________________________________ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel > _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
