On Mon, 2018-07-09 at 16:53 -0500, Jonathon Jongsma wrote:
> 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.
To be precise, if we sent the guest_output_id to the client, there
would be little reason to also store it on the server and we wouldn't
therefore do such a check... What comes from the client must be
correct, otherwise it won't work, as always :)
> 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.
Indeed, it's some extra work. Although an argument could be made that
client <-> server and server <-> vd_agent are two different protocols
and therefore shouldn't share messages. But that's mostly off-topic...
> > 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?
For example everything that c3d suggested we add to the messages for
future use:
- output name
- DPI
- refresh rate
- gamma
- ...
Can't predict the future, but as I said, if we ever want to extend the
monitors_config messages, it quite probably will be because we want to
get some information to the client.
So to reiterate, if we add the following map structure to the server:
[channel_id, monitor_id] -> struct mc_data { guest_output_id }
In hope it will save us from having to change the protocol in the
future by adding more fields to the mc_data, chances are it won't help
us (because we want to send it to the client anyway) and the
guest_output_id will stay the sole field in the struct.
And the server code will be more complex because of this single field.
I hope it's understandable and you get my point :)
Anyway, (unless you reconsider) it's you and Frediano in favor of
Variant 2 and I'm sort of impartial myself, so I think we go for it.
Cheers,
Lukas
> Jonathon
>
> >
> > Cheers,
> > Lukas
> >
> >
> > [1] https://lists.freedesktop.org/archives/spice-devel/2018-June/0442
> > 30.html
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel