btw, that's the toolbar-small images in the search results.
The Miramar (Thunderbird dev version) icon in the dash is probably my fault somehow :)

Yes, that's a known issue. I located the bug in StTextureCache, where a correctly created Cogl texture is scaled to fit a square ClutterTexture. No fix yet, unfortunately. The patch from bug 642043 might impose a problem too once the texture cache issue is fixed.

Created attachment 184035 [details] [review]
st-texture-cache: Fix stretched images

Some functions in StTextureCache enforce square textures, even if
the underlying image may have a different width:height ratio. Add
padding in those cases to keep the resulting image from being
stretched.

Created attachment 184082 [details] [review]
st-texture-cache: Fix stretched images

*grumble*

Clutter start re-using memory after a while, and if we don't set the entire image (which we do when we add padding), the new image ends up being drawn on top of the old one ...

So make sure to clear the entire image before setting the region.

Review of attachment 184082 [details] [review]:

Works, seems OK as a simple approach that avoids fixing multiple callers.

According to my reading, the result of your patch is:

NO PADDING
==========
st_texture_cache_load_sliced_image
st_texture_cache_bind_pixbuf_property
st_texture_cache_load_uri_sync
st_texture_cache_load_file_to_cogl_texture
st_texture_cache_load_file_to_cairo_surface
st_texture_cache_load_file_simple
st_texture_cache_load_from_raw

PADDING
=======
st_texture_cache_load_icon_name
st_texture_cache_load_gicon
st_texture_cache_load_thumbnail
st_texture_cache_load_recent_thumbnail
st_texture_cache_load_from_data

BOTH
====
st_texture_cache_load_uri_async [pads iff available_width == available_height]

* Ones that pad should have a note in their docs that the result will be padded to be square
* Last is IMO wrong - doesn't make sense. Either it should always pad to the aspect ratio of available_width/height or never pad. Never pad would be consistent with an interpretation that only functions passed a single size parameter pad
* Commit message is weird because:
    Some functions in StTextureCache enforce square textures
  The functions didn't  before, but now they do

::: src/st/st-texture-cache.c
@@ +801,3 @@
+  size = MAX (width, height);
+
+  if (!add_padding)

Should definitely add a '|| width == height - clear' isn't free and most of the icons we load will be square

@@ +820,3 @@
+  cogl_pop_framebuffer ();
+  cogl_handle_unref (offscreen);
+

It's may be cheaper to set_region() the region above/below or right/left to avoid swapping between cpu/gpu access, but on the other hand, in an atlas'ed texture the atlas texture is likely already mapped for the GPU, and anyways, this is less code, so OK.

(In reply to comment #5)
> * Ones that pad should have a note in their docs that the result will be padded
> to be square

Is that necessary? The functions under PADDING all take a single size parameter, so padding is what I'd expect rather than stretching the image to a size x size square


> * Last is IMO wrong - doesn't make sense. Either it should always pad to the
> aspect ratio of available_width/height or never pad. Never pad would be
> consistent with an interpretation that only functions passed a single size
> parameter pad

I agree, but as it is using the same internal code path as the functions under PADDING it's a bit tricky. I now figured that a possible check which covers all uses of on_pixbuf_loaded from functions with size parameter but not st_texture_cache_load_uri_async is:

data->uri == NULL || data->thumbnail

That's horribly obscure and easy to break by future additions, so I'd rather add an additional member to AsyncTextureLoadData - is_square maybe?


> * Commit message is weird because:
>     Some functions in StTextureCache enforce square textures
>   The functions didn't  before, but now they do

IMO they did - they call clutter_actor_set_size (texture, size, size); the underlying CoglTexture may not be square, but the ClutterTexture certainly is.


> ::: src/st/st-texture-cache.c
> Should definitely add a '|| width == height - clear' isn't free and most of the
> icons we load will be square

Took me a while to parse, but yeah, will do that.

Created attachment 184103 [details] [review]
st-texture-cache: Fix stretched images

(In reply to comment #6)
> (In reply to comment #5)
> > * Ones that pad should have a note in their docs that the result will be padded
> > to be square

I didn't change this. With this patch version, only functions with a single size parameter get padding.


> data->uri == NULL || data->thumbnail
> 
> That's horribly obscure and easy to break by future additions, so I'd rather
> add an additional member to AsyncTextureLoadData - is_square maybe?

I went with enforced_square - still not entirely happy, I'm open for better suggestions.


> > * Commit message is weird because:
> >     Some functions in StTextureCache enforce square textures
> >   The functions didn't  before, but now they do
> 
> IMO they did - they call clutter_actor_set_size (texture, size, size); the
> underlying CoglTexture may not be square, but the ClutterTexture certainly is.

I tried to clear that up by replacing "textures" with "ClutterTextures" and "underlying image" with "underlying CoglTexture".


> > ::: src/st/st-texture-cache.c
> > Should definitely add a '|| width == height - clear' isn't free and most of the
> > icons we load will be square

Done.

(In reply to comment #6)
> (In reply to comment #5)
> > * Ones that pad should have a note in their docs that the result will be padded
> > to be square
> 
> Is that necessary? The functions under PADDING all take a single size
> parameter, so padding is what I'd expect rather than stretching the image to a
> size x size square

Hmm, didn't notice that we did clutter_actor_set_size(texturem size, size) - verified we do that for all ones taking a size parameter.
 
> > * Last is IMO wrong - doesn't make sense. Either it should always pad to the
> > aspect ratio of available_width/height or never pad. Never pad would be
> > consistent with an interpretation that only functions passed a single size
> > parameter pad
> 
> I agree, but as it is using the same internal code path as the functions under
> PADDING it's a bit tricky. I now figured that a possible check which covers all
> uses of on_pixbuf_loaded from functions with size parameter but not
> st_texture_cache_load_uri_async is:
> 
> data->uri == NULL || data->thumbnail
> 
> That's horribly obscure and easy to break by future additions, so I'd rather
> add an additional member to AsyncTextureLoadData - is_square maybe?
 
enforced_square is fine, it's not like it's exposed.
 
> > * Commit message is weird because:
> >     Some functions in StTextureCache enforce square textures
> >   The functions didn't  before, but now they do
> 
> IMO they did - they call clutter_actor_set_size (texture, size, size); the
> underlying CoglTexture may not be square, but the ClutterTexture certainly is.

Yeah, your current commit message looks fine.

> > ::: src/st/st-texture-cache.c
> > Should definitely add a '|| width == height - clear' isn't free and most of the
> > icons we load will be square
> 
> Took me a while to parse, but yeah, will do that.

Would help if I added the quotes in the right place.

Review of attachment 184103 [details] [review]:

Looks good

Attachment 184103 [details] pushed as 4208078 - st-texture-cache: Fix stretched images