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

[Lukáš Hrázký]

On Wed, 2018-06-20 at 15:57 -0500, Jonathon Jongsma wrote:
> On Wed, 2018-06-20 at 13:33 +0200, Lukáš Hrázký wrote:
> > On Tue, 2018-06-19 at 16:51 -0500, Jonathon Jongsma wrote:
> > > On Tue, 2018-06-19 at 15:15 +0200, Lukáš Hrázký wrote:
> > > > On Tue, 2018-06-19 at 08:11 -0400, Frediano Ziglio wrote:
> > > > ...
> > > > 
> > > > Actually, I've realized this is an interesting point by Marc-
> > > > André,
> > > > the
> > > > client doesn't neccessarily need to care for the output_id I've
> > > > added
> > > > to the protocol.
> > > > 
> > > > There was a suggestion by Uri to keep the output_id in a map on
> > > > the
> > > > server and not send it to the client and back. That would mean no
> > > > protocol change. However, it is not possible because of the ID
> > > > discrepancy. There is no reliable key to use in the server on
> > > > both
> > > > the
> > > > sending and the receiving side.
> > > > 
> > > > It might be an idea to just fix the IDs in the protocol and still
> > > > use
> > > > the map instead of sending the output_id through the client.
> > > 
> > > 
> > > So, I think we mostly agree that the protocol design in this area
> > > is
> > > not ideal. On the other hand, if we can make it work without any
> > > protocol changes, it makes for a far less complicated transition
> > > (no
> > > capabilities flags, no handling of old and new protocol messages,
> > > etc).
> > > So if we can make things work without changes to the protocol, that
> > > would be nice.  
> > > 
> > > Your current approach is that the guest sends a message to the
> > > server
> > > to tell the server the output_id associated with a given stream.
> > > Then
> > > this output_id is passed on to the client so that the client can
> > > pass
> > > it back to the server, who then passes it on to the guest.
> > > 
> > > What if, instead, we switch the responsibilities around? 
> > > 
> > > What if we made the server tell the guest which ID it associates
> > > with
> > > the stream (instead of the guest telling the server). The ID that
> > > the
> > > server assigns to the stream would just be the channel ID + monitor
> > > ID
> > > to match the client's assumptions. Then the guest agent would
> > > maintain
> > > a map from server ID to xrandr ID. That would perhaps allow us to
> > > avoid
> > > changing the protocol messages. Of course, maybe you already
> > > considered
> > > this approach and there's a reason that it won't work that I'm not
> > > considering.
> > 
> > The issue with this is that in the guest there are two agents. The
> > streaming agent and the vd_agent. So the server can tell the
> > streaming
> > agent which ID it assigned to the stream, and the streaming agent
> > would
> > maintain that map, but it's in the vd_agent where you'd need look up
> > the xrandr ID in the map. So this doesn't really work.
> 
> Yes, that's true. It does make it significantly more complicated,
> doesn't it?
> 
> > 
> > It also spreads the channel_id + monitor_id hack into the server,
> > digging us a deeper hole :) I believe we can't get rid of that
> > without
> > protocol changes (and if Frediano's story is correct, it was
> > introduced
> > to avoid protocol changes too).
> 
> Yeah, I think that you're right that we can't move past the
> channel_id+monitor_id assumption without protocol changes. So if we do
> want to get rid of that, I agree that we probably need to change the
> protocol. But in an ideal world, if we change the protocol it would be
> nice to have something that works for both streaming channels and non-
> streaming display channels. It feels like only a partial solution to
> have an explicit id in one case (streaming) but use '0' for all
> displays in the non-streaming case (and still rely on the
> channel_id+monitor_id assumption when we're not streaming). As long as
> we're fixing the protocol, I would love to find a solution that would
> work for both cases.

I'm also planning, as Frediano suggested, to add the channel_id and
monitor_id to the VDAgentMonitorsConfig message (the client -> server
one). That will mean we have the same set of IDs on both sides and
therefore fix this issue.

The output_id is still kind of special, as it originates in the guest
and is only relevant in the guest. Perhaps it should be called
"guest_output_id". So we only have it when we are streaming from the
guest.

There is still the issue with regular QXL monitors, for which there is
no way (AFAICS) to get the guest output_id. You still need it for mouse
motion events and for monitors config if QXL hotplug is missing. The
current implementation relies on having the same array of monitors on
both sides (and therefore, for the zero-based sequence IDs to match).
The output_id is still only a partial fix, in the sense "if we have
output_id, use it, if not, fall back to matching the sequence IDs".

I wanted to get a reliable mapping of the monitors in the beginning,
but couldn't find a way...