> On 15 Jun 2018, at 12:24, Marc-André Lureau <marcandre.lureau@xxxxxxxxx> wrote: > > 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. When we have multiple monitors, how would we decide which one “maps” to QXL? Thanks Christophe > >> >> 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel