[Spice-devel] [PATCH spice-gtk] main: Send monitor config only when it changes
Pavel Grunt
pgrunt at redhat.com
Mon Oct 26 08:16:58 PDT 2015
Hi Eduardo,
On Mon, 2015-10-26 at 10:34 -0200, Eduardo Lima (Etrunko) wrote:
> 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.
ok, renamed.
>
> > 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.
>
Right, it can be used in the following function to avoid the loop at all.
Sending v2, thanks
Pavel
> > +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)
> >
>
>
More information about the Spice-devel
mailing list