[Spice-devel] [RFC PATCH v2 00/20] Monitor ID rework

[Marc-André Lureau]

On Thu, Aug 16, 2018 at 6:26 PM Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> Hello list,
>
> this is the reworked second version of the Monitor ID series. The goal
> of this series is to make the identification of monitors in the
> monitors_config exchange and in the MousePosition messages more robust,
> as well as fix the case of guest-side streaming via the
> spice-streaming-agent, where the current identification mechanism is no
> longer sufficient.
>
> You can also find the patches in the following branches:
> https://gitlab.freedesktop.org/lukash/spice-protocol/tree/monitors-config-v2
> https://gitlab.freedesktop.org/lukash/spice-common/tree/monitors-config-v2
> https://gitlab.freedesktop.org/lukash/spice-streaming-agent/tree/monitors-config-v2
> https://gitlab.freedesktop.org/lukash/spice/tree/monitors-config-v2
> https://gitlab.freedesktop.org/lukash/spice-gtk/tree/monitors-config-v2
> https://gitlab.freedesktop.org/lukash/vd_agent/tree/monitors-config-v2
>
>
> There are two issues with the IDs used currently to identify guest
> monitors:
>
> 1. The IDs sent from the client in VDAgentMonitorsConfig and
> MousePosition messages are equal to either `channel_id + monitor_id` or
> `channel_id ? channel_id : monitor_id`. This is under the assumption
> that there is either only one display_channel or more display channels
> each with only a single monitor. Under this assumption the
> aforementioned expressions are equivalent. It is not a reliable way to
> identify monitors though.
>

It's not that mixed. We only support:

1 display channel, with id=0 & SPICE_DISPLAY_CAP_MONITORS_CONFIG:
display ID = monitor ID

>1 display channel: display ID = channel ID.

Whenever the server receives VD_AGENT_MONITORS_CONFIG, it can also
easily handle the config accordingly.
Similarly on the client side.

> 2. In the guest-side streaming case, the ID scheme mentioned in 1. no
> longer works, because these IDs are sent directly to the vd_agent and
> interpreted as xrandr output IDs in the guest context. We have two main
> use-cases in both of which the IDs sent to the guest do not match the
> guest-side xrandr output IDs:

> a) QXL device whose content is streamed with the mjpeg plugin. So there
> are two displays on the client, the second one a streamed mirror of the
> first. And there are two channels, each of which with one monitor, the IDs
> are 0 and 1.  However, in the guest there is only a single output, ID 0.
>
> b) The main use case, vGPU-accelerated streaming from the guest. There
> usually is a QXL device to see the boot etc., the streaming only starts
> with X. Therefore, the QXL monitor has ID 0 and the streamed monitor has
> an ID of 1. However, the QXL monitor is not used in X, only the vGPU is
> configured in X and it's first monitor under X has the ID of 0.

Sorry I can't follow as I am not familiar enough with the streaming mode.

Is there a new channel that the client will have to handle and somehow
match with a display channel id (or channel_id=0 & monitor_id) ?

>
> In summary, this patch series addresses the issues as follows:
>
> 1. It replaces the current aggregate ID sent from the client to the
> server with the pair (channel_id, monitor_id). In particular, it
> replaces the VDAgentMonitorsConfig message with
> SpiceMsgcMainMonitorsConfig and MousePosition with MousePositionV2.

As said above, I think both client and server can deal with the
situation. I don't like either to expose that detail to the client
API.

>
> 2. On the server, it translates the ID pair to the guest-side ID in
> one of the following ways:
>
> a) For the guest-side streaming case, it looks up the ID in the
> monitors_guest_output_id hash table, into which the IDs are stored from
> StreamInfo messages from the streaming agent (which runs in the guest
> context and therefore has the correct IDs).
>
> b) For the regular SPICE case, the calculation is still using the
> sub-optimal `channel_id ? channel_id : monitor_id` for lack of better
> options. This calculation is now confined in the server though and can
> be more easily replaced with something better later.
>
> 3. The Messages from the server to the vd_agent now contain the
> correct guest-side ID the vd_agent understands. (This involves reworking
> the MonitorsConfig message to contain the ID as an item in the message
> instead of relying on the array index. This is not strictly necessary
> but makes for a cleaner and more explicit code.)

I can't comment on server/agent code, I haven't looked the streaming part.

>
>
> The code is still lacking capabilities to ensure backwards
> compatibility, marked with a TODO in a couple of places and possibly
> more is needed. It is meant to work only with all components having the
> patches from this series. Once this is agreed upon, I'll add the
> capabilities in a later version.
>
> Some intrusive changes were needed on the client side, impacting also
> the spice-gtk API. I'd especially like feedback on that, I'm not sure
> it's good enough as it is.
>
> One known thing the spice-gtk changes break is the fullscreen mode in
> virt-viewer. In fullscreen mode, virt-viewer tries to configure the
> monitors as soon as it starts, while now updating the monitors only work
> after the monitors_config messages were received for them. My plan is to
> attempt to fix this in virt-viewer, meaning building an older
> virt-viewer with spice-gtk containing these changes will result in
> broken fullscreen mode.
>
> I'll also mention I had some redraw glitches and some gnome-shell
> crashes during testing, which most probably were bugs unrelated to
> SPICE. I decided to get the patches out there and investigate later, if
> you test this and get similar issues though, let me know.
>
> Cheers,
> Lukas
>
> --
> 2.18.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau