On Fri, 2018-06-22 at 11:35 +0200, Christophe de Dinechin wrote:
> > On 20 Jun 2018, at 13:56, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> >
> > On Wed, 2018-06-20 at 04:34 +0200, Christophe de Dinechin wrote:
> > > > On 19 Jun 2018, at 17:30, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, 2018-06-19 at 15:41 +0200, Christophe de Dinechin wrote:
> > > > > > On 5 Jun 2018, at 17:30, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > To keep compatibility with old endpoints (any of client, server,
> > > > > > vd_agent), we need to copy the message to add the output_id field.
> > > > > >
> > > > > > The output_id is the guest-side id of the xrandr output (to be precise,
> > > > > > it is the index in the list of xrandr outputs) that is set in the
> > > > > > monitors config messages by the streaming agent. It is later used in the
> > > > > > guest by vd_agent for mouse input and possibly monitors_config
> > > > > > (enabling/disabling monitors and setting the resolution/position of
> > > > > > monitors).
> > > > > >
> > > > > > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > > > > > ---
> > > > > > spice/vd_agent.h | 27 +++++++++++++++++++++++++++
> > > > > > 1 file changed, 27 insertions(+)
> > > > > >
> > > > > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > > > > index dda7044..05c9c40 100644
> > > > > > --- a/spice/vd_agent.h
> > > > > > +++ b/spice/vd_agent.h
> > > > > > @@ -154,6 +154,33 @@ typedef struct SPICE_ATTR_PACKED VDAgentMonitorsConfig {
> > > > > > VDAgentMonConfig monitors[0];
> > > > > > } VDAgentMonitorsConfig;
> > > > > >
> > > > > > +typedef struct SPICE_ATTR_PACKED VDAgentMonConfigV2 {
> > > > >
> > > > > So you are defining a whole new structure just to add one field, right?
> > > >
> > > > Right.
> > > >
> > > > > Would it be better from a compatibility point of view to add a flag indicating, for example, that the “depth” field is replaced with:
> > > > >
> > > > > uint16_t depth;
> > > > > uint16_t output_id;
> > > > >
> > > > > and leave the output_id to 0 unless we have the capability you added? (I ordered the fields assuming little endian).
> > > >
> > > > I don't know. I've also just noticed today that the depth field
> > > > actually seems to be unused, besides being set in the client. But the
> > > > fact that it is being set complicates reuse. And your solution would
> > > > have a problem on big endian architectures?
> > > >
> > > > I'm not much fond of reusing current fields if it involves yet another
> > > > hack. I also was already planning to add the channel_id and monitor_id
> > > > to the message to fix the ID problem in general. So that probably
> > > > warrants a new message.
> > >
> > > I’m not too fond of it either. I’d rather take the opportunity to add more fields, trying to think ahead. See below and response to Jonathon.
> > >
> > > >
> > > > There is the "opposite" message (SpiceMsgDisplayMonitorsConfig) to
> > > > consider too, the one sent from the server to the client (patch 03/16,
> > > > needs to be copied too). That one has the "uint32_t flags" field that
> > > > seems completely unused. That may be a better candidate, since reusing
> > > > the field should be clean and there are no other IDs to add.
> > > >
> > > > > Alternatively, if you want to add a new field, you might want to leave some room for future extensions.
> > > >
> > > > Yeah, maybe... and how much? This triggered me thinking having a
> > > > protocol that is extensible (e.g. protocol buffers, cap'n proto) would
> > > > be much better (topic for another discussion though :D)
> > >
> > > For now, I would look at https://en.wikipedia.org/wiki/Extended_Display_Identification_Data and see the various types of information that display manufacturers consider useful. I’ve written a list in response to Jonathon, with various use cases. We could just reserve the fields for now, and maybe add a flags field stating which ones are populated, that would be 0 initially.
> >
> > I'll keep this topic in the other part of the thread.
> >
> > > >
> > > > > > + /* The output_id is the guest-side id of the xrandr output (to be precise,
> > > > > > + * it is the index in the list of xrandr outputs) that is set in the
> > > > > > + * monitors config messages by the streaming agent. It is later used in the
> > > > > > + * guest by vd_agent for mouse input and possibly monitors_config
> > > > > > + * (enabling/disabling monitors and setting the resolution/position of
> > > > > > + * monitors).
> > > > >
> > > > > Is it useful to explicitly link that to the xrandr output? As far as the protocol or client are concerned, it’s just some opaque output ID.
> > > >
> > > > Not necessarily, but the link is only in the comment. That can be
> > > > changed anytime without compatibility reprecussions :)
> > > >
> > > > > On the other hand, I would add the producers and consumers of this data, and then you could list xrandr as an example (as opposed to a definition).
> > > >
> > > > I can do that. As I said, that's only said in comments and easily
> > > > changeable. I didn't want to draw some theoretical rules and boundaries
> > > > around the one number that are not going to matter much once someone
> > > > needs to change the content :)
> > > >
> > > > Note there is the semantic of "0" representing an unset output_id,
> > > > which might be limiting to the possible usage…
> > > >
> > > > I see you also already noted the "-1" I'm doing with this for the mouse
> > > > motion event... :D Not that great, I agree.
> > >
> > > Any issue with using -1 for unset so that the ID is the same for the mouse?
> >
> > Not much, except for the inconveniece of initializing to -1 and
> > checking explicitly for -1 and having to use a signed int. Which brings
> > us to the opacity of the ID you mentioned. Which I think is not very
> > feasible anyway, though. But I'll keep that in mind and see what I can
> > come up with.
> >
> > > Still not sure why in the new case, the mouse ID cannot start at 1 either.
> >
> > It's because the mouse motion event is not new, it's the old code that
> > expects 0-based sequence of IDs.
>
> Well, the server just copies display_id. The vd_agent uses it to index an array, but I get a definite feeling that you don’t want to have “display_id” be distinct from your “output_id” if you want to get that right on that side.
>
> So intuitively, I think that adjusting the vd_agent to use an output ID is better than hacking a magic -1 somewhere.
But that would mean changing the protocol (if I understand you
correctly), which in this case I think is unnecessary. Once you pass
the number to the mouse motion event, it has the same meaning.
> Or as discussed, making the output_id 0-based (and still use that for inputs in the vd_agent).
That is my current plan.
> Makes me think…
>
> If it’s called “display_id” for inputs in the vd_agent, don’t you want to call it “display_id” in monitor config as well? Or are these two different things? If so, how are they different?
The nomenclature is really quite inconsistent across the various
components. I suppose they end up being mostly the same in the end, but
I don't want to use "display_id" as that seems just too generic and
confusing with all the inconsistency. I was thinking of naming it
"guest_output_id", perhaps "guest_display_id" then...
> Thanks
> Christophe
>
> >
> > > >
> > > > > > + */
> > > > > > + uint32_t output_id;
> > > > >
> > > > > If it’s opaque, 32-bit might be too small, e.g. to pass a Windows handle. Or is it part of the definition that these are necessarily small consecutive IDs and that the agent has to keep a mapping if they want to associate some pointer with it?
> > > >
> > > > I think you have the same information I do here :) The xrandr output
> > > > IDs are the only thing we have atm. I never finished the implementation
> > > > of my crystal ball so I can't predict the future yet :)
> > >
> > > I’d personally favor a zero-based index, because it’s easier to do sanity checks, and that’s how most of the rest of SPICE operates. In that case, 32-bit is more than enough.
> > >
> > >
> > > >
> > > > If the protocol was extensible, it wouldn't be much of an issue. The
> > > > way it is, I used uint32_t thinking it should be enough for any needs
> > > > we have in the future. So the windows handles are what, uint64_t? From
> > > > my very brief googling it seems there's some confusion around it, but
> > > > that handles seem to in general fit in 32 bytes...
> > > >
> > > > > (Yes, I know it contradicts my compatible proposal above, just trying to confuse you, or more realistically, to understand what you have in mind ;-)
> > > >
> > > > It's ok, this is the sort of discussion I wanted to have at this stage.
> > > >
> > > > Thanks,
> > > > Lukas
> > > >
> > > > > > + /*
> > > > > > + * Note a width and height of 0 can be used to indicate a disabled
> > > > > > + * monitor, this may only be used with agents with the
> > > > > > + * VD_AGENT_CAP_SPARSE_MONITORS_CONFIG capability.
> > > > > > + */
> > > > > > + uint32_t height;
> > > > > > + uint32_t width;
> > > > > > + uint32_t depth;
> > > > > > + int32_t x;
> > > > > > + int32_t y;
> > > > > > +} VDAgentMonConfigV2;
> > > > > > +
> > > > > > +typedef struct SPICE_ATTR_PACKED VDAgentMonitorsConfigV2 {
> > > > > > + uint32_t num_of_monitors;
> > > > > > + uint32_t flags;
> > > > > > + VDAgentMonConfigV2 monitors[0];
> > > > > > +} VDAgentMonitorsConfigV2;
> > > > > > +
> > > > > > enum {
> > > > > > VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_WALLPAPER = (1 << 0),
> > > > > > VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_FONT_SMOOTH = (1 << 1),
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > 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
> > >
> > >
> >
> > _______________________________________________
> > 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