[Spice-devel] [PATCH spice-gtk 2/3] move SpiceDisplayConfig from main channel to session

[Marc-André Lureau]

Hi

On Tue, Jul 26, 2016 at 7:55 PM, Pavel Grunt <pgrunt at redhat.com> wrote:
> It helps to get the real display configuration from display channels
> instead of relying on the last requested monitor config.
>

I don't understand well what that patch is supposed to fix. However,
it is not a good idea to check against the current configuration, as
there might be pending config requests (so it's not the last potential
state). Comparing against the last requested config is probably still
the right thing to do here.

(btw, I wonder if renaming display "id" with "nth" wouldn't be a bit
more clear.. and the DisplayConfig vs MonitorConfig is quite
frustrating, I think it's there for historical reasons, but perhaps it
would be worth to merge somehow)


> Related: https://bugs.freedesktop.org/show_bug.cgi?id=94950
> ---
>  src/channel-main.c       | 32 +++++++++-----------------
>  src/spice-session-priv.h | 17 ++++++++++++++
>  src/spice-session.c      | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+), 21 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 26192ed..cf43649 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -54,20 +54,6 @@
>
>  typedef struct spice_migrate spice_migrate;
>
> -typedef enum {
> -    DISPLAY_UNDEFINED,
> -    DISPLAY_DISABLED,
> -    DISPLAY_ENABLED,
> -} SpiceDisplayState;
> -
> -typedef struct {
> -    int                     x;
> -    int                     y;
> -    int                     width;
> -    int                     height;
> -    SpiceDisplayState       display_state;
> -} SpiceDisplayConfig;
> -
>  typedef struct {
>      GHashTable                 *xfer_task;
>      SpiceMainChannel           *channel;
> @@ -2552,6 +2538,8 @@ void spice_main_update_display(SpiceMainChannel *channel, int id,
>                                 gboolean update)
>  {
>      SpiceMainChannelPrivate *c;
> +    SpiceSession *session;
> +    SpiceDisplayConfig display;
>
>      g_return_if_fail(channel != NULL);
>      g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> @@ -2564,15 +2552,17 @@ void spice_main_update_display(SpiceMainChannel *channel, int id,
>
>      g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
>
> -    SpiceDisplayConfig display = {
> -        .x = x, .y = y, .width = width, .height = height,
> -        .display_state = c->display[id].display_state
> -    };
> +    c->display[id].x = x;
> +    c->display[id].y = y;
> +    c->display[id].width = width;
> +    c->display[id].height = height;
>
> -    if (memcmp(&display, &c->display[id], sizeof(SpiceDisplayConfig)) == 0)
> +    session = spice_channel_get_session(SPICE_CHANNEL(channel));
> +    if (spice_session_get_display_config(session, id, &display) &&
> +        memcmp(&display, &c->display[id], sizeof(SpiceDisplayConfig)) == 0) {
> +        SPICE_DEBUG("new monitor config matches current config");
>          return;
> -
> -    c->display[id] = display;
> +    }
>
>      if (update)
>          update_display_timer(channel, 1);
> diff --git a/src/spice-session-priv.h b/src/spice-session-priv.h
> index 049973a..4e6bec4 100644
> --- a/src/spice-session-priv.h
> +++ b/src/spice-session-priv.h
> @@ -37,6 +37,20 @@ typedef struct _PhodavServer PhodavServer;
>
>  G_BEGIN_DECLS
>
> +typedef enum {
> +    DISPLAY_UNDEFINED,
> +    DISPLAY_DISABLED,
> +    DISPLAY_ENABLED,
> +} SpiceDisplayState;
> +
> +typedef struct {
> +    int                     x;
> +    int                     y;
> +    int                     width;
> +    int                     height;
> +    SpiceDisplayState       display_state;
> +} SpiceDisplayConfig;
> +
>  #define WEBDAV_MAGIC_SIZE 16
>
>  SpiceSession *spice_session_new_from_session(SpiceSession *session);
> @@ -99,6 +113,9 @@ guint spice_session_get_n_display_channels(SpiceSession *session);
>  void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel);
>  gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession *mig_session);
>  SpiceAudio *spice_audio_get(SpiceSession *session, GMainContext *context);
> +gboolean spice_session_get_display_config(const SpiceSession *session,
> +                                          const guint id,
> +                                          SpiceDisplayConfig *config);
>  G_END_DECLS
>
>  #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */
> diff --git a/src/spice-session.c b/src/spice-session.c
> index db283d4..c276227 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -2785,3 +2785,61 @@ gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession
>
>      return TRUE;
>  }
> +
> +
> +static void display_monitor_config_to_display_config(const SpiceDisplayMonitorConfig *monitor,
> +                                                     SpiceDisplayConfig *config)
> +{
> +    config->x = monitor->x;
> +    config->y = monitor->y;
> +    config->width = monitor->width;
> +    config->height = monitor->height;
> +    if (monitor->width > 0 && monitor->height > 0)
> +        config->display_state = DISPLAY_ENABLED;
> +    else
> +        config->display_state = DISPLAY_DISABLED;
> +}
> +
> +/*
> + * spice_session_get_display_config:
> + * @session: a Spice session
> + * @id: a display id (zero based)
> + * @config: a #SpiceDisplayConfig to store result
> + *
> + * Goes through all display channels until it finds a display configuration
> + * for display by given id
> + *
> + * Returns: %FALSE if display with the specified id does not exists
> + **/
> +G_GNUC_INTERNAL
> +gboolean spice_session_get_display_config(const SpiceSession *session,
> +                                          const guint id,
> +                                          SpiceDisplayConfig *config)
> +{
> +    RingItem *item;
> +    struct channel *c;
> +    guint checked_displays = 0;
> +
> +    RING_FOREACH(item, &session->priv->channels) {
> +        c = SPICE_CONTAINEROF(item, struct channel, link);
> +        if (SPICE_IS_DISPLAY_CHANNEL(c->channel)) {
> +            guint monitors_max;
> +            g_object_get(c->channel, "monitors-max", &monitors_max, NULL);
> +            if (id < checked_displays + monitors_max) {
> +                const guint index = id - checked_displays;
> +                GArray *monitors = NULL;
> +                g_object_get(c->channel, "monitors", &monitors, NULL);
> +                if (index < monitors->len) {
> +                    SpiceDisplayMonitorConfig *monitor;
> +                    monitor = &g_array_index(monitors, SpiceDisplayMonitorConfig, index);
> +                    display_monitor_config_to_display_config(monitor, config);
> +                }
> +                g_array_unref(monitors);
> +                return (index < monitors->len); /* monitor has been created for the channel */
> +            }
> +            checked_displays += monitors_max;
> +        }
> +    }
> +
> +    return FALSE;
> +}
> --
> 2.9.2
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau