[Spice-devel] [PATCH spice-gtk v2] main: Send monitor config only when it changes
Jonathon Jongsma
jjongsma at redhat.com
Mon Oct 26 13:03:47 PDT 2015
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. Should we compare instead
to the last *received* (from the server) configuration?
A couple typos below.
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)
More information about the Spice-devel
mailing list