> 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.
Or as discussed, making the output_id 0-based (and still use that for inputs in the vd_agent).
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?
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