[Spice-devel] [RFC/POC PATCH spice-protocol 02/16] Create a version 2 of the VDAgentMonitorsConfig message
Christophe de Dinechin
dinechin at redhat.com
Fri Jun 22 13:18:38 UTC 2018
> On 22 Jun 2018, at 14:25, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> On Fri, 2018-06-22 at 11:35 +0200, Christophe de Dinechin wrote:
>>> On 20 Jun 2018, at 13:56, Lukáš Hrázký <lhrazky at redhat.com> 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 at redhat.com> 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 at redhat.com> 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 at redhat.com>
>>>>>>> ---
>>>>>>> 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),
Why? I did not see any part of the code other than the vd_agent that makes assumptions about the range of values, and then the vd_agent only cares about it being less than the number of displays. Is there another spot I missed that depends on that? Or maybe some protocol spec I missed?
It’s moot if you go with your plan to zero-base them anyway.
> 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.
Good.
>
>> 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…
I agree about inconsistency. But if your output_id is the same thing as the vd_agent’s display_id, and if there is no other use of the symbol “display_id” anywhere in the code (I did not find any), then aren't *you* introducing an inconsistency by calling it “output_id” instead of “display_id” and using a different range of values?
(Changing the name to “display_id" would also address my earlier comment where I was confused that we called a function that had “display_id” in the name and argument names, and used it with “output_id”)
Thanks
Christophe
>
>> 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 at lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Spice-devel mailing list
>>>>> Spice-devel at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>
>>>>
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list