[Spice-devel] [RFC/POC PATCH 00/16] add output_id to monitors_config

Fri Jun 15 10:47:02 UTC 2018

> On 15 Jun 2018, at 12:24, Marc-André Lureau <marcandre.lureau at gmail.com> wrote:
> 
> Hi
> 
> On Fri, Jun 15, 2018 at 12:16 PM, Lukáš Hrázký <lhrazky at redhat.com> 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 at redhat.com> 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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>> 
>>> 
>>> 
> 
> 
> 
> -- 
> Marc-André Lureau
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel