On Thu, 2018-06-14 at 21:12 +0200, Marc-André Lureau wrote: > Hi > > On Tue, Jun 5, 2018 at 5:30 PM, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > > Hi everyone, > > > > following is my attempt at solving the ID issues with monitors_config > > and streaming. The concept is as follows: > > > > Before introducing a new solution, could you describe the "issues with > monitors_config and streaming"? I am not following closely enough the > mailing list probably, so a recap would be welcome. I'd like to > understand the shortcomings of the current messages, and see if we are > on the same page about it. Thanks! Right, sorry about that! The issue is when having a plugin a for the streaming agent that is using a HW-accelerated encoder. The typical case is that you have a QXL monitor in the guest which shows the boot and then you have a physical HW device (either directly assigned to the VM or a vGPU) which is configured in the X server. Once the X server starts, the QXL monitor goes blank and you get a second monitor on the client with the streamed content from the streaming agent plugin. The problem is the code and the protocol assumes (amongst other assumptions) that all monitors are configured in X. The monitors on the client are put into an array and the index is used as the ID throughout SPICE. So for the case above, the QXL monitor is ID 0 and the streamed monitor is ID 1. The more pressing issue with this is that a mouse motion event is sent with a display_id 1 from the client, but when it reaches the guest, the correct monitor is ID 0, as it is the only one. The other thing this breaks is monitors_config messages that enable/disable displays and change the resolution. Besides the same "shift" happening here as well, another problem is the information that the monitors belong to different devices (or channels) is erased when they're all put into the single array (under the assumption there is either one channel with multiple monitors or multiple channels with one monitor each). Hope this explains it well enough. It's all very complex and goes over multiple interfaces (the network protocols as well as the spice-gtk API), so the more brilliant ideas you'll have, the more welcome they will be :) Cheers, Lukas > > 1. The streaming-agent sends a new StreamInfo message when it starts > > streaming. The message contains the output_id of the streamed monitor. > > The actual value is the index in the list of xrandr outputs. Basically, > > the number should uniquely identify a monitor in the guest context under > > X. > > > > 2. The server copies the number into the SpiceMsgDisplayMonitorsConfig > > message and sends it to the client as part of the monitors_config > > exchange. > > > > 3. The client stores the output_id in it's list of monitor_configs. Here > > I had to make significant changes, because the monitors_config code in > > spice-gtk is very loosely written and also exposes quite a few > > unnecessary details to the client app. The client sends the output_id > > back to the server as part of the monitors_config messages > > (VDAgentMonitorsConfigV2) and also uses it for the mouse motion event, > > which also contains a display ID interpreted by the vd_agent. In the > > end, the API/ABI towards the client app should remain unchanged. > > > > 4. The server passes the output_id in above-mentioned messages to the > > vd_agent. The output_id is meaningless in the server context. > > (Currently, it doesn't pass the monitors_config messages if there is a > > QXL device that supports it, though. Needs more work.) > > > > 5. vd_agent: > > a) For mouse input, the output_id was passed in the original message, > > so no change needed here, it works. > > > > b) If the server sends monitors_config to the guest, the vdagent will > > prefer to use monitors_configs with the output_ids set, if such are > > present. In that case, it ignores monitors_configs with the output_id > > unset. If no output_ids are present, it should behave as it used to. > > > > A couple of things to note: > > - While I did copy the VDAgentMonitorsConfig to a different message for > > backwards compatibility, I didn't do the same for > > SpiceMsgDisplayMonitorsConfig, it remains to be done. > > > > - I didn't introduce any capabilities to handle the compatibility, also > > remains to be done. Hopefully it is also clear it will be quite a > > non-trivial job to do that :( Ain't gonna make the code prettier either. > > > > For your convenience, you can also pull the branches below: > > https://gitlab.freedesktop.org/lukash/spice-protocol/tree/monitors-config-poc > > https://gitlab.freedesktop.org/lukash/spice-common/tree/monitors-config-poc > > https://gitlab.com/lhrazky/spice-streaming-agent/tree/monitors-config-poc > > https://gitlab.freedesktop.org/lukash/spice/tree/monitors-config-poc > > https://gitlab.freedesktop.org/lukash/spice-gtk/tree/monitors-config-poc > > https://gitlab.freedesktop.org/lukash/vd_agent/tree/monitors-config-poc > > > > All in all, it's big, complex and invasive. Note I did review the > > emergency instructional video [1] and am therefore ready for any > > bombardment you'll be sending my way :D (Don't hesitate to contact me > > with questions either) > > > > Last minute note: I've noticed some of the patches are missing > > Signed-off-by line, since they are not for merging, should not be an > > issue... > > > > > > Lukáš Hrázký (16): > > spice-protocol > > Add the StreamInfo message > > Create a version 2 of the VDAgentMonitorsConfig message > > spice-common > > add output_id to SpiceMsgDisplayMonitorsConfig > > spice-streaming-agent > > Send a StreamInfo message when starting to stream > > spice-server > > Handle the StreamInfo message from the streaming agent > > Use VDAgentMonitorsConfigV2 that contains the output_id field > > spice-gtk > > Rework the handling of monitors_config > > Remove the n_display_channels check when sending monitors_config > > Use an 'enabled' flag instead of status enum in monitors_config > > Use VDAgentMonitorsConfigV2 as the message for monitors_config > > Add output_id to the monitors_config > > Use the new output_id as display ID for the mouse motion event > > vd_agent > > vdagent: Log the diddly doo X11 error > > Improve/add some logging messages > > Use VDAgentMonitorsConfigV2 instead of VDAgentMonitorsConfig > > vdagent: Use output_id from VDAgentMonitorsConfigV2 > > > > [1] https://www.youtube.com/watch?v=IKqXu-5jw60 > > > > -- > > 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