[Spice-devel] monitors_config rework: proposed protocol changes
Jonathon Jongsma
jjongsma at redhat.com
Tue Jul 10 17:00:39 UTC 2018
On Tue, 2018-07-10 at 17:10 +0200, Lukáš Hrázký wrote:
> On Tue, 2018-07-10 at 09:20 -0500, Jonathon Jongsma wrote:
> > On Tue, 2018-07-10 at 10:47 +0200, Lukáš Hrázký wrote:
> > > 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 :)
> >
> > OK, but there's still the fact that we're sending completely
> > unnecessary duplicate information in the message. That seems very
> > inelegant at a protocol level.
>
> To be precise again :), it is not a "completely unnecessary duplicate
> information", if it was, we wouldn't need to store it anywhere... :D
Perhaps it doesn't make much sense to continue this discussion since
we've already basically agreed on the approach to take, but I feel the
need to try to clarify what I mean ;)
You're right of course. But I'm thinking from the point of view of the
client. From the client's point of view, the (channel_id,monitor_id)
pair is essentially an immutable property that can be used to identify
a particular display. On the other hand, the guest_output_id is a piece
of metadata associated with that display that can change based on the
guest's configuration. But the client does not actually *do* anything
with that data. It doesn't affect how the client treats the display, or
how it behaves. It's serves absolutely no purpose for the client. The
only thing the client does with that information is pass it back to the
server (along with the channel_id/monitor_id) so that the server can
know how to convert the channel_id/monitor_id to a guest id.
I would rather not add additional fields to the protocol unless they
actually serve a purpose to the client. Sending the guest_output_id
value to the client feels like we're leaking implementation details
into the protocol. And we have a perfectly good alternative: keep the
map on the server.
Jonathon
>
> > In addition, including the
> > guest_output_id in the protocol message requires us to introduce
> > the
> > concept of an "invalid" guest output id (0 in your patch). That
> > also
> > strikes me as a bit inelegant.
>
> Not as much invalid as "not present". That in general makes it an
> optional field which is a common enough concept that protocols
> usually
> have mechanisms for it. There's just not one in SPICE, so we do it
> the
> inelegant way :)
>
> > So it just "feels" better to me to keep
> > this concept encapsulated within the server.
> >
> > >
> > > > 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 :)
> >
> > Got it. Two points about this:
> >
> > - I'm not convinced that there's really a need to send any of this
> > information to the client. Although as you rightly note, it's
> > difficult
> > to predict the future.
>
> Christophe had features in mind that would need the information...
>
> > - my preference of variant 2 over variant 1 is not necessarily
> > predicated on the fact that it saves us from having to add more
> > fields
> > to this protocol message (although that's a nice benefit in this
> > instance). The reason I prefer it has more to do with the fact that
> > it
> > feels more elegant at a protocol level.
>
> I mostly agree on this one.
>
> > > 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.
> > >
> >
> > OK.
> >
> > Cheers,
> > Jonathon
More information about the Spice-devel
mailing list