[Spice-devel] [RFC/POC PATCH spice-gtk 07/16] Rework the handling of monitors_config
Christophe de Dinechin
cdupontd at redhat.com
Fri Jun 22 13:46:58 UTC 2018
> On 22 Jun 2018, at 14:13, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> On Fri, 2018-06-22 at 12:05 +0200, Christophe de Dinechin wrote:
>>> On 20 Jun 2018, at 11:45, Lukáš Hrázký <lhrazky at 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ý <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?
>>>
>>> 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.
That’s how I understood it, but careful reading convinced me that’s not exactly what you were saying, hence the suggestion to clarify.
What about:
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). Thus,
the data was not encapsulated, and this patch needs to change the
API in order to add new fields to the monitor configuration.
This commit takes the opportunity of this API change to improve
encapsulation, 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).
I believe this is what you mean.
>
>> 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ý <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.
>>>
>>> (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.
Oh, I understood that. My question is why not either:
- add the entry when you enable?
- or pre-populate “in order", and then index directly instead of searching?
Does that make sense?
>
>>>
>>> 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.
You add meat to my argument about lack of encapsulation there ;-)
> 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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>
>>>>
>>>
>>> _______________________________________________
>>> 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