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. 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. 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.
- 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.
>
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel