[Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Marc-André Lureau
mlureau at redhat.com
Tue Oct 11 14:36:51 UTC 2016
----- 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list