[Spice-devel] [PATCH spice-gtk] main: Send monitor config only when it changes
Eduardo Lima (Etrunko)
etrunko at redhat.com
Mon Oct 26 05:34:03 PDT 2015
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 at redhat.com
More information about the Spice-devel
mailing list