(In reply to Larry Reznick from comment #0)
> In the screenshot attachment [...]

It looks like something went wrong there - the attachment is missing.

Created attachment 310525 [details]
Showing desktop with black background surrounding image while background app shows colors that should be on desktop.

Didn't see the attachment file referred in the report, so attached it again.

Shrank the image. It attached this time.

if you take your background file and run:

$ convert ~/Pictures/namegoeshere.jpg -alpha on ~/Pictures/namegoeshere.png

to convert it to an RGBA png image and then change your background to ~/Pictures/namegoeshere.png does the problem go away?

Yes, the problem does go away -- that is, the background colors do appear. BTW, got the following file size change:

$ ls -oh
total 13M
-rw-rw-r--. 1 lreznick 2.1M Feb 28  2009 ASouvenirOfVelasquez_JEMillais_.jpg
-rw-rw-r--. 1 lreznick  11M Sep  2 10:35 ASouvenirOfVelasquez_JEMillais_.png

Would hate to have to apply this.

so my guess is, we're trying to avoid drawing the color matte behind the wallpaper if we think the wallpaper is opaque and fully covers the visible screen, and the logic is screwy.

hmm just found this code:

      if (texture_has_alpha (texture))•
        {•
          ensure_color_texture (self);•
•
          pipeline = create_pipeline (PIPELINE_OVER_REVERSE);•
          cogl_pipeline_set_layer_texture (pipeline, 0, priv->color_texture);•
          cogl_framebuffer_draw_rectangle (fbo, pipeline, 0, 0, width, height);•
          cogl_object_unref (pipeline);•
        }•

so the check is only if the wallpaper is opaque, it doesn't pay any attention to if the background only covers part of the screen.

Created attachment 310538 [details] [review]
background: paint color matte for scaled and centered backgrounds

Some backgrounds don't fully fill the screen.  For those backgrounds
it's important to paint a color behind them to fill in the gaps.

This commit checks for scaled and centered backgrounds, and paints
a color behind them (such as it would do if the background were
translucent)

so i've attached a patch that seems right on the surface, but I haven't tested it. Do you mind trying it out? if you're not in a position to test patches, i'll try to reproduce and test later.

Review of attachment 310538 [details] [review]:

::: src/compositor/meta-background.c
@@ +665,3 @@
+      if (texture_has_alpha (texture) ||
+          priv->style == G_DESKTOP_BACKGROUND_STYLE_CENTERED ||
+          priv->style == G_DESKTOP_BACKGROUND_STYLE_SCALED)

assuming this works, it could be further optimized, by checking the size of the image in relation to the monitor

Review of attachment 310538 [details] [review]:

OK

(In reply to Ray Strode [halfline] from comment #9)
> so i've attached a patch that seems right on the surface, but I haven't
> tested it. Do you mind trying it out?

FWIW, I've tested the patch as part of the review.

(In reply to Florian Müllner from comment #12)
> FWIW, I've tested the patch as part of the review.

Uhm, but I did *not* try to reproduce the initial issue - apparently the image I used for testing already works without the patch ...

Agreed that checking the image size relative to the screen size when using Centered or Scaled makes sense.

How would I test your patch? Or should I?

BTW, this problem appeared only a few months ago. Earlier versions going way back did not have this problem.

I can upload a sample image if you want.

Review of attachment 310538 [details] [review]:

OK, so this patch doesn't actually work - the function is only called with style == WALLPAPER[0], so the additional conditions don't make a difference.

[0] https://git.gnome.org/browse/mutter/tree/src/compositor/meta-background.c#n736

(In reply to Florian Müllner from comment #16)
> OK, so this patch doesn't actually work - the function is only called with
> style == WALLPAPER[0], so the additional conditions don't make a difference.

ah I picked the wrong place of two places

Created attachment 310540 [details] [review]
background: paint color matte for scaled and centered backgrounds

Some backgrounds don't fully fill the screen.  For those backgrounds
it's important to paint a color behind them to fill in the gaps.

This commit checks for scaled and centered backgrounds, and paints
a color behind them (such as it would do if the background were
translucent)

(same caveats apply, still haven't tested it, and it still could be improved to look at the size of the covered area)

(In reply to Larry Reznick from comment #14)
> BTW, this problem appeared only a few months ago. Earlier versions going way
> back did not have this problem.
background drawing got rewritten about a year ago to be more efficient, it probably broke then.

I didn't notice the problem until I upgraded to Fedora 22 in May from F20. Change may have appeared in F21 but possibly not brought back to the F20 libs.

Review of attachment 310540 [details] [review]:

(In reply to Ray Strode [halfline] from comment #19)
> (same caveats apply, still haven't tested it

This one works as expected


> and it still could be improved
> to look at the size of the covered area)

Should we do that then, or at least add a comment in the code? Otherwise this looks OK to me

(In reply to Florian Müllner from comment #22)
> > and it still could be improved
> > to look at the size of the covered area)
> 
> Should we do that then, or at least add a comment in the code? Otherwise
> this looks OK to me
Yea I think so, but the patch is going to be more invasive, since we don't have the texture area available in that part of the code, but we do eventually need it, so we're going to need to move things around to accommodate the check

Created attachment 310548 [details] [review]
background: paint color matte for scaled and centered backgrounds

meta_background_get_texture tries to avoid drawing lower layers if
layers above are fully opaque.  The upper layers may be smaller than
the lower layers, and the code currently fails to account for this.
For instance, if a background is smaller than the monitor and centered,
then the background should appear on a colored matte, but the matte
won't currently get drawn.

This commit changes the code to do more careful accounting of what
areas have been drawn and only skip drawing lower layers if the upper
layers full occlude the lower layers.

maybe something like the above.  I'll test it out tomorrow if you neither of you get to it first

Review of attachment 310548 [details] [review]:

::: src/compositor/meta-background.c
@@ +816,2 @@
       if (texture1 != NULL &&
+          (priv->blend_factor != 1.0 || !cairo_region_is_empty (uncovered_region)))

actually reading over this, a more precise check here would be to get the texture1 area up front and only do the draw if cairo_region_contains_rectangle(uncovered_region, &texture1_area) != CAIRO_REGION_OVERLAP_OUT.  That means moving the get_texture_area call out of draw_texture

(In reply to Ray Strode [halfline] from comment #25)
> maybe something like the above.  I'll test it out tomorrow if you neither of
> you get to it first

I did not test whether drawing of the color layer is omitted when the centered/scaled background covers the monitor area, but it does fix the original issue.

Review of attachment 310548 [details] [review]:

Thanks for working on this bug! I think it's actually a bit different and simpler than you have here - the cogl blend modes being used are a bit unusual and obscure what's going on.

::: src/compositor/meta-background.c
@@ +526,3 @@
+    covered_region = cairo_region_create ();
+  else
+    covered_region = cairo_region_create_rectangle (&texture_area);

This is wrong in the WALLPAPER case. It's over-conservative in the STRETCHED/ZOOM/SPANNED case- that doesn't matter - but it would still be better to move this up into the switch statement.

See comments below, however.

@@ +816,2 @@
       if (texture1 != NULL &&
+          (priv->blend_factor != 1.0 || !cairo_region_is_empty (uncovered_region)))

When there are two textures, we're interpolating between them, not doing OVER compositing. So the check to draw the second texture is *simply* priv->blend_factor != 1.0.

The correct check not to draw the color is whether *both* the textures a) don't have alpha and b) cover the entire area of the monitor as drawn. There's no reason to return out regions here - draw_texture could simply return out a boolean whether the drawing covered the entire monitor (!texture_has_alpha() for the first part of the switch, and !texture_has_alpha && rects_equal(&monitor_rect, &texture_area) for the second part of the switch.

I don't think getting this exactly right is important - this happens only once per animation frame, or once per change of background - but on the other hand, "exactly right" differs from "good enough right" only by the addtion of the rects_equal(&monitor_rect, &texture_area), so it seems reasonable to do.

> This is wrong in the WALLPAPER case.
Oh indeed!
 
> It's over-conservative in the STRETCHED/ZOOM/SPANNED
by over-conservative you mean includes areas outside the monitor geometry?
That shouldn't be true for stretched, but could be true for zoom/spanned.

> When there are two textures, we're interpolating between them, not doing
> OVER compositing. So the check to draw the second texture is *simply*
> priv->blend_factor != 1.0.
okay i'll do a patch first to drop the alpha check, and then a follow up patch for the rest.

> The correct check not to draw the color is whether *both* the textures a)
> don't have alpha and b) cover the entire area of the monitor as drawn.
b) should be if *either* texture covers the entire monitor, not *both*, I think.
I mean if the monitor is covered, it's covered. There's no point in drawing behind
something that's covered up.

> The correct check not to draw the color is whether *both* the textures a)
> don't have alpha and b) cover the entire area of the monitor as drawn.

It just occurred to me you probably meant "both criteria" not "both textures" when you said both above.

Created attachment 310612 [details] [review]
background: simplify conditional in meta_background_get_texture

meta_background_get_texture only draws the bottom image texture
if
   1) the blend factor leaves the top image translucent
   or
   2) the top image is translucent from alpha

The latter case doesn't actually matter since we're using REPLACE
on the top image texture.

This commit drops the unnecessary check for the second case and
applies demorgans law to the conditional for clarity.

Created attachment 310613 [details] [review]
background: paint color matte for scaled and centered backgrounds

Some backgrounds don't fully fill the screen.  For those backgrounds
it's important to paint a color behind them to fill in the gaps.

This commit checks whether or not the background image textures take
up the entire monitor, and in the event they don't, draws a color
behind them (such as it would do if the background were
translucent).

(In reply to Ray Strode [halfline] from comment #29)
> > This is wrong in the WALLPAPER case.
> Oh indeed!
>  
> > It's over-conservative in the STRETCHED/ZOOM/SPANNED
> by over-conservative you mean includes areas outside the monitor geometry?
> That shouldn't be true for stretched, but could be true for zoom/spanned.

Yeah I meant ZOOM/SPANNED

> > When there are two textures, we're interpolating between them, not doing
> > OVER compositing. So the check to draw the second texture is *simply*
> > priv->blend_factor != 1.0.
> okay i'll do a patch first to drop the alpha check, and then a follow up
> patch for the rest.

Yeah, I don't know what I was thinking there - it should look pretty much like the previous test - we don't need to worry about texture2 to figure out if we should draw texture1 or not.

> > The correct check not to draw the color is whether *both* the textures a)
> > don't have alpha and b) cover the entire area of the monitor as drawn.
> b) should be if *either* texture covers the entire monitor, not *both*, I
> think.
> I mean if the monitor is covered, it's covered. There's no point in drawing
> behind something that's covered up.

No, I mean *both* - the two textures here aren't two layers, they are two images that are being interpolated between. If both are drawn, then the only way that the result obscures the background is if they are both full-screen opaque. At the end-points of the interpolation when we draw only one, only that one needs to be full-screen opaque.

Review of attachment 310612 [details] [review]:

Looks good.

Review of attachment 310613 [details] [review]:

::: src/compositor/meta-background.c
@@ +522,3 @@
                                                 texture_area.y + texture_area.height,
                                                 0, 0, 1.0, 1.0);
+      monitor_covered = !texture_has_alpha (texture) && memcmp (&texture_area, monitor_area, sizeof (cairo_rectangle_int_t)) == 0;

There was a missing break here before which is going stab you in the back.

@@ +822,3 @@
           cogl_pipeline_set_layer_wrap_mode (pipeline, 0, get_wrap_mode (priv->style));
 
+          monitor_covered = monitor_covered || draw_texture (self,

See my comment

(In reply to Owen Taylor from comment #33)
> No, I mean *both* - the two textures here aren't two layers, they are two
> images that are being interpolated between. If both are drawn, then the only
> way that the result obscures the background is if they are both full-screen
> opaque. At the end-points of the interpolation when we draw only one, only
> that one needs to be full-screen opaque.
grr, you're right of course, if one of the textures is centered, and one takes up the whole monitor then conceptually the centered texture needs a matte around it during the blend operation.

Created attachment 310615 [details] [review]
background: paint color matte for scaled and centered backgrounds

Some backgrounds don't fully fill the screen.  For those backgrounds
it's important to paint a color behind them to fill in the gaps.

This commit checks whether or not the background image textures take
up the entire monitor, and in the event they don't, draws a color
behind them (such as it would do if the background were
translucent).

Review of attachment 310615 [details] [review]:

Looks good.

Attachment 310612 [details] pushed as 0ffd425 - background: simplify conditional in meta_background_get_texture
Attachment 310615 [details] pushed as 1d56d50 - background: paint color matte for scaled and centered backgrounds