[virt-tools-list] [PATCH virt-viewer 15/19] Hook up handling of Monitors
Christophe Fergeau
cfergeau at redhat.com
Tue Jul 17 12:15:08 UTC 2012
On Mon, Jul 16, 2012 at 06:57:50PM +0200, Marc-André Lureau wrote:
> Rely on spice-gtk display channel monitors property to manage
> displays. The same display channel may now provide several monitors,
> the SpiceDisplay widget must be told which monitor to display
> ---
> src/virt-viewer-display-spice.c | 17 +++----
> src/virt-viewer-display-spice.h | 2 +-
> src/virt-viewer-session-spice.c | 98 +++++++++++++++++++++++++++++++++++----
> 3 files changed, 98 insertions(+), 19 deletions(-)
>
> diff --git a/src/virt-viewer-display-spice.c b/src/virt-viewer-display-spice.c
> index 53430dd..ca74c1a 100644
> --- a/src/virt-viewer-display-spice.c
> +++ b/src/virt-viewer-display-spice.c
> @@ -172,7 +172,7 @@ virt_viewer_display_spice_size_allocate(VirtViewerDisplaySpice *self,
> {
> gdouble dw = allocation->width, dh = allocation->height;
> guint zoom = 100;
> - guint channelid;
> + guint nth;
>
> if (virt_viewer_display_get_auto_resize(VIRT_VIEWER_DISPLAY(self)) == FALSE)
> return;
> @@ -187,13 +187,11 @@ virt_viewer_display_spice_size_allocate(VirtViewerDisplaySpice *self,
> dh /= ((double)zoom / 100.0);
> }
>
> - g_object_get(self->priv->channel, "channel-id", &channelid, NULL);
> + g_object_get(self, "nth-display", &nth, NULL);
>
> SpiceMainChannel *main_channel = virt_viewer_session_spice_get_main_channel(
> VIRT_VIEWER_SESSION_SPICE(virt_viewer_display_get_session(VIRT_VIEWER_DISPLAY(self))));
> - spice_main_set_display(main_channel,
> - channelid,
> - 0, 0, dw, dh);
> + spice_main_set_display(main_channel, nth, 0, 0, dw, dh);
> }
>
> static void
> @@ -212,7 +210,8 @@ enable_accel_changed(VirtViewerApp *app,
>
> GtkWidget *
> virt_viewer_display_spice_new(VirtViewerSessionSpice *session,
> - SpiceChannel *channel)
> + SpiceChannel *channel,
> + gint monitorid)
> {
> VirtViewerDisplaySpice *self;
> VirtViewerApp *app;
> @@ -222,15 +221,17 @@ virt_viewer_display_spice_new(VirtViewerSessionSpice *session,
> g_return_val_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel), NULL);
>
> g_object_get(channel, "channel-id", &channelid, NULL);
> + // We don't allow monitorid != 0 && channelid != 0
> + g_return_val_if_fail(channelid == 0 || monitorid == 0, NULL);
>
> self = g_object_new(VIRT_VIEWER_TYPE_DISPLAY_SPICE,
> "session", session,
> - "nth-display", channelid,
> + "nth-display", channelid + monitorid,
This looks like it's not necessarily unique with (channel id, monitor id)
being (0, 2) and (2, 0), can it an issue? Or are we guaranteed that this
situation cannot occur?
Update: Alon told me on IRC that the 2 situations are mutually exclusive,
can you add that to the comment?
> NULL);
> self->priv->channel = channel;
>
> g_object_get(session, "spice-session", &s, NULL);
> - self->priv->display = spice_display_new(s, channelid);
> + self->priv->display = spice_display_new_with_monitor(s, channelid, monitorid);
> g_object_unref(s);
>
> virt_viewer_signal_connect_object(self->priv->display, "notify::ready",
> diff --git a/src/virt-viewer-display-spice.h b/src/virt-viewer-display-spice.h
> index 701ed85..c2013ec 100644
> --- a/src/virt-viewer-display-spice.h
> +++ b/src/virt-viewer-display-spice.h
> @@ -66,7 +66,7 @@ struct _VirtViewerDisplaySpiceClass {
>
> GType virt_viewer_display_spice_get_type(void);
>
> -GtkWidget* virt_viewer_display_spice_new(VirtViewerSessionSpice *session, SpiceChannel *channel);
> +GtkWidget* virt_viewer_display_spice_new(VirtViewerSessionSpice *session, SpiceChannel *channel, gint monitorid);
>
> G_END_DECLS
>
> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c
> index 6577237..0df11ef 100644
> --- a/src/virt-viewer-session-spice.c
> +++ b/src/virt-viewer-session-spice.c
> @@ -414,6 +414,80 @@ agent_connected_changed(SpiceChannel *cmain,
> }
>
> static void
> +destroy_display(gpointer data)
> +{
> + VirtViewerDisplay *display = VIRT_VIEWER_DISPLAY(data);
> + VirtViewerSession *session = virt_viewer_display_get_session(display);
> +
> + DEBUG_LOG("Destroying spice display %p", display);
> + virt_viewer_session_remove_display(session, display);
> + g_object_unref(display);
> +}
> +
> +static void
> +virt_viewer_session_spice_display_monitors(SpiceChannel *channel,
> + GParamSpec *pspec G_GNUC_UNUSED,
> + VirtViewerSessionSpice *self)
> +{
> + GArray *monitors = NULL;
> + GPtrArray *displays = NULL;
> + GtkWidget *display;
> + guint i, monitors_max;
> +
> + g_object_get(channel,
> + "monitors", &monitors,
> + "monitors-max", &monitors_max,
> + NULL);
> + g_return_if_fail(monitors != NULL);
> + g_return_if_fail(monitors->len <= monitors_max);
Is monitors-max coming from the network? If so, should we make sure it's
not very huge to avoid allocating too much memory?
> +
> + displays = g_object_get_data(G_OBJECT(channel), "virt-viewer-displays");
> + if (displays == NULL) {
> + displays = g_ptr_array_new();
> +#if GLIB_CHECK_VERSION(2, 22, 0)
> + g_ptr_array_set_free_func(displays, destroy_display);
> + g_object_set_data_full(G_OBJECT(channel), "virt-viewer-displays",
> + displays, (GDestroyNotify)g_ptr_array_unref);
> +#endif
missing
#else
g_object_set_data_full(G_OBJECT(channel), "virt-viewer-displays",
displays, NULL);
#endif
or something similar
VirtViewerSession already has its list of displays, do we really need to
maintain an additional list of displays and associate it with the
SpiceChannel object?
> + }
> +
> + g_ptr_array_set_size(displays, monitors_max);
> +
> + for (i = 0; i < monitors_max; i++) {
> + display = g_ptr_array_index(displays, i);
> + if (display == NULL) {
> + display = virt_viewer_display_spice_new(self, channel, i);
> + DEBUG_LOG("creating spice display (#:%d)", i);
> + g_ptr_array_index(displays, i) = g_object_ref(display);
The g_object_ref is not needed, this is leaking the initial ref from
_display_spice_new
> + }
> +
> + g_object_freeze_notify(G_OBJECT(display));
> + virt_viewer_display_set_enabled(VIRT_VIEWER_DISPLAY(display), FALSE);
> + virt_viewer_session_add_display(VIRT_VIEWER_SESSION(self),
> + VIRT_VIEWER_DISPLAY(display));
> + }
> +
> + for (i = 0; i < monitors->len; i++) {
> + SpiceDisplayMonitorConfig *c = &g_array_index(monitors, SpiceDisplayMonitorConfig, i);
Naming this variable 'monitor' would make things more readable imo.
> + display = g_ptr_array_index(displays, c->id);
> + g_return_if_fail(display != NULL);
> +
> + if (c->width == 0 || c->width == 0)
> + continue;
> +
> + virt_viewer_display_set_enabled(VIRT_VIEWER_DISPLAY(display), TRUE);
> + virt_viewer_display_set_desktop_size(VIRT_VIEWER_DISPLAY(display),
> + c->width, c->height);
> + }
> +
> + for (i = 0; i < monitors_max; i++)
> + g_object_thaw_notify(g_ptr_array_index(displays, i));
> +
> + g_clear_pointer(&monitors, g_array_unref);
This is very new in glib, do we have a fallback definition for this symbol?
> +
> +}
> +
> +static void
> virt_viewer_session_spice_channel_new(SpiceSession *s,
> SpiceChannel *channel,
> VirtViewerSession *session)
> @@ -441,20 +515,17 @@ virt_viewer_session_spice_channel_new(SpiceSession *s,
>
> g_signal_connect(channel, "notify::agent-connected", G_CALLBACK(agent_connected_changed), self);
> agent_connected_changed(channel, NULL, self);
> +
> + g_signal_emit_by_name(session, "session-connected");
> }
>
> if (SPICE_IS_DISPLAY_CHANNEL(channel)) {
> - GtkWidget *display;
> -
> - g_signal_emit_by_name(session, "session-connected");
> + g_signal_emit_by_name(session, "session-initialized");
>
> - DEBUG_LOG("new display channel (#%d)", id);
> - display = virt_viewer_display_spice_new(self, channel);
> - g_object_set_data(G_OBJECT(channel), "virt-viewer-display", display);
> - virt_viewer_session_add_display(VIRT_VIEWER_SESSION(session),
> - VIRT_VIEWER_DISPLAY(display));
> + g_signal_connect(channel, "notify::monitors",
> + G_CALLBACK(virt_viewer_session_spice_display_monitors), self);
>
> - g_signal_emit_by_name(session, "session-initialized");
> + spice_channel_connect(channel);
> }
>
> if (SPICE_IS_INPUTS_CHANNEL(channel)) {
> @@ -538,7 +609,14 @@ virt_viewer_session_spice_channel_destroy(G_GNUC_UNUSED SpiceSession *s,
> if (SPICE_IS_DISPLAY_CHANNEL(channel)) {
> VirtViewerDisplay *display = g_object_get_data(G_OBJECT(channel), "virt-viewer-display");
> DEBUG_LOG("zap display channel (#%d, %p)", id, display);
> - virt_viewer_session_remove_display(session, display);
> +#if !GLIB_CHECK_VERSION(2, 22, 0)
> + GPtrArray *displays = g_object_get_data(G_OBJECT(channel), "virt-viewer-displays");
> + if (displays) {
> + g_ptr_array_foreach(displays, (GFunc)destroy_display, NULL);
> + g_ptr_array_free(displays, TRUE);
> + g_object_set_data(channel, "virt-viewer-displays", NULL);
> + }
> +#endif
> }
>
> if (SPICE_IS_PLAYBACK_CHANNEL(channel) && self->priv->audio) {
> --
> 1.7.10.4
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20120717/8928b485/attachment.sig>
More information about the virt-tools-list
mailing list