> 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.
>
>>> + /* 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?
Still not sure why in the new case, the mouse ID cannot start at 1 either.
>
>>> + */
>>> + 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