Hi On Fri, Jun 15, 2018 at 12:16 PM, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > 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. Since the "streamed monitor" replaces the QXL monitor, wouldn't it make sense for them to share the same ID? This would be handled by the server/guest transparently. > > 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). Correct and so far was a fine solution. Either you do multiple monitor over a single channel, or you use multiple channels, each with one monitor. > > 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 :) If we can avoid modifying all the layers and exposing the inner details, I would encourage the exploration of other solution. >> > 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 >> >> >> -- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel