[Spice-devel] [RFC/POC PATCH spice-gtk 07/16] Rework the handling of monitors_config
Christophe de Dinechin
cdupontd at redhat.com
Tue Jun 19 15:06:20 UTC 2018
Hi Lukas,
Please don’t be disturbed by the many comments. Overall, looks like really good progress, and insightful analysis of the problem. Probably will require several more iterations, but that’s entirely expected. Thanks a lot.
> On 5 Jun 2018, at 17:30, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> The code relied on spice_main_channel_update_display() called from the
> client application to create a list of monitor_configs to be sent to the
> server (to enable/disable monitors and set resolution). 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.
I have trouble following the reasoning. It’s only adding a client-visible attribute that requires an API change, right? Is the output_id necessarily visible to the client, or could that be kept internal inside the SPICE API?
So I think the feeling is right (you want to hide some internal details) but the way it’s spelled out does not sound right to me.
>
> 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 (strictly
> speaking, they never did, as the list was static and they only filled
> them up with values and enabled/disabled the items, which also has it's
> problems).
>
> 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.
> Presumably other clients can use whatever they like as the id, but since
> the monitors sent in monitors_config on the main channel need to match
> those received in monitors_config on the display channels, they probably
> need to do basically the same. The code is making a lot of assumptions
> here.
>
> So, we make one more assumption that all clients use the channel_id +
> monitor_id to pass to spice_main_channel_update_display{,_enabled}() as
> the id argument and use that to look up the correct monitor_config.
>
> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> ---
> spice-common | 2 +-
> src/channel-display.c | 23 ++++++-
> src/channel-main.c | 139 ++++++++++++++++++++++++---------------
> src/channel-main.h | 2 +-
> src/map-file | 5 ++
> src/spice-glib-sym-file | 5 ++
> src/spice-session-priv.h | 1 +
> src/spice-session.c | 129 ++++++++++++++++++++++++++++++++++++
> src/spice-session.h | 30 +++++++++
> 9 files changed, 278 insertions(+), 58 deletions(-)
>
> diff --git a/spice-common b/spice-common
> index 8096b12..11aeda6 160000
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit 8096b1206bb266b8d0b80b3e4c0d36fc621d772d
> +Subproject commit 11aeda6b04628258f0cb604727f6f800710a2d9e
> diff --git a/src/channel-display.c b/src/channel-display.c
> index e0520a3..9e83e43 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1051,8 +1051,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
> + if (!spice_channel_test_capability(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
> + g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> + }
>
> if (preferred_compression != SPICE_IMAGE_COMPRESSION_INVALID) {
> spice_display_channel_change_preferred_compression(channel, preferred_compression);
> @@ -1868,6 +1870,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;
> @@ -1906,6 +1909,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_find_monitor_config(session, channel->priv->channel_id, i)) {
> + spice_session_update_monitor_config(session, channel->priv->channel_id, i,
> + head->x, head->y, head->width, head->height);
> + } 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.
What is the point of doing this? Why not add code when enabling a monitor that adds it if not present?
Also, as implemented, it’s O(N^2) since there is a loop in find_monitor_config.
Or if you want a dense array, then you don’t need a GArray, you may as well pre-allocate and use a two-dimentional array indexed by channel_id and monitor_id, that will be faster.
But at present, it’s the worse of both world: dense array gobbling memory, with loops to search that can’t take advantage of the fact that it’s dense so you have to search every time instead of direct indexing.
Note that going dense would avoid the risk of having two entries with the same monitor_id+channel_id.
> + for (i = config->count; i < config->max_allowed; i++) {
> + if (!spice_session_find_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 3d682d6..a35e9b9 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -55,20 +55,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;
> @@ -105,7 +91,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;
> @@ -1115,13 +1100,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)));
> +
DRY - See comment below on reuse of that expression
> 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;
> + }
> }
> }
>
> @@ -1135,18 +1124,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,
> @@ -1481,15 +1471,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)));
> +
DRY - See comment on expression reuse.
> + 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;
> @@ -1513,12 +1506,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;
> }
> @@ -2630,7 +2630,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 or equivalent
> * @x: x position
> * @y: y position
> * @width: display width
> @@ -2656,7 +2656,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 or equivalent
Frankly, the added comment does not clarify anything for me ;-) Maybe remove “or equivalent”? Or explain what you mean by that. The code you added has the channel_id+monitor_id hardcoded, AFAICT, not “equivalent"
Or maybe phrase as “id must be channel_id+monitor_id for monitor configuration to be found”.
> * @x: x position
> * @y: y position
> * @width: display width
> @@ -2675,8 +2675,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);
> @@ -2684,19 +2682,32 @@ 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;
> -
> - g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
It looks like the test above was removed. You replaced that with a g_warning and return.
But what if we find the same channel_id+monitor_id twice? Should we fail in that case too?
> + GArray *monitor_configs = spice_session_get_monitor_configs(
> + spice_channel_get_session(SPICE_CHANNEL(channel)));
You reuse that expression several times in the patch. Maybe add spice_get_channel_monitor_configs(channel)?
>
> - SpiceDisplayConfig display = {
> - .x = x, .y = y, .width = width, .height = height,
> - .display_state = c->display[id].display_state
> - };
> + int found = 0;
Personal preference: either make ‘found' a boolean, or count how many you have and make it an error to have more than one.
> + 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) {
Extraneous parentheses (personal preference, please ignore if you disagree)
> + found = 1;
> + break;
Minor style preference: add !found to the for loop, makes it IMO clearer when the loop exits.
> + }
> + }
>
> - if (memcmp(&display, &c->display[id], sizeof(SpiceDisplayConfig)) == 0)
> + if (!found) {
> + g_warning("Display ID %d not found while trying to update the display (monitor config).", id);
> return;
> + }
There was a memcmp that checked the content fo the monitor config, presumably to avoid sending updates if we call the function with the existing values. Any reason for removing it?
>
> - c->display[id] = display;
> + 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);
> +
> + mc->x = x;
> + mc->y = y;
> + mc->width = width;
> + mc->height = height;
>
> if (update)
> update_display_timer(channel, 1);
> @@ -2705,7 +2716,7 @@ void spice_main_channel_update_display(SpiceMainChannel *channel, int id, int x,
> /**
> * spice_main_set_display:
> * @channel: a #SpiceMainChannel
> - * @id: display ID
> + * @id: the display ID - assumed to be channel_id + monitor_id or equivalent
> * @x: x position
> * @y: y position
> * @width: display width
> @@ -2947,7 +2958,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 or equivalent, -1 to update all
> * @enabled: wether display @id is enabled
> * @update: if %TRUE, update guest display state after 1sec.
> *
> @@ -2972,7 +2983,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 or equivalent, -1 to update all
> * @enabled: wether display @id is enabled
> * @update: if %TRUE, update guest display state after 1sec.
> *
> @@ -2990,23 +3001,43 @@ 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)));
> +
DRY
> + 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)
> + int found = 0;
> + 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 = 1;
> + break;
> + }
> + }
DRY. Consider a function that returns a SpiceMonitorConfig * or NULL. Also would make it simpler to update the “channel_id + monitor_id” hypothesis. Right next to spice_session_find_monitor_config, just need to find a different name ;-)
> +
> + if (!found) {
> + g_warning("Display ID %d not found while trying to %s the display (monitor config).",
> + id, enabled ? "enable" : "disable");
> 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)
> @@ -3016,7 +3047,7 @@ void spice_main_channel_update_display_enabled(SpiceMainChannel *channel, int id
> /**
> * 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 or equivalent, -1 to update all
> * @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 530378d..bbc2fea 100644
> --- a/src/channel-main.h
> +++ b/src/channel-main.h
> @@ -112,7 +112,7 @@ G_DEPRECATED_FOR(spice_main_channel_clipboard_selection_request)
> void spice_main_clipboard_request(SpiceMainChannel *channel, guint32 type);
>
> G_DEPRECATED_FOR(spice_main_channel_update_display)
> -void spice_main_set_display(SpiceMainChannel *channel, int id,int x, int y, int width, int height);
> +void spice_main_set_display(SpiceMainChannel *channel, int id, int x, int y, int width, int height);
> G_DEPRECATED_FOR(spice_main_update_display)
> void spice_main_update_display(SpiceMainChannel *channel, int id, int x, int y, int width,
> int height, gboolean update);
> diff --git a/src/map-file b/src/map-file
> index cdb81c3..72ee2f3 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -120,9 +120,13 @@ spice_port_write_finish;
> spice_record_channel_get_type;
> spice_record_channel_send_data;
> spice_record_send_data;
> +spice_session_add_monitor_config;
> spice_session_connect;
> spice_session_disconnect;
> +spice_display_state_get_type;
> +spice_session_find_monitor_config;
> spice_session_get_channels;
> +spice_session_get_monitor_configs;
> spice_session_get_proxy_uri;
> spice_session_get_read_only;
> spice_session_get_type;
> @@ -131,6 +135,7 @@ spice_session_is_for_migration;
> spice_session_migration_get_type;
> spice_session_new;
> spice_session_open_fd;
> +spice_session_update_monitor_config;
> 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..c186624 100644
> --- a/src/spice-glib-sym-file
> +++ b/src/spice-glib-sym-file
> @@ -99,9 +99,13 @@ spice_port_write_finish
> spice_record_channel_get_type
> spice_record_channel_send_data
> spice_record_send_data
> +spice_session_add_monitor_config
> spice_session_connect
> spice_session_disconnect
> +spice_display_state_get_type
> +spice_session_find_monitor_config
> spice_session_get_channels
> +spice_session_get_monitor_configs
> spice_session_get_proxy_uri
> spice_session_get_read_only
> spice_session_get_type
> @@ -110,6 +114,7 @@ spice_session_is_for_migration
> spice_session_migration_get_type
> spice_session_new
> spice_session_open_fd
> +spice_session_update_monitor_config
> 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..12b0764 100644
> --- a/src/spice-session-priv.h
> +++ b/src/spice-session-priv.h
> @@ -100,6 +100,7 @@ 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);
> +
Spurious space that I really don’t care about, keep it if you want, just was in an “Annoying Mood” day ;-)
> G_END_DECLS
>
> #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */
> diff --git a/src/spice-session.c b/src/spice-session.c
> index 57acc63..c170110 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -90,6 +90,14 @@ struct _SpiceSessionPrivate {
> GStrv secure_channels;
> gint color_depth;
>
> + /* A list of monitor configs for all channels and monitors. It is filled up
> + * as display channels are created from their monitors_config messages. The
> + * client application then updates these through
> + * spice_main_channel_update_display{,_enabled} to enable/disable them and
> + * set the resolution.
> + */
> + GArray *monitor_configs;
> +
> int connection_id;
> int protocol;
> SpiceChannel *cmain; /* weak reference */
> @@ -294,6 +302,8 @@ static void spice_session_init(SpiceSession *session)
> s->glz_window = glz_decoder_window_new();
> update_proxy(session, NULL);
>
> + s->monitor_configs = g_array_new(FALSE, TRUE, sizeof(SpiceMonitorConfig));
> +
> s->usb_manager = spice_usb_device_manager_get(session, &err);
> if (err != NULL) {
> SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", err->message);
> @@ -349,6 +359,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);
> @@ -2800,6 +2812,123 @@ gboolean spice_session_is_for_migration(SpiceSession *session)
> return session->priv->for_migration;
> }
>
> +/**
> + * spice_session_get_monitor_configs:
> + * @session: a #SpiceSession
> + *
> + * Returns: a pointer to the array of SpiceMonitorConfigs
> + * Since: 0.36
> + **/
> +GArray *spice_session_get_monitor_configs(SpiceSession *session)
> +{
> + g_assert(session != NULL);
> +
> + return session->priv->monitor_configs;
> +}
> +
> +/**
> + * spice_session_find_monitor_config:
> + * @session: a #SpiceSession
> + * @channel_id: a channel ID
> + * @monitor_id: a monitor ID
> + *
> + * Looks up a monitor config which has the channel_id and monitor_id equal to
> + * those specified in aruments.
> + *
> + * Returns: a pointer to SpiceMonitorConfig if found, %NULL otherwise
> + * Since: 0.36
> + **/
> +SpiceMonitorConfig *spice_session_find_monitor_config(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_add_monitor_config:
> + * @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
> + *
> + * Appends a new #SpiceMonitorConfig to the end of monitor_configs array under
> + * the session and sets its attributes to those specified in arguments.
> + *
> + * Since: 0.36
> + **/
> +void spice_session_add_monitor_config(SpiceSession *session, uint32_t channel_id, uint32_t monitor_id,
> + int x, int y, int width, int height)
> +{
> + GArray *monitor_configs = spice_session_get_monitor_configs(session);
> + g_assert(monitor_configs != NULL);
> +
> + g_return_if_fail(x >= 0);
> + g_return_if_fail(y >= 0);
> + g_return_if_fail(width >= 0);
> + g_return_if_fail(height >= 0);
Just curious why define an interface that barfs on negative value, but does not take unsigned? Copy-paste inheritance, OK, but still, don’t know why it’s that way elsewhere.
> +
> + 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 idx=%u +%d+%d:%dx%d",
> + monitor_configs->len, x, y, width, height);
> +
> + g_array_append_val(monitor_configs, mc);
> +}
> +
> +/**
> + * spice_session_update_monitor_config:
> + * @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
> + *
> + * Looks up #SpiceMonitorConfig with channel_id and monitor_id equal to those
> + * in arguments in the session's monitor_configs array and sets its x, y, width
> + * and height to those specified in arguments.
> + *
> + * Since: 0.36
> + **/
> +void spice_session_update_monitor_config(SpiceSession *session, uint32_t channel_id, uint32_t monitor_id,
> + int x, int y, int width, int height)
> +{
> + SpiceMonitorConfig *mc = spice_session_find_monitor_config(session, channel_id, monitor_id);
> +
> + g_return_if_fail(mc != NULL);
> +
> + SPICE_DEBUG("Updating monitor config channel_id: %u, monitor_id: %u, +%d+%d-%dx%d",
> + channel_id, monitor_id, x, y, width, height);
> +
> + mc->x = x;
> + mc->y = y;
> + mc->width = width;
> + mc->height = height;
> +}
Seems a bit redundant with spice_main_channel_update_display. What’s the added value? Looks to me like one should call the other.
> +
> 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..6d97ab2 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;
> + int x;
> + int y;
> + int width;
> + int height;
> + SpiceDisplayState display_state;
> +} SpiceMonitorConfig;
> +
> GType spice_session_get_type(void);
>
> SpiceSession *spice_session_new(void);
> @@ -115,6 +133,18 @@ gboolean spice_session_get_read_only(SpiceSession *session);
> SpiceURI *spice_session_get_proxy_uri(SpiceSession *session);
> gboolean spice_session_is_for_migration(SpiceSession *session);
>
> +GArray *spice_session_get_monitor_configs(SpiceSession *session);
> +
> +SpiceMonitorConfig *spice_session_find_monitor_config(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,
> + int x, int y, int width, int height);
> +
> +void spice_session_update_monitor_config(SpiceSession *session, uint32_t channel_id, uint32_t monitor_id,
> + int x, int y, int width, int height);
> +
> G_END_DECLS
>
> #endif /* __SPICE_CLIENT_SESSION_H__ */
> --
> 2.17.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list