On Fri, 2018-06-22 at 12:05 +0200, Christophe de Dinechin wrote:
> > On 20 Jun 2018, at 11:45, Lukáš Hrázký <lhra...@redhat.com> wrote:
> >
> > On Tue, 2018-06-19 at 17:06 +0200, Christophe de Dinechin wrote:
> > > 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.
> >
> > No worries, thanks for the review!
> >
> > > > On 5 Jun 2018, at 17:30, Lukáš Hrázký <lhra...@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?
> >
> > I'm also having a hard time understanding what exactly you don't
> > understand or find confusing from the text :)
>
> The part I find confusing is the “thus adding… requires”. It seems that you
> believe if follows from the previous sentence. And it does follow iff the
> output_id needs to be visible. But since the output_id is not visible today
> (because it just does not exist), therefore it seems it’s not needed…
Hmm... The output_id does not need to be visible. But before this
patch, all monitors_config fields had to be visible by design, which is
what the first sentence of the paragraph in question is trying to say.
> But if I’m the only one confused, I’ll shut up.
>
> > You're right, the practical problem is I don't want to make the
> > output_id visible to the client app (I use "client app" = e.g. virt-
> > viewer, "client" could mean either virt-viewer or spice-gtk) and have
> > to change the API.
> >
> > The underlying reason for this problem is that items to the
> > monitors_config list are added when the client app calls the
> > spice_main_channel_update_display() API method, with all the data for
> > the new item in arguments. A better approach (the more "standard" one,
> > IMO) is to add items to the list inside spice-gtk and only use the API
> > method to update the items.
> >
> > So that's what I did and I tried to describe that in the text. I have
> > no problem fixing the description, I'm just not sure how...
> >
> > > 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ý <lhra...@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.
> >
> > (as a side note, I think some appearances of "dense" should have been
> > "sparse" in your text, which makes it quite confusing)
>
> I don’t think so. I think that what was confusing is that my last sentence
> was false. If you index a[i][j], you can have multiple entries with the same
> i+j. Don’t know what I was thinking.
>
> >
> > :) You know I usually have a more high-level approach to things... I
> > guess it depends on the point of view.
> >
> > So first off, I want the convenience of a dynamic container here and I
> > don't care about the memory. I know about the O(N^2) and I don't care
> > much about that either. We're about to have 5 or less items in the
> > array in majority of cases and no prospects of it ever going much
> > higher.
>
> I’d refer you to
> https://septheory.wordpress.com/2018/06/04/premature-optimization/, but I
> know you don’t like when I quote my own stuff… :-)
>
> The fact that it’s not entirely insufferable because we happen to have a very
> small number of items is not an excuse for having code that can’t seem to
> decide if all items in the array are populated or not. Pick one or the other,
> I don’t care, but choose :-)
But the population of the array by disabled monitor_configs here is
valid. The configs are there, just the monitors are disabled. When the
client app wants to enable them, it calls
spice_main_channel_update_display_enabled(), which expects the item to
be there, otherwise it errors out.
> >
> > What I want is to have a monitor_config item in the array for every
> > monitor that exists. This simplifies the concept in the way that you
> > only create the items in one place (here). In the update methods
> > (spice-gtk API), you know the item should always be there and if it
> > isn't you error out (a very good thing to have on the API). If the
> > update methods were to create missing items, you couldn't error out if
> > they're missing. And there are also fields (like the new output_id)
> > which don't have a clear default value, so in the update method, you
> > don't really know what to put in there (if the client app uses a wrong
> > ID). So this approach is less error prone and has a better defined
> > behaviour.
> >
> > About the preallocated (that would be "sparse", right?) array, that was
> > there before. I find it an unnecessarily low-level approach here (sure,
> > can just be me :)).
>
> Oh no, it’s not only you ;-)
>
>
> > The only real advantage I see is the two entries
> > problem. OTOH, you need to distinguish which items are set and which
> > are unset. As I said, I prefer to have a managed dynamic array and be
> > done with it :)
>
> I’m fine with a dynamic array. If you decide that’s what it is, then don’t
> pre-populate it. It will only accelerate all the search loops.
>
> >
> > > > + 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"
> >
> > Yeah :) I don't know really, what to put in the comment for this mess
> > :)
>
> I’d say just drop “or equivalent”.
>
> > The "equivalent" is supposed to mean "anything that turns out to have
> > the same value", because under the assumptions that you either have a
> > single channel with multiple monitors or multiple channels with a
> > single monitor each, for example the expression "channel_id ?
> > channel_id : monitor_id" happens to have the same value and is also
> > used elsewhere in the code.... :)
>
> Well, it only has the same value under the assumption that one of them is
> zero. If that’s really is the assumption you are talking about, then write it
> as:
>
> assumed to be channel_id+monitor_id with at least one of them being zero
Ok.
> >
> > > 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?
> >
> > No idea. I suppose not failing (e.g. returning without updating
> > anything) is slightly better, because then it has some chance to
> > "work". Adding a warning for that case might be a good idea though.
>
> OK
>
> >
> > > > + 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)?
> >
> > Yeah, maybe. I find these getters somewhat verbose and confusing, but
> > I'm reluctant to add more "aggregate" getters that increase the amount
> > of glue code. It doesn't save that much and adds another level when you
> > go looking for what it actually does. But a matter of taste I guess, I
> > can add that if people want.
>
> Adding a “spice_channel_get_monitor_configs(channel)”, whether it’s a macro
> or a function, is a win in readability and encapsulation.
I don't think it's such a win in readability, because I myself would
then (if I saw this) think monitors_config is an attribute of the
channel.
But it is shorter, yeah.
> > > > - 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.
> >
> > I wasn't sure of boolean in C, I'll change it.
> >
> > > > + 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)
> >
> > Yeah, not sure where they came from, I'll remove them.
> >
> > > > + found = 1;
> > > > + break;
> > >
> > > Minor style preference: add !found to the for loop, makes it IMO clearer
> > > when the loop exits.
> >
> > OK.
> >
> > > > + }
> > > > + }
> > > >
> > > > - 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?
> >
> > Ah, I missed that, need to fix it, thanks.
> >
> > > >
> > > > - 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 ;-)
> >
> > TBH I considered that, but I just couldn't get myself to write such an
> > ugly function (that could potentially be reused) :D
> >
> > But I suppose I should, though it's only repeated twice.
>
>
> >
> > > > +
> > > > + 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.
> >
> > True, I should fix that :)
> >
> > > > +
> > > > + 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.
> >
> > Not really, this one takes channel_id and monitor_id, the other the id
> > = channel_id + monitor_id. I think it's clear you can't call this one
> > from there and it's a bad idea to call that one (which really should be
> > deprecated and I plan to look into it in the next version of the
> > series) from here…
>
> Ah, if the plan is to deprecate it, then it makes sense.
>
> >
> > Thanks,
> > Lukas
> >
> > > > +
> > > > 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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
