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 679901 - Support _NET_WM_OPAQUE_REGION
Support _NET_WM_OPAQUE_REGION
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2012-07-13 23:30 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-02-06 17:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Support _NET_WM_OPAQUE_REGION (5.39 KB, patch)
2012-07-13 23:30 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Support _NET_WM_OPAQUE_REGION (5.84 KB, patch)
2012-07-16 07:25 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Support _NET_WM_OPAQUE_REGION (7.75 KB, patch)
2012-07-16 19:08 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Support _NET_WM_OPAQUE_REGION (7.31 KB, patch)
2012-07-16 19:09 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
window-actor: Clean up finalization code with simple uses of g_clear_pointer (2.22 KB, patch)
2013-01-15 03:40 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window-actor: Remove custom region destruction methods with g_clear_pointer (4.54 KB, patch)
2013-01-15 03:40 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window-actor: Merge two simple methods (2.98 KB, patch)
2013-01-15 03:40 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window-actor: Remove conditional checks for the shape region (3.93 KB, patch)
2013-01-15 03:40 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
window-actor: Remove the bounding region (6.46 KB, patch)
2013-01-15 03:40 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Support _NET_WM_OPAQUE_REGION (6.82 KB, patch)
2013-01-15 03:40 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Support _NET_WM_OPAQUE_REGION (7.21 KB, patch)
2013-01-15 04:11 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Support _NET_WM_OPAQUE_REGION (7.96 KB, patch)
2013-01-15 08:16 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
window-actor: Ensure we always have a valid shape_region (1.36 KB, patch)
2013-01-16 18:33 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Support _NET_WM_OPAQUE_REGION (8.00 KB, patch)
2013-01-16 22:18 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2012-07-13 23:30:53 UTC
See https://mail.gnome.org/archives/wm-spec-list/2011-December/msg00000.html

KDE/Kwin has already implemented this property. I don't know if any clients
other than Qt support it (it might be worth adding to GTK+).
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-07-13 23:30:55 UTC
Created attachment 218774 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-07-13 23:32:18 UTC
Open question: if there's a _NET_WM_OPAQUE_REGION on a non-ARGB32 window, do we respect the client, and stop the overhead of scanning? Do we ignore the region set by the client? Do we intersect/union the two?
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-07-16 07:25:42 UTC
Created attachment 218882 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.



