[Spice-devel] monitors_config rework: proposed protocol changes

[Lukáš Hrázký]

On Tue, 2018-07-03 at 06:56 -0400, Frediano Ziglio 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;
> 
> OT: this can be done in a preparation patch, even now, is
> just a rename.
> 
> >     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;
> 
> OT: channel_id is 8 bit.
> 
> > +   uint32_t monitor_id;
> > +   int32_t guest_output_id;
> 
> OT: fields can be added at the end.

There is no advantage however to adding them at the end. There will
need to be a new struct for this message anyway. So I took the
opportunity and put them at the beginning, since I consider it a
convention to put IDs first in structs. Might just be me being used to
that though.

> >     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.
> > 
> 
> I think making clear is for the guest and not confusing with the already
> confusing "display_id" is good.
> 
> > 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 would vote for this anyway.

Ok, noted.

> > 
> > 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?
> > 
> 
> Sorry, not clear. In the last sentence the "map" you mean in Variant 2
> the (channel_id, monitor_id) -> guest_output_id ??

Yes, the "map" is the one from Variant 2 holding the guest_output_id
and potentially other data belonging to the monitors config that does
not need to be sent to the client.

Not sure if anything else is unclear in the paragraph. It's discussing
the main difference between the two variants...

> > Cheers,
> > Lukas
> > 
> > 
> > [1] https://lists.freedesktop.org/archives/spice-devel/2018-June/044230.html
> 
> I'm slightly in favour of Variant 2 (not changing SpiceMsgDisplayHead beside
> the field rename).
> 
> 
> LONG OT: protocol extensions
> 
> How to extend the protocol is not really well documented.
> Usually you add a capability which means you support some new messages and
> you add the message.
> 
> On the implementation the demarshaller (receiver side) is happy if the
> message received is longer (beside when terminating with a flexible array).
> This will allow for instance the client to send to server a message with
> additional fields at the end of the message (old servers will just ignore
> additional fields).
> On compatibility demarshalling data, messages can be extended to fill
> additional missing fields using the @virtual attribute. For instance
> you could extend
> 
>   message Demo {
>       uint32 field1; 
>   };
> 
> to
> 
>   message Demo {
>       uint32 field1;
>       uint32 field2 @virtual(0);
>   };
>   message Demo2 {
>       uint32 field1;
>       uint32 field2;
>   } @ctype(SpiceMsgDemo);
> 
> or even
> 
>   message Demo {
>       uint32 field1;
>       uint32 field2 @virtual(0);
>       uint8 field2_present @virtual(0);
>   };
>   message Demo2 {
>       uint32 field1;
>       uint32 field2;
>       uint8 field2_present @virtual(1);
>   } @ctype(SpiceMsgDemo);
> 
> You can also use combination of flags and switches, but not for
> structures contained into arrays (not supported).
> 
> END LONG OT!

Thanks for explaining. But yeah, not usable here, since the structs
being extended are in an array.

Cheers,
Lukas

> Frediano