Hi,
On 01/16/2013 03:07 PM, Marc-André Lureau wrote:
----- Mensaje original -----
Both the spice-widget code, as well as remote-viewer expect that
monitors[i]->id == i, but if ie only the qxl-0 and qxl-2 outputs are
enabled this is not true.
I've chosen to fix this by making this assumption true. This does
mean
Your explanation is misleading, we don't know what monitors[] you are talking about. There is no such "expectation" in the code, ie nothing bad will happen:
Please, please stop being so damn stubborn, and listen to what other
people have to say once in a while.
You already made the same point at the beginning of this thread, and
I already pointed out the 2 places where the monitors property of
the display channel is actually used, and how both of them expect
that monitors[i]->id == i, to quote from that mail:
"
spice-widget.c: update_monitor_area()
c = &g_array_index(monitors, SpiceDisplayMonitorConfig, d->monitor_id);
virt-viewer-session-spice.c: virt_viewer_session_spice_display_monitors():
display = g_ptr_array_index(displays, monitor->id);
"
As for not knowing what monitors I'm talking about:
1) The titel of the patch is "channel-display: Make monitors array contain monitors in id order"
So how about the monitors property of the display channel ?
2) This is also very clear from the code changes itself.
>> that the monitors array can now contain 0x0 sized (iow disabled)
>> monitors,
>> remote-viewer already checks for this.
>
> Yes, because it builds its own array.
Not true, it gets the monitors property directly from the display channel:
virt-viewer-session-spice.c: virt_viewer_session_spice_display_monitors():
g_object_get(channel,
"monitors", &monitors,
"monitors-max", &monitors_max,
NULL);
And then it creates max_monitors VirtViewerDisplaySpice
objects, and then walks the monitors array enabling
any VirtViewerDisplaySpice where width != 0 && height != 0:
for (i = 0; i < monitors->len; i++) {
SpiceDisplayMonitorConfig *monitor = &g_array_index(monitors, SpiceDisp
display = g_ptr_array_index(displays, monitor->id);
g_return_if_fail(display != NULL);
if (monitor->width == 0 || monitor->width == 0)
continue;
virt_viewer_display_set_enabled(VIRT_VIEWER_DISPLAY(display), TRUE);
virt_viewer_display_set_desktop_size(VIRT_VIEWER_DISPLAY(display),
monitor->width, monitor->height);
}
See how it is addressing the monitors property of the display-channel
by id !! let me single out the line where this happens for you (again):
display = g_ptr_array_index(displays, monitor->id);
>> Thus this patch:
>> 1) Makes the monitors[i]->id == i assumption be true
>
> No need for changing that.
/me bangs head on table.
There is a need to change that because the monitors property of the display
channel is a 1 on 1 copy of the monitors message on the wire, and for that
message monitors[i]->id == i is simply *not* true, yet it is assumed in
2 places in the code, as I've pointed out *twice* now!
>
>> 2) Makes spice-widget check for 0x0 sized monitors and treat them as
>> disabled
>
> Nor that.
again:
spice-widget.c: update_monitor_area()
c = &g_array_index(monitors, SpiceDisplayMonitorConfig, d->monitor_id);
See how the array is indexed by id, since it can contain only 2 monitors,
with for example id 0 and id 3, this will index it out of bounds.
If we fix this by inserting 0x0 sized monitors, then the checking for
such monitors becomes necessary!
Regards,
Hans
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel