[virt-tools-list] [PATCH virt-viewer 15/19] Hook up handling of Monitors
Marc-André Lureau
marcandre.lureau at gmail.com
Tue Jul 17 12:38:15 UTC 2012
Hi
On Tue, Jul 17, 2012 at 2:15 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 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?
added a 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?
I don't know what would be very huge, probably >16 starts to be
*really* a lot already, but if it's what's the server has..
>> +
>> + 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
Yes, corrected. I wonder if it wouldn't be simpler to have only the
fallback version.. What do you think?
>
> 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?
They are not necessarily correlated. We need a list of display per
channel, not per virt viewer session. The session could have fewer
displays, or perhaps more in the future (if it later supports both
monitorconfig and multiple display channels)
>> + }
>> +
>> + 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
The display widget is GInitiallyUnowned, and will get sink ref by the
parent widget. We need an additional ref for the book-keeping here.
>> + 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.
ack
>> + g_clear_pointer(&monitors, g_array_unref);
>
> This is very new in glib, do we have a fallback definition for this symbol?
No, adding it to glib-compat.h.
--
Marc-André Lureau
More information about the virt-tools-list
mailing list