Now with a patch that actually works!
Comment 4 Owen Taylor 2012-07-16 16:14:07 UTC
(In reply to comment #2)
> Open question: if there's a _NET_WM_OPAQUE_REGION on a non-ARGB32 window, do we
> respect the client, and stop the overhead of scanning? Do we ignore the region
> set by the client? Do we intersect/union the two?

I don't really understand this question - the scanning is about the opaque part of the decorations, which the client doesn't know, right?

Are you instead talking about any shape region there might be on the window? If there's a non-default output shape and a _NET_WM_OPAQUE region both set (ARGB or RGB), I think it's safest to intersect the two rather than assuming that the _NET_WM_OPAQUE_REGION already includes the output shape - this is slightly slower but I'm imaging the weird bugs you could get if some toolkit is updated to set _NET_WM_OPAQUE_REGION but some application goes directly to the X library to shape a window behind the toolkits back.

(And it's a corner case - the combination is going to be very rare.)
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-07-16 17:16:17 UTC
(In reply to comment #4)
> Are you instead talking about any shape region there might be on the window? If
> there's a non-default output shape and a _NET_WM_OPAQUE region both set (ARGB
> or RGB), I think it's safest to intersect the two rather than assuming that the
> _NET_WM_OPAQUE_REGION already includes the output shape - this is slightly
> slower but I'm imaging the weird bugs you could get if some toolkit is updated
> to set _NET_WM_OPAQUE_REGION but some application goes directly to the X
> library to shape a window behind the toolkits back.

Yeah, that's what I meant. I wonder if we can get a spec update for that to match what KWin does.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-07-16 19:08:40 UTC
Created attachment 218941 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.



Aha, I didn't realize that the intersection semantics were part of
the specification. Updated.
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-07-16 19:09:59 UTC
Created attachment 218942 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.



No idea where the _NET_WM_WINDOW_TYPE change came from.
Comment 8 Owen Taylor 2013-01-09 16:39:37 UTC
Review of attachment 218942 [details] [review]:

Would be good to land a gnome-terminal patch to set this for fully opaque terminals soon so we don't forget - it's actually a big win.

::: src/compositor/meta-window-actor.c
@@ +1696,3 @@
+            }
+          else
+            return cairo_region_reference (priv->window->opaque_region);

What about the handling of the frame? This code looks like that a window with _NET_WM_OPAQUE_REGION set is no longer counted as having an obscuring frame, since OPAQUE_REGION only handles the client area. I think this does matter - although drawing small strips underneath the titlebar isn't a huge deal, "leakages" like this make validating and understanding what is going on in drawing harder - I don't want us to be redrawing a small strip under the title of fully opaque ARGB windows but not under the titlebar of RGB windows.

::: src/core/window-props.c
@@ +1692,3 @@
     { display->atom__NET_WM_STRUT,         META_PROP_VALUE_INVALID, reload_struts,            FALSE, FALSE },
     { display->atom__NET_WM_STRUT_PARTIAL, META_PROP_VALUE_INVALID, reload_struts,            FALSE, FALSE },
+    { display->atom__NET_WM_OPAQUE_REGION, META_PROP_VALUE_CARDINAL_LIST, reload_opaque_region,     TRUE, FALSE },

I think you want this to be TRUE TRUE - this property makes as much sense on a override-redirect window as a normal application window.

For a little less confusion, I'd move it up enough so that it's with all the other properties that have INIT=true - since the ordering is how properties are ordered on initial load, it makes sense to have all the initially-loaded properties together.

::: src/core/window.c
@@ +7359,3 @@
+    {
+      cairo_rectangle_int_t *rects;
+      int i = 0, j = 0;

I'd rather have these initializations right before the loop - having them away from the loop is a recipe for mischief if someone decides to add a different loop using i as a counter variable in between. (And is hard to read)

@@ +7386,3 @@
+
+ out:
+  window->opaque_region = opaque_region;

you leak the old opaque region? it also look like you leak it when the window object is destroyed.
Comment 9 Jasper St. Pierre (not reading bugmail) 2013-01-15 03:40:40 UTC
Created attachment 233493 [details] [review]
window-actor: Clean up finalization code with simple uses of g_clear_pointer

While not a massive change by itself, adding new code to use g_clear_pointer
without porting existing usage looks strange.
Comment 10 Jasper St. Pierre (not reading bugmail) 2013-01-15 03:40:45 UTC
Created attachment 233494 [details] [review]
window-actor: Remove custom region destruction methods with g_clear_pointer
Comment 11 Jasper St. Pierre (not reading bugmail) 2013-01-15 03:40:47 UTC
Created attachment 233495 [details] [review]
window-actor: Merge two simple methods

With some recent changes to how mask textures are constructed from
shapes, a helper method we made was only called in one place, allowing
us to drop a reference/destroy, and remove a double clear.
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-01-15 03:40:50 UTC
Created attachment 233496 [details] [review]
window-actor: Remove conditional checks for the shape region

With recent changes in the way the window mask texture is constructed,
the shape_region is always set, which means that we can remove
conditionals checking if the shape region is set.
Comment 13 Jasper St. Pierre (not reading bugmail) 2013-01-15 03:40:53 UTC
Created attachment 233497 [details] [review]
window-actor: Remove the bounding region

With the shape region always set, it turns out the bounding region
is only used in one place, that's easily replaced with a variable
we already have available to us.
Comment 14 Jasper St. Pierre (not reading bugmail) 2013-01-15 03:40:56 UTC
Created attachment 233498 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.
Comment 15 Jasper St. Pierre (not reading bugmail) 2013-01-15 03:44:01 UTC
While working on this tonight, I realized that we always set shape_region, and the rest of these patches just fell out.

 src/compositor/meta-window-actor.c | 241 +++++++++++++++++++++++--------------------------------------------------------------------------------------------------------------
 1 file changed, 41 insertions(+), 200 deletions(-)

So I'm happy.
Comment 16 Jasper St. Pierre (not reading bugmail) 2013-01-15 04:11:48 UTC
Created attachment 233499 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.



--

Properly support ARGB32 windows without _NET_WM_OPAQUE_REGION again.

This patch series has been tested on:

  * Normal GTK+ windows
  * OR windows, managed by GTK+ (custom test, uses GTK_WINDOW_TYPE_POPUP)
  * Shaped normal windows (oclock)
  * Shaped bad windows (sets a client shape beyond the extents of the window,
                        originally made as a test case for bug #627880)
  * ARGB32 windows, opaque and not opaque (gnome-terminal, transparent and solid)

I have not actually tested with a client that sets _NET_WM_OPAQUE_REGION,
though I expect to soon. The first giant list of patches should be able
to land, though.
Comment 17 Jasper St. Pierre (not reading bugmail) 2013-01-15 08:16:45 UTC
Created attachment 233504 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.



Fix support for _NET_WM_OPAQUE_REGION using a hand-constructed test case,
and add support for pathological 0-width/0-height cases that should be
treated as empty.
Comment 18 Owen Taylor 2013-01-16 17:22:02 UTC
Review of attachment 233493 [details] [review]:

Basically good, one comment deleted that shouldn't have been. Good with that fixed.

::: src/compositor/meta-window-actor.c
@@ -431,3 @@
-
-  /*
-   * Release the extra reference we took on the actor.

You lost this comment, it's not the *most* useful comment (the important stuff is in the paired comment in meta_window_actor_constructed), but I think it's worth keeping
Comment 19 Owen Taylor 2013-01-16 17:29:05 UTC
Review of attachment 233494 [details] [review]:

if you like :-)
Comment 20 Owen Taylor 2013-01-16 17:32:32 UTC
Review of attachment 233495 [details] [review]:

OK
Comment 21 Owen Taylor 2013-01-16 17:46:20 UTC
Review of attachment 233496 [details] [review]:

Not comfortable with this patch - I don't think you've restructured the code in a way that we can be sure that you've handled:

/* We need to be defensive here because there are corner cases
 * where getting the shape fails on a window being destroyed			
 * and similar.			
 */

Note that check_needs_reshape() does *not* always set priv->shape_region, and also there's really no guarantee that check_needs_reshape() is always 100% of the time called before, say, meta_window_actor_get_paint_volume()

If you want to remove these checks, you need to structure the code so that the shape_region is 100% of the time set to something from the point that the window is constructed, or you need to have some sort of meta_window_actor_ensure_shape_region() that 100% of the time sets the shape region before using it.

Maybe just use the bounds region as a fallback for the shape region so you don't have to check both?
Comment 22 Owen Taylor 2013-01-16 17:50:00 UTC
(In reply to comment #21)

> Note that check_needs_reshape() does *not* always set priv->shape_region, and
> also there's really no guarantee that check_needs_reshape() is always 100% of
> the time called before, say, meta_window_actor_get_paint_volume()
> 
> If you want to remove these checks, you need to structure the code so that the
> shape_region is 100% of the time set to something from the point that the
> window is constructed, or you need to have some sort of
> meta_window_actor_ensure_shape_region() that 100% of the time sets the shape
> region before using it.
> 
> Maybe just use the bounds region as a fallback for the shape region so you
> don't have to check both?

Sorry, partially invalid comment, since priv->shape_region is always set by checK_needs_reshape(), but the comment about ordering still applies.
Comment 23 Jasper St. Pierre (not reading bugmail) 2013-01-16 18:33:31 UTC
Created attachment 233618 [details] [review]
window-actor: Ensure we always have a valid shape_region

Force a reshape at startup to ensure that shape_region is always
constructed and valid, even if the window is unredirected or frozen.



This patch goes after "Merge two simple methods". It can be squashed with the conditional checks patch if you want.
Comment 24 Owen Taylor 2013-01-16 19:01:34 UTC
Review of attachment 233504 [details] [review]:

::: src/compositor/meta-window-actor.c
@@ +1531,1 @@
     return priv->shape_region;

this doesn't make sense to me, has_opaque_region is not set for RGB windows -

(!priv->argb || priv->has_opaque_region) 

?

@@ +2154,3 @@
+       * opaque, the shadow shape will be the top semicircle.
+       */
+      cairo_region_intersect (region, priv->window->opaque_region);

In reference to this, note the handling of clip_shadow_under_window() - I think for windows with a frame, this is fairly satisfactory handling, and I don't think we start wanting to take the opaque region into account - which is going to give wonky behavior with weird edge cases - there is no requirement that you set your opaque region to any sort of region that makes sense to the user - it's just needs to be fully contained within opaque pixels.

For windows without a frame, we never currently shadow argb windows, so again it won't matter. Do we want to change that? - I guess the argument is that we want to shadow CSD windows the same as windows where we add the frame, and in the simplest case the shadow corresponds to the opaque region. I'm OK trying that, though there will be definitely be pathologies if clients try to do sophisticated things like tracking the opaque region from drawing.

This code looks to me like it will result in all drawing being clipped to the opaque region, since it runs before build_and_scan_frame_mask()

::: src/core/window.c
@@ +7392,3 @@
+
+  if (window->opaque_region)
+    g_clear_pointer (&window->opaque_region, g_free);

not g_free!

@@ +7433,3 @@
+
+      /* Pathological case: Operations like intersections will
+       * fail if cairo regions empty. Guard against somebody

that sounds odd, really?
Comment 25 Jasper St. Pierre (not reading bugmail) 2013-01-16 22:18:48 UTC
Created attachment 233631 [details] [review]
Support _NET_WM_OPAQUE_REGION

This new hint allows compositors to know what portions of a window
will be obscured, as a region above them is opaque. For an RGB window,
possible to glean this information from the bounding shape region of
a client window, but not for an ARGB32 window. This new hint allows
clients that use ARGB32 windows to say which part of the window is
opaque, allowing this sort of optimization.
Comment 26 Jasper St. Pierre (not reading bugmail) 2013-01-18 05:29:37 UTC
Attachment 233493 [details] pushed as 079dd60 - window-actor: Clean up finalization code with simple uses of g_clear_pointer
Attachment 233494 [details] pushed as 53534b4 - window-actor: Remove custom region destruction methods with g_clear_pointer
Attachment 233495 [details] pushed as e28a36a - window-actor: Merge two simple methods


Pushed first three patches with suggested changes.
Comment 27 Owen Taylor 2013-02-04 16:24:13 UTC
Review of attachment 233618 [details] [review]:

Looks good
Comment 28 Owen Taylor 2013-02-04 16:26:56 UTC
Review of attachment 233496 [details] [review]:

OK now - I think it's better to squash with the other patch - it's clearer to do everything at once, then to have one tiny patch to ensure an invariant, and another patch to use the invariant.

::: src/compositor/meta-window-actor.c
@@ +1834,3 @@
+      MetaShadowFactory *factory = meta_shadow_factory_get_default ();
+      const char *shadow_class = meta_window_actor_get_shadow_class (self);
+      cairo_rectangle_int_t shape_bounds;

mixed-declarations-and-code - in terms of conformance it's fine - we're requiring C99 - in terms of style, it's a bit odd to do it just on one place.
Comment 29 Owen Taylor 2013-02-04 20:24:50 UTC
Review of attachment 233631 [details] [review]:

Looks good to me other than one comment about a comment

::: src/compositor/meta-window-actor.c
@@ +2151,3 @@
+       * pixels. For these regions, we want to prevent painting
+       * regions in windows below this and prevent painting a
+       * shadow for this area as well.

"prevent painting a shadow for this area" sounds weird - it sounds like we don't paint the shadow that is created by the opaque area - which is certainly not the case. "For these regions, we want to avoid painting windows and shadows beneath them." or something like that.
Comment 30 Owen Taylor 2013-02-04 21:52:19 UTC
Review of attachment 233497 [details] [review]:

Looks basically OK to me, some details about exactly what ::size-changed means need a bit of thought.

::: src/compositor/meta-window-actor.c
@@ +1717,3 @@
         g_warning ("NOTE: Not using GLX TFP!\n");
 
+      meta_window_actor_update_shape (self);

Now that you are using window->rect in check_needs_reshape rather than pixmap size, there's really no reason to call this here rather than in meta_window_actor_sync_actor_position() right after calling meta_window_actor_queue_create_pixmap(), which is a more obvious place for it. That gets things wrong if window->rect changes without window->frame.rect changing, but that's gotten wrong by the current code too.

@@ +1718,3 @@
 
+      meta_window_actor_update_shape (self);
+      g_signal_emit (self, signals[SIZE_CHANGED], 0);

Hmm, you are now emitting this unconditionally - the time I like it least is when a window is newly created. But it seems like it is already emitted on new window creation, so that's not a regression. And emitting it when a window is, say, redirected after being unredirected, is not a big problem.

::size-changed is supposed to correspond to the actual drawn area of the window (the outer rect) - see usage in workspace.js. So again, if window->rect changes without window->frame.rect changing, we'll get things wrong, and were getting things wrong before.

I think it's OK as is (here - not moved along with the call to update_shape()) but needs a comment, along the lines of:

    /* ::size-changed is supposed to refer to meta_window_get_outer_rect(). Emitting
     * it here works pretty much OK because a new value of the *input* rect (which
     * is the outer rect with the addition of invisible borders), forces a new pixmap
     * and we get here. In the rare case where a change to the window size was
     * exactly balanced by a change to the invisible borders, we would miss emitting
     * the signal. We also emit spurious signals when we get a new pixmap without a new
     * size, but that should be mostly harmless.
     */

Hmmm, thinking about it, workspace.js is actually taking ::size-changed to refer to the combination of the input rect *and* the outer rect - it wants to be called if either is called. Adjust the above accordingly, but I don't think this really matters in practice.
Comment 31 Jasper St. Pierre (not reading bugmail) 2013-02-06 05:06:43 UTC
Attachment 233496 [details] pushed as 19420f1 - window-actor: Remove conditional checks for the shape region
Attachment 233497 [details] pushed as 3fe5a67 - window-actor: Remove the bounding region


Pushed these two patches with suggested fixes. Sorry about the messed up commit history in git.
Comment 32 Jasper St. Pierre (not reading bugmail) 2013-02-06 05:08:24 UTC
Attachment 233631 [details] pushed as a613a55 - Support _NET_WM_OPAQUE_REGION
Comment 33 drago01 2013-02-06 11:12:23 UTC
(In reply to comment #30)
> Review of attachment 233497 [details] [review]:
> 
> Looks basically OK to me, some details about exactly what ::size-changed means
> need a bit of thought.
> 
> ::: src/compositor/meta-window-actor.c
> @@ +1717,3 @@
>          g_warning ("NOTE: Not using GLX TFP!\n");
> 
> +      meta_window_actor_update_shape (self);
> 
> Now that you are using window->rect in check_needs_reshape rather than pixmap
> size, there's really no reason to call this here rather than in
> meta_window_actor_sync_actor_position() right after calling
> meta_window_actor_queue_create_pixmap(), which is a more obvious place for it.
> That gets things wrong if window->rect changes without window->frame.rect
> changing, but that's gotten wrong by the current code too.

Well without calling this here shadows end up broken (the shadows just contain a part of the background).

A patch like this:

diff --git a/src/compositor/meta-window-actor.c b/src/compositor/meta-window-act
index 8c1617d..bd31202 100644
--- a/src/compositor/meta-window-actor.c
+++ b/src/compositor/meta-window-actor.c
@@ -1733,6 +1733,8 @@ check_needs_pixmap (MetaWindowActor *self)
 
       texture = meta_shaped_texture_get_texture (META_SHAPED_TEXTURE (priv->act
 
+      meta_window_actor_update_shape (self);
+
       /*
        * This only works *after* actually setting the pixmap, so we have to
        * do it here.

Fixes it.