GNOME Bugzilla – Bug 705510
Merge the wip/wayland-display branch
Last modified: 2013-08-19 07:49:53 UTC
In 3.10, the xrandr interaction is moving behind a D-Bus interface in mutter (or gnome-shell). The GnomeRR code in gnome-desktop should be ported to use this API. This is done in the wip/wayland-display branch.
Created attachment 250977 [details] [review] API: GnomeRR: replace direct XRandR calls with DBus calls to mutter Mutter now provides a DBus API that wraps XRandR and abstracts away the detail of running X or wayland. A number of API changes are needed, as GnomeRR is no longer in charge of maintaining the monitors.xml file updated.
Created attachment 250978 [details] [review] GnomeRR: restore support for gamma ramps The color panel of control center needs it. Note: the color panel and color g-s-d plugins will have different changes for wayland support, but they still need the code in the XRandR case.
Review of attachment 250977 [details] [review]: ::: libgnome-desktop/gnome-rr-config.c @@ +300,3 @@ return FALSE; + if (strcmp (output1->priv->product, output2->priv->product) != 0) g_strcmp0()? @@ +1092,2 @@ { + static const enum wl_output_transform y_reflected_map[4] = { [] instead of [4] will get you an array the right size already. @@ +1099,3 @@ + enum wl_output_transform ret; + + switch (rotation & 0x7F) Can we do better than magic numbers here? ::: libgnome-desktop/gnome-rr-config.h @@ -134,3 @@ -gboolean gnome_rr_config_apply_with_time (GnomeRRConfig *configuration, - GnomeRRScreen *screen, - guint32 timestamp, Are we losing the timestamp for the event? It seems to be pretty important in the X case, where we might be getting events for slightly older events (eg. you made 2 apply calls, and you're getting the event reply for the first one). ::: libgnome-desktop/gnome-rr-debug.c @@ +84,3 @@ for (i = 0; outputs[i] != NULL; i++) { g_print ("[%s]\n", gnome_rr_output_get_name (outputs[i])); + g_print ("\tconnected: %i\n", 1); We don't get to see disconnected outputs? That's going to be a problem in some cases (think turned off VGA monitor on the other end of a cable). ::: libgnome-desktop/gnome-rr-private.h @@ +88,3 @@ + META_POWER_SAVE_SUSPEND, + META_POWER_SAVE_OFF, +} MetaPowerSave; Can you get this as a full copy/pasted file from mutter instead of this? ::: libgnome-desktop/gnome-rr.c @@ +364,3 @@ + GnomeRROutput *output; + + g_variant_get_child (outputs, i, "(uxiaussssiauaua{sv})", &id, EEEEeeeeAAaarrghh. @@ +1225,3 @@ output->display_name = g_strdup (_("Built-in Display")); +#if 0 Remove that? @@ +1599,2 @@ + /* GnomeRRMode */ + crtc->current_mode = current_mode_id >= 0 ? mode_by_id (crtc->info, current_mode_id) : NULL; Not exactly readable...
Review of attachment 250978 [details] [review]: Where is the code that actually uses this?
Review of attachment 250978 [details] [review]: Looks good otherwise. ::: libgnome-desktop/gnome-rr.h @@ -197,3 @@ -#endif - -#if 0 /* configuration writing */ Can you split that change off?
(In reply to comment #3) > Review of attachment 250977 [details] [review]: > > ::: libgnome-desktop/gnome-rr-config.h > @@ -134,3 @@ > -gboolean gnome_rr_config_apply_with_time (GnomeRRConfig > *configuration, > - GnomeRRScreen *screen, > - guint32 timestamp, > > Are we losing the timestamp for the event? It seems to be pretty important in > the X case, where we might be getting events for slightly older events (eg. you > made 2 apply calls, and you're getting the event reply for the first one). There is a serial number in the DBus API, so XRandR timestamp complexity would be hidden in mutter, and we just make sure that ApplyConfiguration match a recent enough GetResources(). > ::: libgnome-desktop/gnome-rr-debug.c > @@ +84,3 @@ > for (i = 0; outputs[i] != NULL; i++) { > g_print ("[%s]\n", gnome_rr_output_get_name (outputs[i])); > + g_print ("\tconnected: %i\n", 1); > > We don't get to see disconnected outputs? That's going to be a problem in some > cases (think turned off VGA monitor on the other end of a cable). Do you mean externally turned off or disabled in g-c-c? In the former case, they're effectively unplugged, in the latter case they would still be reported as connected - *I hope*. In any case, basically all code paths I saw, except for configuration matching and saving, filtered out disconnected outputs, which is why I don't expose them in the API. > ::: libgnome-desktop/gnome-rr-private.h > @@ +88,3 @@ > + META_POWER_SAVE_SUSPEND, > + META_POWER_SAVE_OFF, > +} MetaPowerSave; > > Can you get this as a full copy/pasted file from mutter instead of this? It's just an enum, it seems an overkill to have a header file.
(In reply to comment #4) > Review of attachment 250978 [details] [review]: > > Where is the code that actually uses this? color plugin of gnome-settings-daemon (and Richard told me in person this is useful)
(In reply to comment #6) > (In reply to comment #3) > > Review of attachment 250977 [details] [review] [details]: > > > > ::: libgnome-desktop/gnome-rr-config.h > > @@ -134,3 @@ > > -gboolean gnome_rr_config_apply_with_time (GnomeRRConfig > > *configuration, > > - GnomeRRScreen *screen, > > - guint32 timestamp, > > > > Are we losing the timestamp for the event? It seems to be pretty important in > > the X case, where we might be getting events for slightly older events (eg. you > > made 2 apply calls, and you're getting the event reply for the first one). > > There is a serial number in the DBus API, so XRandR timestamp complexity would > be hidden in mutter, and we just make sure that ApplyConfiguration match a > recent enough GetResources(). Good. > > ::: libgnome-desktop/gnome-rr-debug.c > > @@ +84,3 @@ > > for (i = 0; outputs[i] != NULL; i++) { > > g_print ("[%s]\n", gnome_rr_output_get_name (outputs[i])); > > + g_print ("\tconnected: %i\n", 1); > > > > We don't get to see disconnected outputs? That's going to be a problem in some > > cases (think turned off VGA monitor on the other end of a cable). > > Do you mean externally turned off or disabled in g-c-c? > In the former case, they're effectively unplugged, in the latter case they > would still be reported as connected - *I hope*. > In any case, basically all code paths I saw, except for configuration matching > and saving, filtered out disconnected outputs, which is why I don't expose them > in the API. Right. > > ::: libgnome-desktop/gnome-rr-private.h > > @@ +88,3 @@ > > + META_POWER_SAVE_SUSPEND, > > + META_POWER_SAVE_OFF, > > +} MetaPowerSave; > > > > Can you get this as a full copy/pasted file from mutter instead of this? > > It's just an enum, it seems an overkill to have a header file. Up until the day when it changes. I would *really* prefer a separate header file.
Created attachment 251140 [details] [review] API: GnomeRR: replace direct XRandR calls with DBus calls to mutter Mutter now provides a DBus API that wraps XRandR and abstracts away the detail of running X or wayland. A number of API changes are needed, as GnomeRR is no longer in charge of maintaining the monitors.xml file updated.
Created attachment 251141 [details] [review] GnomeRR: restore support for gamma ramps The color panel of control center needs it. Note: the color panel and color g-s-d plugins will have different changes for wayland support, but they still need the code in the XRandR case.
Created attachment 251142 [details] [review] GnomeRR: restore support for raw EDID access The color plugin of gnome-settings-daemon needs it to build the default ICC profile for uncalibrated displays.
CCing Richard, as he asked me for EDID support.
Review of attachment 251140 [details] [review]: The rest looks good, but I'd feel easier not receiving huge structs like that over the wire, and have something slightly more structured (either because we share the GVariant definition with mutter like the "meta-xrandr-shared.h" or it's generated from the xml file. Looks good to commit once you've gone over that hurdle. ::: libgnome-desktop/gnome-rr-config.c @@ +1137,1 @@ + g_variant_builder_init (&crtc_builder, G_VARIANT_TYPE ("a(uiiiuaua{sv})")); This is pretty gross. @@ +1152,1 @@ + g_variant_builder_add (&crtc_builder, "(uiiiuaua{sv})", This is still gross.
Review of attachment 251141 [details] [review]: Yep.
Review of attachment 251142 [details] [review]: Looks good.
(In reply to comment #13) > Review of attachment 251140 [details] [review]: > > The rest looks good, but I'd feel easier not receiving huge structs like that > over the wire, and have something slightly more structured (either because we > share the GVariant definition with mutter like the "meta-xrandr-shared.h" or > it's generated from the xml file. > > Looks good to commit once you've gone over that hurdle. > > ::: libgnome-desktop/gnome-rr-config.c > @@ +1137,1 @@ > + g_variant_builder_init (&crtc_builder, G_VARIANT_TYPE > ("a(uiiiuaua{sv})")); > > This is pretty gross. > > @@ +1152,1 @@ > + g_variant_builder_add (&crtc_builder, "(uiiiuaua{sv})", > > This is still gross. I understand your complaint, but on the other hand I don't want to move entirely to a{sv} or similar, because this is all required stuff. Also, using string keys increases the amount of stuff transferred on the bus. Would it be fine to have the defines for the structure types in the shared header?
Shared header would be good, generated structure would be even better (from the xml file).
(In reply to comment #17) > Shared header would be good, generated structure would be even better (from the > xml file). How do I generate a structure from the xml file?
(In reply to comment #18) > (In reply to comment #17) > > Shared header would be good, generated structure would be even better (from the > > xml file). > > How do I generate a structure from the xml file? I would have hoped that gdbus-codegen could generate something useful for that (a constant for example) but it doesn't. Shared header will be fine for now.
Created attachment 252052 [details] [review] GnomeRR: fix gnome-rr-debug test case Use GnomeRR facilities to read the EDID instead of direct X access, because the actual XID is not exposed in the API (although it is exported on the bus)
Created attachment 252053 [details] [review] GnomeRRConfig: try again when failing application because of access denied If we get an error due to stale configuration information, try again a second time after refreshing the screen. This can happen when mutter is using the X11 backend, as that causes multiple events, so a single ApplyConfiguration() can increase the serial multiple times, and there is a race between when mutter gets the event (increasing the serial) and when we get it, asking for the new configuration. I'm not sure this is the right approach - it kind of defeats the point of having a serial in the first place. But without it, on X11, you need press "detect monitors" all the time, otherwise you get access denied errors.
Created attachment 252080 [details] [review] GnomeRR: add async construction of GnomeRRScreen We aren't going anywhere, if gnome-settings-daemon blocks on mutter which is blocking on gnome-settings-daemon...
Created attachment 252086 [details] [review] GnomeRR: add async construction of GnomeRRScreen We aren't going anywhere, if gnome-settings-daemon blocks on mutter which is blocking on gnome-settings-daemon... This one probably works even better.
Review of attachment 252052 [details] [review]: Looks fine
Review of attachment 252086 [details] [review]: looks fine to me, apart from the question below ::: libgnome-desktop/gnome-rr.c @@ +593,3 @@ + priv->info = screen_info_new (self, &error); + if (!priv->info) + return g_task_return_error (task, error); Are we leaving things in an inconsistent state here, with priv->proxy != NULL and priv->info == NULL ?
(In reply to comment #25) > Review of attachment 252086 [details] [review]: > > looks fine to me, apart from the question below > > ::: libgnome-desktop/gnome-rr.c > @@ +593,3 @@ > + priv->info = screen_info_new (self, &error); > + if (!priv->info) > + return g_task_return_error (task, error); > > Are we leaving things in an inconsistent state here, with priv->proxy != NULL > and priv->info == NULL ? Yes, but cleanup checks for priv->info anyway, and that's the only allowed operation if initialization fails.
Review of attachment 252053 [details] [review]: hmm, ok. ::: libgnome-desktop/gnome-rr-config.c @@ +641,3 @@ + if (!ok) + { + if (g_error_matches (local_error, G_DBUS_ERROR, G_DBUS_ERROR_ACCESS_DENIED)) access denied is the error we get in this case ? hmm, ok
Review of attachment 252086 [details] [review]: ok then
Attachment 251140 [details] pushed as 632dffb - API: GnomeRR: replace direct XRandR calls with DBus calls to mutter Attachment 251141 [details] pushed as 52662ef - GnomeRR: restore support for gamma ramps Attachment 251142 [details] pushed as a31024a - GnomeRR: restore support for raw EDID access Attachment 252052 [details] pushed as 65b6926 - GnomeRR: fix gnome-rr-debug test case Attachment 252053 [details] pushed as dc85c78 - GnomeRRConfig: try again when failing application because of access denied Attachment 252086 [details] pushed as eb34fd0 - GnomeRR: add async construction of GnomeRRScreen