After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 705510 - Merge the wip/wayland-display branch
Merge the wip/wayland-display branch
Status: RESOLVED FIXED
Product: gnome-desktop
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Desktop Maintainers
Desktop Maintainers
wayland
Depends on: 705670
Blocks: 705507 705573
 
 
Reported: 2013-08-05 12:58 UTC by Matthias Clasen
Modified: 2013-08-19 07:49 UTC
See Also:
GNOME target: 3.10
GNOME version: ---


Attachments
API: GnomeRR: replace direct XRandR calls with DBus calls to mutter (122.10 KB, patch)
2013-08-06 14:51 UTC, Giovanni Campagna
needs-work Details | Review
GnomeRR: restore support for gamma ramps (6.49 KB, patch)
2013-08-06 14:51 UTC, Giovanni Campagna
accepted-commit_now Details | Review
API: GnomeRR: replace direct XRandR calls with DBus calls to mutter (129.54 KB, patch)
2013-08-08 11:52 UTC, Giovanni Campagna
committed Details | Review
GnomeRR: restore support for gamma ramps (4.21 KB, patch)
2013-08-08 11:52 UTC, Giovanni Campagna
committed Details | Review
GnomeRR: restore support for raw EDID access (4.00 KB, patch)
2013-08-08 11:52 UTC, Giovanni Campagna
committed Details | Review
GnomeRR: fix gnome-rr-debug test case (3.05 KB, patch)
2013-08-17 15:59 UTC, Giovanni Campagna
committed Details | Review
GnomeRRConfig: try again when failing application because of access denied (2.77 KB, patch)
2013-08-17 16:00 UTC, Giovanni Campagna
committed Details | Review
GnomeRR: add async construction of GnomeRRScreen (5.74 KB, patch)
2013-08-17 23:15 UTC, Giovanni Campagna
none Details | Review
GnomeRR: add async construction of GnomeRRScreen (7.61 KB, patch)
2013-08-17 23:44 UTC, Giovanni Campagna
committed Details | Review

Description Matthias Clasen 2013-08-05 12:58:50 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.
Comment 1 Giovanni Campagna 2013-08-06 14:51:06 UTC
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.
Comment 2 Giovanni Campagna 2013-08-06 14:51:44 UTC
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.
Comment 3 Bastien Nocera 2013-08-06 15:11:53 UTC
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...
Comment 4 Bastien Nocera 2013-08-06 15:13:48 UTC
Review of attachment 250978 [details] [review]:

Where is the code that actually uses this?
Comment 5 Bastien Nocera 2013-08-06 15:18:03 UTC
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?
Comment 6 Giovanni Campagna 2013-08-07 09:17:21 UTC
(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.
Comment 7 Giovanni Campagna 2013-08-07 09:19:31 UTC
(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)
Comment 8 Bastien Nocera 2013-08-07 09:24:51 UTC
(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.
Comment 9 Giovanni Campagna 2013-08-08 11:52:31 UTC
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.
Comment 10 Giovanni Campagna 2013-08-08 11:52:36 UTC
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.
Comment 11 Giovanni Campagna 2013-08-08 11:52:40 UTC
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.
Comment 12 Giovanni Campagna 2013-08-08 11:56:47 UTC
CCing Richard, as he asked me for EDID support.
Comment 13 Bastien Nocera 2013-08-08 13:45:44 UTC
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.
Comment 14 Bastien Nocera 2013-08-08 13:46:13 UTC
Review of attachment 251141 [details] [review]:

Yep.
Comment 15 Bastien Nocera 2013-08-08 13:47:29 UTC
Review of attachment 251142 [details] [review]:

Looks good.
Comment 16 Giovanni Campagna 2013-08-08 15:11:16 UTC
(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?
Comment 17 Bastien Nocera 2013-08-08 15:13:42 UTC
Shared header would be good, generated structure would be even better (from the xml file).
Comment 18 Giovanni Campagna 2013-08-08 15:22:14 UTC
(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?
Comment 19 Bastien Nocera 2013-08-08 15:38:25 UTC
(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.
Comment 20 Giovanni Campagna 2013-08-17 15:59:09 UTC
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)
Comment 21 Giovanni Campagna 2013-08-17 16:00:15 UTC
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.
Comment 22 Giovanni Campagna 2013-08-17 23:15:14 UTC
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...
Comment 23 Giovanni Campagna 2013-08-17 23:44:07 UTC
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.
Comment 24 Matthias Clasen 2013-08-18 18:16:50 UTC
Review of attachment 252052 [details] [review]:

Looks fine
Comment 25 Matthias Clasen 2013-08-18 18:21:25 UTC
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 ?
Comment 26 Giovanni Campagna 2013-08-18 18:24:48 UTC
(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.
Comment 27 Matthias Clasen 2013-08-18 19:44:07 UTC
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
Comment 28 Matthias Clasen 2013-08-19 01:44:07 UTC
Review of attachment 252086 [details] [review]:

ok then
Comment 29 Matthias Clasen 2013-08-19 01:44:12 UTC
Review of attachment 252086 [details] [review]:

ok then
Comment 30 Giovanni Campagna 2013-08-19 07:49:24 UTC
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