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 694393 - Any background style different from zoom is broken in the overview
Any background style different from zoom is broken in the overview
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2013-02-21 22:14 UTC by Giovanni Campagna
Modified: 2013-03-02 22:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screenshot of the problem (181.89 KB, image/png)
2013-02-21 22:55 UTC, Giovanni Campagna
  Details
compositor: invert background texture scale factor (8.65 KB, patch)
2013-02-25 03:12 UTC, Ray Strode [halfline]
reviewed Details | Review
compositor: when tiling background, center on screen (5.70 KB, patch)
2013-02-25 03:12 UTC, Ray Strode [halfline]
committed Details | Review
compositor: fix background vignette for non-stretched styles (8.42 KB, patch)
2013-02-25 03:12 UTC, Ray Strode [halfline]
committed Details | Review

Description Giovanni Campagna 2013-02-21 22:14:41 UTC
Set up a background image which is smaller than you screen, and set
org.gnome.desktop.background picture-options
to anything but zoom.
You'll observe that the effect in the overview is wrong, because it's applied to the gradient and to the image separately, and only to the first copy of the image, if more than one exists.

The same problem exists, to a lesser extent, in the screenshield too. But we can always say that effects exist there as a stop gap until we fix having a different background, so it's less of a bug.

The simple and appropriate, but unfortunately expensive, fix is to go back to ClutterOffscreenEffect and render the background into a screen-sized texture on which we can apply the effect.
Alternatively, we can put the necessary logic into GLSL, but it's neither easy nor clean.

(Btw, zoom is the default value, and that works, so this is not high priority)
Comment 1 drago01 2013-02-21 22:41:38 UTC
(In reply to comment #0)

> 
> The simple and appropriate, but unfortunately expensive, fix is to go back to
> ClutterOffscreenEffect and render the background into a screen-sized texture on
> which we can apply the effect.

Ugh no. Yeah it is the easiest way to do it but as you said the costs are too high so this simply isn't an option.
Comment 2 Ray Strode [halfline] 2013-02-21 22:42:43 UTC
so you're saying we're not being 100% correct because we're double applying the effect.

If we care about this, I think we can mitigate the issue by clipping out the top background in the same way we clip out windows that are on top. That still has problems with translucency, of course.

We could also add a meta_background_flatten() that flattens one background onto another in the event the background has effects other than Meta.BackgroundEffects.NONE

I don't think we need to resort to screen sized textures.  why do you say that?
Comment 3 Giovanni Campagna 2013-02-21 22:54:12 UTC
Maybe I wasn't clear on what's the problem: it's not that the apply the effect twice, but that what we apply to the actor on top is wrongly sized.

I hope the screenshot explains enough (it's a solid green 300x400 image, centered over a white pattern).

Also, I don't understand how meta_background_flatten() would work.
Comment 4 Giovanni Campagna 2013-02-21 22:55:08 UTC
Created attachment 237122 [details]
Screenshot of the problem
Comment 5 Ray Strode [halfline] 2013-02-21 23:01:22 UTC
ohh, we just need to fix the glsl to use the correct position when computing the pixel brightness.
Comment 6 Ray Strode [halfline] 2013-02-25 03:12:37 UTC
Created attachment 237324 [details] [review]
compositor: invert background texture scale factor

The background code computes a scale factor used for mapping
a texture to an actor. It multiples the texture area in
pixel space by the scale factor to get texture coordinates
between 0.0 and 1.0.

This scale factor actually reads better (for me, today, anyway)
as a divisor than a multiplicand.

This commit inverts the factor, and uses division instead of
multiplication.
Comment 7 Ray Strode [halfline] 2013-02-25 03:12:41 UTC
Created attachment 237325 [details] [review]
compositor: when tiling background, center on screen

The WALLPAPER style of background painting currently
draws starting in the upper left corner of each monitor.

This isn't really correct, it means the seam between
monitors doesn't match up and edges look unbalanced if
the tile isn't a multipe of monitor size.

Really, the tiles should be centered in the middle of
the screen.  (Just like when tiling a bathroom floor,
tiles should start in the center of the room.)

This commit reworks the math to make that happen.
Comment 8 Ray Strode [halfline] 2013-02-25 03:12:45 UTC
Created attachment 237326 [details] [review]
compositor: fix background vignette for non-stretched styles

The background vignette currently fits itself to the painted
texture, instead of the monitor.  This causes some very
wrong looking drawing for backgrounds that don't fill the screen.

This commit reworks the vignette shader code to be clearer, more
correct, and parameterized so that it knows how to scale and
position the vignette.
Comment 9 drago01 2013-03-02 11:19:14 UTC
Review of attachment 237324 [details] [review]:

I am not sure I like this ... the older one reads better to me but I have no strong opinion either way so ok.
Comment 10 drago01 2013-03-02 11:22:06 UTC
Review of attachment 237325 [details] [review]:

Makes sense.
Comment 11 drago01 2013-03-02 11:26:31 UTC
Review of attachment 237326 [details] [review]:

Looks good.
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-03-02 11:34:30 UTC
Review of attachment 237324 [details] [review]:

::: src/compositor/meta-background.c
@@ +420,3 @@
       cairo_region_get_rectangle (paintable_region, i, &texture_subarea);
 
+      tx1 = (texture_subarea.x - texture_area.x) / texture_x_scale;

"scale" usually means multiplication. I think we should change the variable name in this case, or just keep it as multiplication.
Comment 13 Ray Strode [halfline] 2013-03-02 22:33:22 UTC
(In reply to comment #9)
> Review of attachment 237324 [details] [review]:
> 
> I am not sure I like this ... the older one reads better to me but I have no
> strong opinion either way so ok.

I'll just drop it.
Comment 14 Ray Strode [halfline] 2013-03-02 22:39:29 UTC
Attachment 237325 [details] pushed as 0e3d164 - compositor: when tiling background, center on screen
Attachment 237326 [details] pushed as 6e831c8 - compositor: fix background vignette for non-stretched styles