Hey,
Patch looks good to me, just a couple of minor comments.
Fwiw, this was marginally easier to review by splitting the code
movement in one patch (virt_viewer_align_monitors_linear), then the
addition of virt_viewer_shift_monitors_to_origin and finally the actual
changes.
On Tue, Oct 07, 2014 at 01:30:03PM -0500, Jonathon Jongsma wrote:
> When using a custom fullscreen display configuration, it's possible to
> specify that e.g. a single screen should be fullscreen on client
> monitor #4. Since we send down absolute positions and disable alignment
> when all windows are in fullscreen, we can send configurations with a
> very large offset to the top-left corner. This could result in the guest
> trying to create a screen that was much larger than necessary. For
> example when sending a configuration of 1280x1024+4240+0, the guest
> would need to allocate a screen of size 5520x1024, which might fail if
> video memory was too low. To avoid this issue, we shift all displays
> down so that the minimum X coordinate for all screens is at x=0, and the
Nit: For me the displays are shifted up as +0+0 is the top left corner.
"We shift all displays so that..." should be good for everyone :)
> minimum y coordinate is at y=0.
> ---
> src/virt-viewer-session-spice.c | 22 +++++++---
> src/virt-viewer-session.c | 53 ++----------------------
> src/virt-viewer-util.c | 91 +++++++++++++++++++++++++++++++++++++++++
> src/virt-viewer-util.h | 6 +++
> 4 files changed, 117 insertions(+), 55 deletions(-)
>
> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c
> index 885399c..37c083e 100644
> --- a/src/virt-viewer-session-spice.c
> +++ b/src/virt-viewer-session-spice.c
> @@ -770,7 +770,7 @@ virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice *self)
> GdkScreen *screen = gdk_screen_get_default();
> SpiceMainChannel* cmain = virt_viewer_session_spice_get_main_channel(self);
> VirtViewerApp *app = NULL;
> - GdkRectangle dest;
> + GdkRectangle *displays;
> gboolean agent_connected;
> gint i;
> gsize ndisplays = 0;
> @@ -804,18 +804,30 @@ virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice *self)
>
> ndisplays = virt_viewer_app_get_n_initial_displays(app);
> g_debug("Performing full screen auto-conf, %" G_GSIZE_FORMAT " host monitors", ndisplays);
> + displays = g_new0(GdkRectangle, ndisplays);
>
> for (i = 0; i < ndisplays; i++) {
> + GdkRectangle* rect = &displays[i];
> gint j = virt_viewer_app_get_initial_monitor_for_display(app, i);
> if (j == -1)
> continue;
>
> - gdk_screen_get_monitor_geometry(screen, j, &dest);
> - g_debug("Set SPICE display %d to (%d,%d)-(%dx%d)",
> - i, dest.x, dest.y, dest.width, dest.height);
> - spice_main_set_display(cmain, i, dest.x, dest.y, dest.width, dest.height);
> + gdk_screen_get_monitor_geometry(screen, j, rect);
> + }
> +
> + virt_viewer_shift_monitors_to_origin(displays, ndisplays);
> +
> + for (i = 0; i < ndisplays; i++) {
> + GdkRectangle *rect = &displays[i];
> + if (rect->width == 0 || rect->height == 0)
> + continue;
The initial code did not skip disabled displays, any idea if this was
causing issues? Or was this handled by checking
virt_viewer_app_get_initial_monitor_for_display() for -1 ?
> +
> + spice_main_set_display(cmain, i, rect->x, rect->y, rect->width, rect->height);
> spice_main_set_display_enabled(cmain, i, TRUE);
> + g_debug("Set SPICE display %d to (%d,%d)-(%dx%d)",
> + i, rect->x, rect->y, rect->width, rect->height);
> }
> + g_free(displays);
>
> spice_main_send_monitor_config(cmain);
> self->priv->did_auto_conf = TRUE;
> diff --git a/src/virt-viewer-session.c b/src/virt-viewer-session.c
> index d9c84a6..4262eda 100644
> --- a/src/virt-viewer-session.c
> +++ b/src/virt-viewer-session.c
> @@ -339,55 +339,6 @@ virt_viewer_session_init(VirtViewerSession *session)
> session->priv = VIRT_VIEWER_SESSION_GET_PRIVATE(session);
> }
>
> -/* simple sorting of monitors. Primary sort left-to-right, secondary sort from
> - * top-to-bottom, finally by monitor id */
> -static int
> -displays_cmp(const void *p1, const void *p2, gpointer user_data)
> -{
> - guint diff;
> - GdkRectangle *displays = user_data;
> - guint i = *(guint*)p1;
> - guint j = *(guint*)p2;
> - GdkRectangle *m1 = &displays[i];
> - GdkRectangle *m2 = &displays[j];
> - diff = m1->x - m2->x;
> - if (diff == 0)
> - diff = m1->y - m2->y;
> - if (diff == 0)
> - diff = i - j;
> -
> - return diff;
> -}
> -
> -static void
> -virt_viewer_session_align_monitors_linear(GdkRectangle *displays, guint ndisplays)
> -{
> - gint i, x = 0;
> - guint *sorted_displays;
> -
> - g_return_if_fail(displays != NULL);
> -
> - if (ndisplays == 0)
> - return;
> -
> - sorted_displays = g_new0(guint, ndisplays);
> - for (i = 0; i < ndisplays; i++)
> - sorted_displays[i] = i;
> - g_qsort_with_data(sorted_displays, ndisplays, sizeof(guint), displays_cmp, displays);
> -
> - /* adjust monitor positions so that there's no gaps or overlap between
> - * monitors */
> - for (i = 0; i < ndisplays; i++) {
> - guint nth = sorted_displays[i];
> - g_assert(nth < ndisplays);
> - GdkRectangle *rect = &displays[nth];
> - rect->x = x;
> - rect->y = 0;
> - x += rect->width;
> - }
> - g_free(sorted_displays);
> -}
> -
> static void
> virt_viewer_session_on_monitor_geometry_changed(VirtViewerSession* self,
> VirtViewerDisplay* display G_GNUC_UNUSED)
> @@ -428,7 +379,9 @@ virt_viewer_session_on_monitor_geometry_changed(VirtViewerSession* self,
> }
>
> if (!all_fullscreen)
> - virt_viewer_session_align_monitors_linear(monitors, nmonitors);
> + virt_viewer_align_monitors_linear(monitors, nmonitors);
> +
> + virt_viewer_shift_monitors_to_origin(monitors, nmonitors);
>
> klass->apply_monitor_geometry(self, monitors, nmonitors);
> g_free(monitors);
> diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c
> index 655f489..401b220 100644
> --- a/src/virt-viewer-util.c
> +++ b/src/virt-viewer-util.c
> @@ -488,6 +488,97 @@ end:
> g_strfreev(v2);
> return retval;
> }
> +
> +/* simple sorting of monitors. Primary sort left-to-right, secondary sort from
> + * top-to-bottom, finally by monitor id */
> +static int
> +displays_cmp(const void *p1, const void *p2, gpointer user_data)
> +{
> + guint diff;
> + GdkRectangle *displays = user_data;
> + guint i = *(guint*)p1;
> + guint j = *(guint*)p2;
> + GdkRectangle *m1 = &displays[i];
> + GdkRectangle *m2 = &displays[j];
> + diff = m1->x - m2->x;
> + if (diff == 0)
> + diff = m1->y - m2->y;
> + if (diff == 0)
> + diff = i - j;
> +
> + return diff;
> +}
> +
> +void
> +virt_viewer_align_monitors_linear(GdkRectangle *displays, guint ndisplays)
> +{
> + gint i, x = 0;
> + guint *sorted_displays;
> +
> + g_return_if_fail(displays != NULL);
> +
> + if (ndisplays == 0)
> + return;
> +
> + sorted_displays = g_new0(guint, ndisplays);
> + for (i = 0; i < ndisplays; i++)
> + sorted_displays[i] = i;
> + g_qsort_with_data(sorted_displays, ndisplays, sizeof(guint), displays_cmp, displays);
> +
> + /* adjust monitor positions so that there's no gaps or overlap between
> + * monitors */
> + for (i = 0; i < ndisplays; i++) {
> + guint nth = sorted_displays[i];
> + g_assert(nth < ndisplays);
> + GdkRectangle *rect = &displays[nth];
> + rect->x = x;
> + rect->y = 0;
> + x += rect->width;
> + }
> + g_free(sorted_displays);
> +}
> +
> +/* Shift all displays down so that the monitor origin is at (0,0). This reduces
Same comment about 'down' which could be removed.
> + * the size of the screen that will be required on the guest in the case where
> + * there is a fullscreen window on a non-primary client monitor. For example,
Is primary client monitor always +0+0 ? If not, let's remove the
'non-primary'
> + * instead of sending down the following configuration:
> + * 1280x1024+4240+0
> + * which implies that the guest screen must be at least 5520x1024, we'd send
> + * 1280x1024+0+0
> + * which implies the guest screen is only 1280x1024. The first version might
> + * fail if the guest video memory is not large enough to handle a screen of
> + * that size.
> + */
> +void
> +virt_viewer_shift_monitors_to_origin(GdkRectangle *displays, guint ndisplays)
> +{
> + gint xmin = G_MAXINT;
> + gint ymin = G_MAXINT;
> + gint i;
> +
> + g_return_if_fail(ndisplays > 0);
> +
> + for (i = 0; i < ndisplays; i++) {
> + GdkRectangle *display = &displays[i];
> + if (display->width > 0 && display->height > 0) {
> + xmin = MIN(xmin, display->x);
> + ymin = MIN(ymin, display->y);
> + }
> + }
> +
> + if (xmin > 0 || ymin > 0) {
> + g_debug("%s: Shifting all monitors down by (%i, %i)", G_STRFUNC, xmin, ymin);
s/down//
ACK.
Christophe
Attachment:
pgp2uycYuZ4QA.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
