[Spice-devel] monitors_config rework: proposed protocol changes

[Jonathon Jongsma]

Apologies for the delay in replying to this email. It's an excellent
summary of the options.

On Tue, 2018-06-26 at 15:34 +0200, Lukáš Hrázký wrote:
> Hi all,
> 
> after posting the PoC patch series for the monitors_config rework and
> the discussion there getting a bit hard to follow, I thought I'd try
> to
> summarize the proposed protocol. There are also spice-gtk API changes
> to consider, which I've decided to leave for a later email.
> 
> There are two variants for consideration right now:
> 
> 
> Variant 1
> =========
> 
> server -> client
> ----------------
> 
> struct SpiceMsgDisplayHead {
> -   uint32_t id;
> +   uint32_t monitor_id;
>     uint32_t surface_id;
> +   int32_t guest_output_id;
>     uint32_t width;
>     uint32_t height;
>     uint32_t x;
>     uint32_t y;
>     uint32_t flags;
> +   uint8_t reserved[?];
> };
> 
> struct SpiceMsgDisplayMonitorsConfig {
>     uint16_t count;
>     uint16_t max_allowed;
>     struct SpiceMsgDisplayHead heads[0];
> };
> 
> 
> client -> server -> vd_agent
> ----------------------------
> 
> struct VDAgentMonConfig {
> +   uint32_t channel_id;
> +   uint32_t monitor_id;
> +   int32_t guest_output_id;
>     uint32_t height;
>     uint32_t width;
>     uint32_t depth;
>     int32_t x;
>     int32_t y;
> +   uint8_t reserved[?];
> };
> 
> struct VDAgentMonitorsConfig {
>     uint32_t num_of_monitors;
>     uint32_t flags;
>     struct VDAgentMonConfig monitors[];
> };
> 
> 
> The "SpiceMsgDisplayHead::id" is renamed to "monitor_id" for
> consistency. It is defined in spice-common, so I believe we can do
> that
> without breaking anything.
> 
> The name of the "guest_output_id" is subject to discussion, I'd
> prefer
> to make it clear it is an ID originating from the guest.
> "guest_display_id" is an option.
> 
> The value of the "guest_output_id" would be a zero-based sequence, an
> index of an output in xrandr, with -1 meaning the value is unset. (as
> opposed to the v1 of the patch series, where the sequence was one-
> based, 0 meaning it's unset).
> 
> The "reserved" fields are added based on Christophe de Dinechin's
> suggestion to prepare for possible features described in [1].
> 
> 
> Variant 2
> =========
> 
> The protocol changes are the same as in Variant 1, except we do not
> add
> the guest_output_id to the messages (and only add channel_id and
> monitor_id to VDAgentMonConfig). The guest_output_id can be kept in a
> map on the server, to which the key would be the (channel_id,
> monitor_id) pair.
> 
> The guest_output_id can then be fetched from the map when needed and
> after that the mechanisms are identical in theory. In practice the
> guest_output_id is needed mainly (and most probably only) in messages
> to the vd_agent. As of now for example the VDAgentMonitorsConfig
> message is forwarded to the vd_agent as-is on the server, which would
> need bigger code changes and different messages for client -> server
> and for server -> agent.
> 
> Variant 2 would also require to change the protocol of
> SpiceMsgcMousePosition (the mouse position even sent from the client
> to
> the server), which now sends a display_id, which is equivalent to
> guest_output_id, but now would have to send (channel_id, monitor_id).
> Doing this is not necessarily a bad idea even if we go with Variant
> 1.
> 

I personally prefer variant 2 over variant 1. It doesn't seem necessary
for the client to deal with the guest_output_id, especially since the
guest_output_id can be deduced by the server from the
channel_id/monitor_id pair. 

If the client does send all three values, we need the server to be able
to deal with a buggy client that sends a guest_output_id that doesn't
match with what the server thinks should be the guest_output_id for the
channel_id/monitor_id pair. So I think that exposing the
guest_output_id to the client complicates things somewhat.

On the other hand, this means that the server needs to mess with the
agent message rather than sending it on verbatim to the agent. That's
not ideal either, but overall I think it's not bad enough to support
variant 1 over variant 2.


> 
> I think the decision between the variants boils down to whether we
> want
> all the monitors_config data go to the client (and therefore adding
> anything means a protocol change), or keep the client <-> server
> protocol to the bare minimum and store the rest on the server. I
> would
> say, though, that majority of the data we would ever want to add to
> the
> monitors_config message would be becase we need it on the client (the
> amount of information Christophe wanted to add to the messages for
> the
> client seems to be in accord with that). So in that case the map
> would
> just be a mostly useless complication of the code?

I'm not quite sure I understand what you mean by this part. Can you
give a concrete example of what data we might want to add?

Jonathon

> 
> Cheers,
> Lukas
> 
> 
> [1] https://lists.freedesktop.org/archives/spice-devel/2018-June/0442
> 30.html
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel