On Thu, Aug 16, 2018 at 6:26 PM Lukáš Hrázký <lhrazky@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel