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

Fri Jun 15 11:01:36 UTC 2018

On Fri, 2018-06-15 at 12:24 +0200, Marc-André Lureau 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.

Well... for one, it does not entirely replace it. From a user
experience PoV, yes, it makes sense to "replace" one monitor for the
other. We've even considered switching the content of the display
channel from QXL to the streamed one. But in the system, they are
different monitors on different channels, so it's a bad idea to give
them the same ID. And the ID we're talking about is actually (for the
monitors config messages):

server -> client: a pair (channel_id, monitor_id)

client -> server: channel_id + monitor_id, converted to an array index
under the assumption I mentioned

So we can't really make it the same even if we wanted.

> > 
> > 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.

I respectfully disagree. Doing the channel_id + monitor_id I mentioned
above is asking for trouble, which is exactly what we have now.

> Either you do multiple monitor over a single channel, or you use multiple
> channels, each with one monitor.

That is a limitation you can introduce, perhaps there was a reason for
it. But the code is taking shortcuts and borderline hacks because of
this, while doing it right wouldn't be much harder. I'm not looking to
blame or judge anyone, I don't know how this came into place. But what
it is now is (IMO) a bad design and I feel the need to say it after
working on it for quite some time ;)

> > 
> > 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.

I don't see how we can do that... And the inner details are already
quite exposed, unfortunately :(

As I said, ideas welcome.

> > > > 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
> > > 
> > > 
> > > 
> 
> 
> 