> 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?
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).
Alternatively, if you want to add a new field, you might want to leave some room for future extensions.
> + /* 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.
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).
> + */
> + 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?
(Yes, I know it contradicts my compatible proposal above, just trying to confuse you, or more realistically, to understand what you have in mind ;-)
> + /*
> + * 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