[Spice-devel] [RFC PATCH v2 00/20] Monitor ID rework

[Lukáš Hrázký]

On Mon, 2018-08-27 at 16:30 +0200, Gerd Hoffmann wrote:
> On Mon, Aug 27, 2018 at 03:34:54PM +0200, Lukáš Hrázký wrote:
> > On Mon, 2018-08-27 at 14:27 +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > 1. The IDs sent from the client in VDAgentMonitorsConfig and
> > > > MousePosition messages are equal to either `channel_id + monitor_id` or
> > > > `channel_id ? channel_id : monitor_id`. This is under the assumption
> > > > that there is either only one display_channel or more display channels
> > > > each with only a single monitor. Under this assumption the
> > > > aforementioned expressions are equivalent. It is not a reliable way to
> > > > identify monitors though.
> > > 
> > > Can we define this to be "channel_id + monitor_id", then use sparse
> > > channel ids?  qxl has room for up to 16 monitors in the protocol
> > > messages.  channel_id is 8 bit.  So we could support up to 16 display
> > > devices with up to 16 outputs each if we enumerate the display channels
> > > 0, 16, 32 instead of 0, 1, 2.
> > 
> > While I suppose technically possible, a couple issues:
> > 
> > 1. It is AFAICS not backwards compatible anyway (you don't change the
> > protocol itself, but you change the content):
> > 
> > channel_id | monitor_id | old display_id | your display_id
> > ----------------------------------------------------------
> >          0 |          0 |              0 |               0
> >          1 |          0 |              1 |              16
> >          2 |          0 |              2 |              32
> > ...
> 
> No.  The idea is to change the channel id, not the math to calculate
> the display id.
> 
> current scheme:
> 
>    channel  monitor  display
>      0         0       0
>      0         1       1    <== Oops, two displays
>      1         0       1    <== with the same ID.
>      1         1       2
> 
> suggested scheme:
> 
>    channel  monitor  display
>      0         0       0
>      0         1       1
>     16         0      16
>     16         1      17
> 
> Maybe it is better to make the size of the gaps depend on the number of
> monitors a channel has, i.e. if channel 0 is a qxl device with up to
> four outputs configured we skip channels 1+2+3 and the next display
> device gets channel 4.  We don't have holes in the display numbering
> then.

Ah, right.

This would work for the channel_id + monitor_id formula, but not for
the channel_id ? channel_id : monitor_id one. AFAIK channel_id ?
channel_id : monitor_id is used only in spice-gtk and channel_id +
monitor_id is used in virt-viewer and spicy.

However, this will still cause issues with the monitors_config
messages. The individual configs for the heads (see 13/20,
channel_main.c at the code that's being replaced) are stored in an
array of the size MAX_DISPLAY. And the channel_id + monitor_id is the
index into the array. That could also be fixed, but the array is sent
that way over the network, with the ID still being the index. So that
would become quite ugly.

And we still need to fix the other half of the problem, which is the
guest-side display ID, and after we do it, AFAICS we still need a
compatibility switch between the old way the channel_ids were assigned
and your new way, as they would require a different approach of
calculating it?

All in all, unless I'm still missing something, I think it would be too
ugly to be worth it...

Cheers,
Lukas

> > 2. I don't want to do this, while it's better than simple channel_id +
> > monitor_id, it's still unnecessary to encode the numbers this way, two
> > separate fields are much cleaner.
> 
> Sure, two fields are cleaner.
> 
> > (and with it not saving us
> > compatibility trouble...)
> 
> Well, I hope it *does* save us the compatibility trouble, that is the
> whole point.  If if doesn't work out because there assumtions in the
> code base about the way channels are numbered, then scratch the idea.
> 
> > (on a side note, I'm still assimilating the information you shared in
> > the other emails and thinking about ways to move forward, that's why
> > I'm slow on the replies...)
> 
> Sure.  Take your time and feel free to ask if you need more information.
> 
> cheers,
>   Gerd
>