Hi Marc-André,
On Fri, 2018-08-17 at 16:27 +0200, Marc-André Lureau wrote:
> Hi
>
> On Thu, Aug 16, 2018 at 6:27 PM Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> >
> > Before this commit, the code relied on
> > spice_main_channel_update_display() called from the client application
> > to add items to the list of monitor_configs to be sent to the server
> > (stored in SpiceMainChannelPrivate.display). One of the disadvantages
> > this approach has is that all data that you need in the list have to go
> > through the client application. Thus adding any attribute to a
> > monitor_config requires an API change.
>
> What additional attribute do you need?
This is a remnant from the first version of the patch series, in which
I needed to add the "guest_output_id" attribute (in that series it was
called "output_id").
It remains a valid point, although not practically relevant anymore.
> > This commit changes this by creating and maintaining our own list of
> > monitor_configs under the session object. The methods that are exposed
> > to the client app no longer create new items in this list.
>
> What is the advantge of moving the config and the API to the session?
> It "looks" better, but it doesn't seem necessary.
It is indeed not necessary. But since the monitors_config list is
accessed from both the main and display channels, it made sense to move
it to the session. And with it I moved the API. I had to change it
anyway, so the move doesn't add much cost.
> Before, we had a fixed array of MAX_DISPLAY size, and we could just
> index into it. Now it's a dynamic array. Why?
Because you indexed the array with a `channel_id ? channel_id :
monitor_id`. Once the assumption of either of them being 0 is gone,
this is no longer unambiguous.
> >
> > Now the spice_main_channel_update_display{,_enabled}() functions only
> > update existing items in the spice-gtk-maintained list. The identifier
> > for these functions was unfortunately the index in the array on the
> > spice-gtk side and channel_id + monitor_id in e.g. virt-viewer. This
> > worked under the assumption that there is either only one display
> > channel or multiple display channels each with only one monitor.
> > Presumably other clients can use whatever they like as the id, but doing
> > something else most probably wouldn't work.
> >
> > So, we use the channel_id + monitor_id in the now-deprecated
> > spice_main_channel_update_display{,_enabled}() to match against their
> > single 'id' argument. This works for as long as the assumption mentioned
> > above holds true, which doesn't for the guest streaming use case
> > anymore. So for that use case a new client is needed using the new
> > consistent API.
>
> Hmm, streaming will require display channel ID != 0 & monitor ID ? Why?
I'm not entirely sure I understand, but if you're asking why we break
the assumption (I should give it a name at this stage, I keep repeating
this :) - I'll call it assumption ZERO) that either channel_id or
monitor_id needs to be 0, then that's described in the cover letter.
It's the 2.b in listing the issues: With vGPU streaming, you usually
have a QXL device with a single monitor to show the boot and then a
vGPU with some number of monitors that are streaming X.
> > An unsorted list is used and the items are looked up by simple
> > iteration. No more than 5 items are really expected in the list. The
> > order of the items in the list is determined by their creation timing
> > and therefore is undeterministic. The order should no longer matter for
> > anything though.
> >
> > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > ---
> > src/channel-display.c | 23 +++++-
> > src/channel-main.c | 156 +++++++++++++++++++++++++--------------
> > src/channel-main.h | 3 +
> > src/map-file | 4 +
> > src/spice-glib-sym-file | 4 +
> > src/spice-session-priv.h | 10 +++
> > src/spice-session.c | 152 ++++++++++++++++++++++++++++++++++++++
> > src/spice-session.h | 31 ++++++++
> > src/spice-widget.c | 7 +-
> > tools/spicy.c | 15 ++--
> > 10 files changed, 334 insertions(+), 71 deletions(-)
> >
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 7c663cb..868aa83 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1049,8 +1049,10 @@ static void spice_display_channel_up(SpiceChannel *channel)
> > out->marshallers->msgc_display_init(out->marshaller, &init);
> > spice_msg_out_send_internal(out);
> >
> > - /* notify of existence of this monitor */
> > - g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> > + // if we don't have MONITORS_CONFIG_CAPABILITY, notify of existence of this monitor
>
> In general we use C /* */ comments
I actually thought I've seen it mentioned in the coding style, but now
I don't see it there, so maybe I was wrong.
Anyway, I've seen plenty of C++ style comments throughout the code.
Unless the team agrees we don't want those (which frankly doesn't make
much sense), I'm going to use them where I find them better suitable.
> > + if (!spice_channel_test_capability(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
> > + g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> > + }
> >
>
> This looks like an unrelated change, please split it.
It is related, though it probably deserves more explanation.
This is needed with the new way of adding and updating items in the
monitor_configs list. If you call this notify here, the code will
attempt to update monitor_configs even though they weren't added to the
list yet.
Thinking about it now, this quite probably breaks if you have an old
enough server without the capability. I'll add this to my list of
things to check. Question is if we still need to support not having
that capability...
> > if (preferred_compression != SPICE_IMAGE_COMPRESSION_INVALID) {
> > spice_display_channel_change_preferred_compression(channel, preferred_compression);
> > @@ -1869,6 +1871,7 @@ static void display_handle_surface_destroy(SpiceChannel *channel, SpiceMsgIn *in
> > /* coroutine context */
> > static void display_handle_monitors_config(SpiceChannel *channel, SpiceMsgIn *in)
> > {
> > + SpiceSession *session = spice_channel_get_session(channel);
> > SpiceMsgDisplayMonitorsConfig *config = spice_msg_in_parsed(in);
> > SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> > guint i;
> > @@ -1907,6 +1910,22 @@ static void display_handle_monitors_config(SpiceChannel *channel, SpiceMsgIn *in
> > mc->y = head->y;
> > mc->width = head->width;
> > mc->height = head->height;
> > +
> > + if (spice_session_get_monitor_config(session, channel->priv->channel_id, i)) {
> > + spice_session_update_monitor_config_dimensions(session, channel->priv->channel_id, i,
> > + head->x, head->y, head->width, head->height, FALSE);
> > + } else {
> > + spice_session_add_monitor_config(session, channel->priv->channel_id, i,
> > + head->x, head->y, head->width, head->height);
> > + }
> > + }
> > +
> > + // On top of the above, create disabled configs for all monitors up to max_allowed
> > + // if they don't exist.
> > + for (i = config->count; i < config->max_allowed; i++) {
> > + if (!spice_session_get_monitor_config(session, channel->priv->channel_id, i)) {
> > + spice_session_add_monitor_config(session, channel->priv->channel_id, i, 0, 0, 0, 0);
> > + }
> > }
> >
> > g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 4c6bc70..a23bc64 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -52,20 +52,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;
> > @@ -102,7 +88,6 @@ struct _SpiceMainChannelPrivate {
> > guint agent_msg_pos;
> > uint8_t agent_msg_size;
> > uint32_t agent_caps[VD_AGENT_CAPS_SIZE];
> > - SpiceDisplayConfig display[MAX_DISPLAY];
> > gint timer_id;
> > GQueue *agent_msg_queue;
> > GHashTable *file_xfer_tasks;
> > @@ -1111,13 +1096,17 @@ gboolean spice_main_channel_send_monitor_config(SpiceMainChannel *channel)
> > c = channel->priv;
> > g_return_val_if_fail(c->agent_connected, FALSE);
> >
> > + GArray *monitor_configs = spice_session_get_monitor_configs(
> > + spice_channel_get_session(SPICE_CHANNEL(channel)));
> > +
> > if (spice_main_channel_agent_test_capability(channel, VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
> > - monitors = SPICE_N_ELEMENTS(c->display);
> > + monitors = monitor_configs->len;
> > } 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 < monitor_configs->len; i++) {
> > + if (g_array_index(monitor_configs, SpiceMonitorConfig, i).display_state == DISPLAY_ENABLED) {
> > monitors += 1;
> > + }
> > }
> > }
> >
> > @@ -1131,18 +1120,19 @@ gboolean spice_main_channel_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 < monitor_configs->len; i++) {
> > + SpiceMonitorConfig *mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> > + if (mc->display_state != DISPLAY_ENABLED) {
> > if (spice_main_channel_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 = mc->width;
> > + mon->monitors[j].height = mc->height;
> > + mon->monitors[j].x = mc->x;
> > + mon->monitors[j].y = mc->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,
> > @@ -1477,15 +1467,18 @@ static void agent_clipboard_release(SpiceMainChannel *channel, guint selection)
> >
> > static gboolean any_display_has_dimensions(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 (c->display[i].width > 0 && c->display[i].height > 0)
> > + GArray *monitor_configs = spice_session_get_monitor_configs(
> > + spice_channel_get_session(SPICE_CHANNEL(channel)));
> > +
> > + for (i = 0; i < monitor_configs->len; i++) {
> > + SpiceMonitorConfig *mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> > + if (mc->width > 0 && mc->height > 0) {
> > return TRUE;
> > + }
> > }
> >
> > return FALSE;
> > @@ -1509,12 +1502,19 @@ static gboolean timer_set_display(gpointer data)
> > }
> >
> > session = spice_channel_get_session(SPICE_CHANNEL(channel));
> > + GArray *monitor_configs = spice_session_get_monitor_configs(session);
> >
> > if (!spice_main_channel_agent_test_capability(channel, VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
> > /* 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) {
> > +
> > + guint n_display_channels = spice_session_get_n_display_channels(session);
> > + if (n_display_channels < monitor_configs->len) {
> > + SPICE_DEBUG("Not sending monitors config, missing monitors");
> > + return FALSE;
> > + }
> > + for (i = 0; i < n_display_channels; i++)
> > + if (g_array_index(monitor_configs, SpiceMonitorConfig, i).display_state == DISPLAY_UNDEFINED) {
> > SPICE_DEBUG("Not sending monitors config, missing monitors");
> > return FALSE;
> > }
> > @@ -1525,7 +1525,7 @@ static gboolean timer_set_display(gpointer data)
> > }
> >
> > /* any context */
> > -static void update_display_timer(SpiceMainChannel *channel, guint seconds)
> > +void spice_main_channel_update_display_timer(SpiceMainChannel *channel, guint seconds)
> > {
> > SpiceMainChannelPrivate *c = channel->priv;
> >
> > @@ -2010,7 +2010,7 @@ static void main_agent_handle_msg(SpiceChannel *channel,
> > }
> > c->agent_caps_received = true;
> > g_coroutine_signal_emit(self, signals[SPICE_MAIN_AGENT_UPDATE], 0);
> > - update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
> > + spice_main_channel_update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
> >
> > if (caps->request)
> > agent_announce_caps(self);
> > @@ -2626,7 +2626,7 @@ gboolean spice_main_channel_agent_test_capability(SpiceMainChannel *channel, gui
> > /**
> > * spice_main_update_display:
> > * @channel: a #SpiceMainChannel
> > - * @id: display ID
> > + * @id: the display ID - assumed to be channel_id + monitor_id
>
> Well, not exactly. display ID is rather: either monitor_id OR
> channel_id... We don't support channel_id + monitor_id...
Under assumption ZERO the expressions are equivalent:
channel_id + monitor_id == channel_id ? channel_id : monitor_id
In fact, both were used in various parts of the code (you can find it
throughtout the patches). Yes, I wouldn't call it pretty...
> > * @x: x position
> > * @y: y position
> > * @width: display width
> > @@ -2652,7 +2652,7 @@ void spice_main_update_display(SpiceMainChannel *channel, int id,
> > /**
> > * spice_main_channel_update_display:
> > * @channel: a #SpiceMainChannel
> > - * @id: display ID
> > + * @id: the display ID - assumed to be channel_id + monitor_id
> > * @x: x position
> > * @y: y position
> > * @width: display width
> > @@ -2671,8 +2671,6 @@ void spice_main_update_display(SpiceMainChannel *channel, int id,
> > void spice_main_channel_update_display(SpiceMainChannel *channel, int id, int x, int y, int width,
> > int height, gboolean update)
> > {
> > - SpiceMainChannelPrivate *c;
> > -
> > g_return_if_fail(channel != NULL);
> > g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> > g_return_if_fail(x >= 0);
> > @@ -2680,28 +2678,48 @@ void spice_main_channel_update_display(SpiceMainChannel *channel, int id, int x,
> > g_return_if_fail(width >= 0);
> > g_return_if_fail(height >= 0);
> >
> > - c = SPICE_MAIN_CHANNEL(channel)->priv;
> > + GArray *monitor_configs = spice_session_get_monitor_configs(
> > + spice_channel_get_session(SPICE_CHANNEL(channel)));
> >
> > - g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
> > + bool found = false;
> > + SpiceMonitorConfig *mc = NULL;
> > + for (size_t i = 0; i < monitor_configs->len; ++i) {
> > + mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> > + if (mc->channel_id + mc->monitor_id == id) {
> > + found = true;
> > + break;
> > + }
> > + }
> >
> > - SpiceDisplayConfig display = {
> > - .x = x, .y = y, .width = width, .height = height,
> > - .display_state = c->display[id].display_state
> > - };
> > + if (!found) {
> > + g_warning("Display ID %d not found while trying to update the display (monitor config).", id);
> > + return;
> > + }
> >
> > - if (memcmp(&display, &c->display[id], sizeof(SpiceDisplayConfig)) == 0)
> > + if (mc->x == x &&
> > + mc->y == y &&
> > + mc->width == width &&
> > + mc->height == height) {
> > return;
> > + }
> > +
> > + CHANNEL_DEBUG(channel,
> > + "Update display (monitor config) id: %d (channel_id: %u, monitor_id: %u), +%d+%d-%dx%d",
> > + id, mc->channel_id, mc->monitor_id, x, y, width, height);
> >
> > - c->display[id] = display;
> > + mc->x = x;
> > + mc->y = y;
> > + mc->width = width;
> > + mc->height = height;
> >
> > if (update)
> > - update_display_timer(channel, 1);
> > + spice_main_channel_update_display_timer(channel, 1);
> > }
> >
> > /**
> > * spice_main_set_display:
> > * @channel: a #SpiceMainChannel
> > - * @id: display ID
> > + * @id: the display ID - assumed to be channel_id + monitor_id
> > * @x: x position
> > * @y: y position
> > * @width: display width
> > @@ -2943,7 +2961,7 @@ void spice_main_channel_clipboard_selection_request(SpiceMainChannel *channel, g
> > /**
> > * spice_main_update_display_enabled:
> > * @channel: a #SpiceMainChannel
> > - * @id: display ID (if -1: set all displays)
> > + * @id: the display ID - assumed to be channel_id + monitor_id
> > * @enabled: wether display @id is enabled
> > * @update: if %TRUE, update guest display state after 1sec.
> > *
> > @@ -2968,7 +2986,7 @@ void spice_main_update_display_enabled(SpiceMainChannel *channel, int id, gboole
> > /**
> > * spice_main_channel_update_display_enabled:
> > * @channel: a #SpiceMainChannel
> > - * @id: display ID (if -1: set all displays)
> > + * @id: the display ID - assumed to be channel_id + monitor_id
> > * @enabled: wether display @id is enabled
> > * @update: if %TRUE, update guest display state after 1sec.
> > *
> > @@ -2986,33 +3004,57 @@ void spice_main_update_display_enabled(SpiceMainChannel *channel, int id, gboole
> > void spice_main_channel_update_display_enabled(SpiceMainChannel *channel, int id, gboolean enabled,
> > gboolean update)
> > {
> > - SpiceDisplayState display_state = enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED;
> > g_return_if_fail(channel != NULL);
> > g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> > g_return_if_fail(id >= -1);
> >
> > - SpiceMainChannelPrivate *c = channel->priv;
> > + GArray *monitor_configs = spice_session_get_monitor_configs(
> > + spice_channel_get_session(SPICE_CHANNEL(channel)));
> > +
> > + SpiceDisplayState display_state = enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED;
> >
> > if (id == -1) {
> > + SPICE_DEBUG("Updating all monitor configs' state to %s", enabled ? "enabled" : "disabled");
> > +
> > gint i;
> > - for (i = 0; i < G_N_ELEMENTS(c->display); i++) {
> > - c->display[i].display_state = display_state;
> > + for (i = 0; i < monitor_configs->len; i++) {
> > + g_array_index(monitor_configs, SpiceMonitorConfig, i).display_state = display_state;
> > }
> > } else {
> > - g_return_if_fail(id < G_N_ELEMENTS(c->display));
> > - if (c->display[id].display_state == display_state)
> > + bool found = false;
> > + SpiceMonitorConfig *mc = NULL;
> > + for (size_t i = 0; i < monitor_configs->len; ++i) {
> > + mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> > + if (mc->channel_id + mc->monitor_id == id) {
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!found) {
> > + g_warning("Display ID %d not found while trying to %s the display (monitor config).",
> > + id, enabled ? "enable" : "disable");
> > + return;
> > + }
> > +
> > + if (mc->display_state == display_state) {
> > return;
> > - c->display[id].display_state = display_state;
> > + }
> > +
> > + SPICE_DEBUG("Updating monitor config id: %d (channel_id: %u, monitor_id: %u) state to %s",
> > + id, mc->channel_id, mc->monitor_id, enabled ? "enabled" : "disabled");
> > +
> > + mc->display_state = display_state;
> > }
> >
> > if (update)
> > - update_display_timer(channel, 1);
> > + spice_main_channel_update_display_timer(channel, 1);
> > }
> >
> > /**
> > * spice_main_set_display_enabled:
> > * @channel: a #SpiceMainChannel
> > - * @id: display ID (if -1: set all displays)
> > + * @id: the display ID - assumed to be channel_id + monitor_id
> > * @enabled: wether display @id is enabled
> > *
> > * When sending monitor configuration to agent guest, don't set
> > diff --git a/src/channel-main.h b/src/channel-main.h
> > index fd7a3bb..0495bb2 100644
> > --- a/src/channel-main.h
> > +++ b/src/channel-main.h
> > @@ -101,6 +101,9 @@ gboolean spice_main_channel_file_copy_finish(SpiceMainChannel *channel,
> >
> > void spice_main_channel_request_mouse_mode(SpiceMainChannel *channel, int mode);
> >
> > +// TODO had to add this to the interface to access it from spice-session, what to do with it?
> > +void spice_main_channel_update_display_timer(SpiceMainChannel *channel, guint seconds);
> > +
> > #ifndef SPICE_DISABLE_DEPRECATED
> > G_DEPRECATED_FOR(spice_main_channel_clipboard_selection_grab)
> > void spice_main_clipboard_grab(SpiceMainChannel *channel, guint32 *types, int ntypes);
> > diff --git a/src/map-file b/src/map-file
> > index cdb81c3..0e4c375 100644
> > --- a/src/map-file
> > +++ b/src/map-file
> > @@ -122,6 +122,8 @@ spice_record_channel_send_data;
> > spice_record_send_data;
> > spice_session_connect;
> > spice_session_disconnect;
> > +spice_display_state_get_type;
> > +spice_session_get_monitor_config;
> > spice_session_get_channels;
> > spice_session_get_proxy_uri;
> > spice_session_get_read_only;
> > @@ -131,6 +133,8 @@ spice_session_is_for_migration;
> > spice_session_migration_get_type;
> > spice_session_new;
> > spice_session_open_fd;
> > +spice_session_update_monitor_config_dimensions;
> > +spice_session_update_monitor_config_enabled;
> > spice_session_verify_get_type;
> > spice_set_session_option;
> > spice_smartcard_channel_get_type;
> > diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file
> > index b19844c..897a303 100644
> > --- a/src/spice-glib-sym-file
> > +++ b/src/spice-glib-sym-file
> > @@ -101,6 +101,8 @@ spice_record_channel_send_data
> > spice_record_send_data
> > spice_session_connect
> > spice_session_disconnect
> > +spice_display_state_get_type
> > +spice_session_get_monitor_config
> > spice_session_get_channels
> > spice_session_get_proxy_uri
> > spice_session_get_read_only
> > @@ -110,6 +112,8 @@ spice_session_is_for_migration
> > spice_session_migration_get_type
> > spice_session_new
> > spice_session_open_fd
> > +spice_session_update_monitor_config_dimensions
> > +spice_session_update_monitor_config_enabled
> > spice_session_verify_get_type
> > spice_set_session_option
> > spice_smartcard_channel_get_type
> > diff --git a/src/spice-session-priv.h b/src/spice-session-priv.h
> > index 03005aa..77cb8c6 100644
> > --- a/src/spice-session-priv.h
> > +++ b/src/spice-session-priv.h
> > @@ -100,6 +100,16 @@ 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);
> > const gchar* spice_audio_data_mode_to_string(gint mode);
> > +
> > +GArray *spice_session_get_monitor_configs(SpiceSession *session);
> > +
> > +SpiceMonitorConfig *spice_session_get_monitor_config_mutable(SpiceSession *session,
> > + uint32_t channel_id,
> > + uint32_t monitor_id);
> > +
> > +void spice_session_add_monitor_config(SpiceSession *session, uint32_t channel_id, uint32_t display_id,
> > + uint32_t x, uint32_t y, uint32_t width, uint32_t height);
> > +
> > G_END_DECLS
> >
> > #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index b1aeb84..29a4c69 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -90,6 +90,11 @@ struct _SpiceSessionPrivate {
> > GStrv secure_channels;
> > gint color_depth;
> >
> > + /* A list of monitor configs for all channels and monitors. It is filled up
> > + * with monitors_config messages as they arrive on display channels.
> > + */
> > + GArray *monitor_configs;
> > +
> > int connection_id;
> > int protocol;
> > SpiceChannel *cmain; /* weak reference */
> > @@ -288,6 +293,8 @@ static void spice_session_init(SpiceSession *session)
> > s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> > s->glz_window = glz_decoder_window_new();
> > update_proxy(session, NULL);
> > +
> > + s->monitor_configs = g_array_new(FALSE, TRUE, sizeof(SpiceMonitorConfig));
> > }
> >
> > static void
> > @@ -340,6 +347,8 @@ spice_session_dispose(GObject *gobject)
> > g_clear_object(&s->proxy);
> > g_clear_object(&s->webdav);
> >
> > + g_array_free(s->monitor_configs, TRUE);
> > +
> > /* Chain up to the parent class */
> > if (G_OBJECT_CLASS(spice_session_parent_class)->dispose)
> > G_OBJECT_CLASS(spice_session_parent_class)->dispose(gobject);
> > @@ -2823,6 +2832,149 @@ gboolean spice_session_is_for_migration(SpiceSession *session)
> > return session->priv->for_migration;
> > }
> >
> > +void spice_session_add_monitor_config(SpiceSession *session, uint32_t channel_id, uint32_t monitor_id,
> > + uint32_t x, uint32_t y, uint32_t width, uint32_t height)
> > +{
> > + GArray *monitor_configs = spice_session_get_monitor_configs(session);
> > + g_assert(monitor_configs != NULL);
> > +
> > + SpiceMonitorConfig mc = {
> > + .channel_id = channel_id,
> > + .monitor_id = monitor_id,
> > + .x = x,
> > + .y = y,
> > + .width = width,
> > + .height = height,
> > + .display_state = DISPLAY_DISABLED
> > + };
> > +
> > + SPICE_DEBUG("Adding new monitor_config channel_id: %u, monitor_id: %u, +%u+%u:%ux%u",
> > + channel_id, monitor_id, x, y, width, height);
> > +
> > + g_array_append_val(monitor_configs, mc);
> > +}
> > +
> > +GArray *spice_session_get_monitor_configs(SpiceSession *session)
> > +{
> > + g_assert(session != NULL);
> > +
> > + return session->priv->monitor_configs;
> > +}
> > +
> > +SpiceMonitorConfig *spice_session_get_monitor_config_mutable(SpiceSession *session,
> > + uint32_t channel_id,
> > + uint32_t monitor_id)
> > +{
> > + GArray *monitor_configs = spice_session_get_monitor_configs(session);
> > + g_assert(monitor_configs != NULL);
> > +
> > + for (size_t i = 0; i < monitor_configs->len; ++i) {
> > + SpiceMonitorConfig *d = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> > + if (d->channel_id == channel_id && d->monitor_id == monitor_id) {
> > + return d;
> > + }
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/**
> > + * spice_session_get_monitor_config:
> > + * @session: a #SpiceSession
> > + * @channel_id: a channel ID
> > + * @monitor_id: a monitor ID
> > + *
> > + * Returns a #SpiceMonitorConfig identified by the @channel_id and @monitor_id.
> > + *
> > + * Returns: a pointer to SpiceMonitorConfig if found, %NULL otherwise
>
> Why do you need that new API and expose the structure? I suggest to
> make it a seperate commit with rationale.
A very good question. Apparently, I don't need it on the API anymore. I
probably needed it in v1 when the guest_output_id was there. I'll
remove it from the API.
> > + *
> > + * Since: 0.36
> > + **/
> > +const SpiceMonitorConfig *spice_session_get_monitor_config(SpiceSession *session,
> > + uint32_t channel_id,
> > + uint32_t monitor_id)
> > +{
> > + return spice_session_get_monitor_config_mutable(session, channel_id, monitor_id);
> > +}
> > +
> > +/**
> > + * spice_session_update_monitor_config_dimensions:
> > + * @session: a #SpiceSession
> > + * @channel_id: a channel ID
> > + * @monitor_id: a monitor ID
> > + * @x: X coordinate of the monitor
> > + * @y: Y coordinate of the monitor
> > + * @width: width of the monitor
> > + * @height: height of the monitor
> > + *
> > + * Sets the dimensions (@x, @y, @width, @height) of a #SpiceMonitorConfig
> > + * identified with @channel_id and @monitor_id.
> > + *
> > + * Since: 0.36
> > + **/
> > +void spice_session_update_monitor_config_dimensions(SpiceSession *session,
> > + uint32_t channel_id, uint32_t monitor_id,
> > + uint32_t x, uint32_t y,
> > + uint32_t width, uint32_t height, gboolean update)
> > +{
> > + SpiceMonitorConfig *mc = spice_session_get_monitor_config_mutable(session, channel_id, monitor_id);
> > +
> > + g_return_if_fail(mc != NULL);
> > +
> > + if (mc->x == x &&
> > + mc->y == y &&
> > + mc->width == width &&
> > + mc->height == height) {
> > + return;
> > + }
> > +
> > + SPICE_DEBUG("Updating monitor config channel_id: %u, monitor_id: %u, +%u+%u-%ux%u",
> > + channel_id, monitor_id, x, y, width, height);
> > +
> > + mc->x = x;
> > + mc->y = y;
> > + mc->width = width;
> > + mc->height = height;
> > +
> > + if (update) {
> > + spice_main_channel_update_display_timer((SpiceMainChannel *) session->priv->cmain, 1);
> > + }
> > +}
> > +
> > +/**
> > + * spice_session_update_monitor_config_enabled:
> > + * @session: a #SpiceSession
> > + * @channel_id: a channel ID
> > + * @monitor_id: a monitor ID
> > + * @enabled: The enabled flag to set
> > + *
> > + * Sets the @enabled flag of a #SpiceMonitorConfig
> > + * identified with @channel_id and @monitor_id.
> > + *
> > + * Since: 0.36
> > + **/
> > +void spice_session_update_monitor_config_enabled(SpiceSession *session,
> > + uint32_t channel_id, uint32_t monitor_id,
> > + gboolean enabled, gboolean update)
> > +{
> > + SpiceMonitorConfig *mc = spice_session_get_monitor_config_mutable(session, channel_id, monitor_id);
> > +
> > + g_return_if_fail(mc != NULL);
> > +
> > + if (mc->display_state == (enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED)) {
> > + return;
> > + }
> > +
> > + SPICE_DEBUG("%s monitor config channel_id: %u, monitor_id: %u",
> > + enabled ? "Enabling" : "Disabling", channel_id, monitor_id);
> > +
> > + mc->display_state = enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED;
> > +
> > + if (update) {
> > + spice_main_channel_update_display_timer((SpiceMainChannel *) session->priv->cmain, 1);
> > + }
> > +}
> > +
> > G_GNUC_INTERNAL
> > void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel)
> > {
> > diff --git a/src/spice-session.h b/src/spice-session.h
> > index cfe02b1..16523bb 100644
> > --- a/src/spice-session.h
> > +++ b/src/spice-session.h
> > @@ -23,6 +23,8 @@
> > #endif
> >
> > #include <glib-object.h>
> > +#include <stdint.h>
> > +
> > #include "spice-types.h"
> > #include "spice-uri.h"
> > #include "spice-glib-enums.h"
> > @@ -103,6 +105,22 @@ struct _SpiceSessionClass
> > gchar _spice_reserved[SPICE_RESERVED_PADDING];
> > };
> >
> > +typedef enum {
> > + DISPLAY_UNDEFINED,
> > + DISPLAY_DISABLED,
> > + DISPLAY_ENABLED,
> > +} SpiceDisplayState;
> > +
> > +typedef struct {
> > + uint32_t channel_id;
> > + uint32_t monitor_id;
> > + uint32_t x;
> > + uint32_t y;
> > + uint32_t width;
> > + uint32_t height;
> > + SpiceDisplayState display_state;
> > +} SpiceMonitorConfig;
> > +
> > GType spice_session_get_type(void);
> >
> > SpiceSession *spice_session_new(void);
> > @@ -115,6 +133,19 @@ gboolean spice_session_get_read_only(SpiceSession *session);
> > SpiceURI *spice_session_get_proxy_uri(SpiceSession *session);
> > gboolean spice_session_is_for_migration(SpiceSession *session);
> >
> > +const SpiceMonitorConfig *spice_session_get_monitor_config(SpiceSession *session,
> > + uint32_t channel_id,
> > + uint32_t monitor_id);
> > +
> > +void spice_session_update_monitor_config_dimensions(SpiceSession *session,
> > + uint32_t channel_id, uint32_t monitor_id,
> > + uint32_t x, uint32_t y,
> > + uint32_t width, uint32_t height, gboolean update);
> > +
> > +void spice_session_update_monitor_config_enabled(SpiceSession *session,
> > + uint32_t channel_id, uint32_t monitor_id,
> > + gboolean enabled, gboolean update);
> > +
> > G_END_DECLS
> >
> > #endif /* __SPICE_CLIENT_SESSION_H__ */
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index 853c9df..e361c91 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -256,7 +256,7 @@ static void update_ready(SpiceDisplay *display)
> > * application will manage the state of the displays.
> > */
> > if (d->resize_guest_enable) {
> > - spice_main_channel_update_display_enabled(d->main, get_display_id(display), ready, TRUE);
> > + spice_session_update_monitor_config_enabled(d->session, d->channel_id, d->monitor_id, ready, TRUE);
> > }
> >
> > if (d->ready == ready)
> > @@ -1204,8 +1204,9 @@ static void recalc_geometry(GtkWidget *widget)
> > d->ww, d->wh, zoom);
> >
> > if (d->resize_guest_enable)
> > - spice_main_channel_update_display(d->main, get_display_id(display),
> > - d->area.x, d->area.y, d->ww / zoom, d->wh / zoom, TRUE);
> > + spice_session_update_monitor_config_dimensions(d->session, d->channel_id, d->monitor_id,
> > + d->area.x, d->area.y,
> > + d->ww / zoom, d->wh / zoom, TRUE);
> > }
> >
> > /* ---------------------------------------------------------------- */
> > diff --git a/tools/spicy.c b/tools/spicy.c
> > index 263c15f..fc2b88a 100644
> > --- a/tools/spicy.c
> > +++ b/tools/spicy.c
> > @@ -614,11 +614,11 @@ static void menu_cb_resize_to(GtkAction *action G_GNUC_UNUSED,
> >
> > gtk_widget_show_all(dialog);
> > if (gtk_dialog_run(GTK_DIALOG (dialog)) == GTK_RESPONSE_APPLY) {
> > - spice_main_channel_update_display_enabled(win->conn->main, win->id + win->monitor_id, TRUE,
> > - FALSE);
> > - spice_main_channel_update_display(
> > - win->conn->main,
> > - win->id + win->monitor_id,
> > + spice_session_update_monitor_config_enabled(win->conn->session, win->id, win->monitor_id,
> > + TRUE, FALSE);
> > + spice_session_update_monitor_config_dimensions(
> > + win->conn->session,
> > + win->id, win->monitor_id,
> > gtk_spin_button_get_value_as_int(GTK_SPIN_BUTTON(spin_x)),
> > gtk_spin_button_get_value_as_int(GTK_SPIN_BUTTON(spin_y)),
> > gtk_spin_button_get_value_as_int(GTK_SPIN_BUTTON(spin_width)),
> > @@ -1439,10 +1439,7 @@ static void del_window(spice_connection *conn, SpiceWindow *win)
> >
> > g_debug("del display monitor %d:%d", win->id, win->monitor_id);
> > conn->wins[win->id * CHANNELID_MAX + win->monitor_id] = NULL;
> > - if (win->id > 0)
> > - spice_main_channel_update_display_enabled(conn->main, win->id, FALSE, TRUE);
> > - else
> > - spice_main_channel_update_display_enabled(conn->main, win->monitor_id, FALSE, TRUE);
> > + spice_session_update_monitor_config_enabled(conn->session, win->id, win->monitor_id, FALSE, TRUE);
> > spice_main_channel_send_monitor_config(conn->main);
> >
> > destroy_spice_window(win);
> > --
> > 2.18.0
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
> All in all, it looks to me like you are moving away from the static
> monitor config array, to a dynamic array that can support a mix of
> channel_id & monitor_id. In short, it feels wrong because that's not
> something we want to support. Or do we?
I think we have to, just because of the QXL + vGPU streaming case. I'm
sure we're still open to suggestions how to do it better, so far nobody
came up with a better way.
Thanks for the comments,
Lukas
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel