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