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 679268 - Update cached textures automatically on file changes
Update cached textures automatically on file changes
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.4.x
Other Linux
: Normal minor
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 644125 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-07-02 17:20 UTC by Jean-François Fortin Tam
Modified: 2012-10-17 21:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st-texture-cache: Add texture-file-changed signal (5.47 KB, patch)
2012-09-20 21:02 UTC, Florian Müllner
needs-work Details | Review
st-theme-node: Add method to invalidate drawing state (1.70 KB, patch)
2012-09-20 21:02 UTC, Florian Müllner
committed Details | Review
st-widget: Keep background-image and border-image updated (2.57 KB, patch)
2012-09-20 21:02 UTC, Florian Müllner
none Details | Review
st-widget: Keep background-image and border-image updated (2.58 KB, patch)
2012-09-20 21:49 UTC, Florian Müllner
needs-work Details | Review
st-texture-cache: Add API to remove cache data (2.16 KB, patch)
2012-09-21 14:50 UTC, Florian Müllner
reviewed Details | Review
st-widget: Add method to clear background-image (2.52 KB, patch)
2012-09-21 14:50 UTC, Florian Müllner
committed Details | Review
userMenu: Force reload of background-image on icon changes (1.20 KB, patch)
2012-09-21 14:50 UTC, Florian Müllner
reviewed Details | Review
st-texture-cache: Add API to remove cache data (2.40 KB, patch)
2012-09-21 21:03 UTC, Florian Müllner
committed Details | Review
userMenu: Force reload of background-image on icon changes (1.43 KB, patch)
2012-09-21 21:04 UTC, Florian Müllner
committed Details | Review
userMenu: Rely on automatic texture changes (1.18 KB, patch)
2012-09-24 20:11 UTC, Florian Müllner
committed Details | Review
st: Remove unused methods (4.04 KB, patch)
2012-09-24 20:11 UTC, Florian Müllner
committed Details | Review
st-texture-cache: Add texture-file-changed signal (5.38 KB, patch)
2012-10-17 19:09 UTC, Florian Müllner
committed Details | Review
st-widget: Keep background-image and border-image updated (2.60 KB, patch)
2012-10-17 19:13 UTC, Florian Müllner
committed Details | Review
st: Fix handling of file:// URIs in _st_theme_resolve_uri() (719 bytes, patch)
2012-10-17 19:13 UTC, Florian Müllner
committed Details | Review
st: Canonicalize URLs in stylesheets (1.56 KB, patch)
2012-10-17 20:41 UTC, Florian Müllner
needs-work Details | Review
st: Canonicalize URLs in stylesheets (1.67 KB, patch)
2012-10-17 21:01 UTC, Florian Müllner
none Details | Review
st: Canonicalize URLs in stylesheets (1.93 KB, patch)
2012-10-17 21:27 UTC, Florian Müllner
committed Details | Review

Description Jean-François Fortin Tam 2012-07-02 17:20:35 UTC
1. Activate the user menu in the top-right corner
2. Click the avatar to open control center
3. Change the avatar

Result: avatar has been changed (it is updated on the screensaver lock screen) but the change is not reflected in the shell's user menu until you restart gnome-shell.
Comment 1 Florian Müllner 2012-07-02 19:43:17 UTC
Ugh. This is a regression introduced by recent accountsservice changes. When changing the icon, rather than passing the new url around (which could be unavailable when the icon is needed), accountsservice copies the file to /var/lib/AccountsService/icons/$USER and emits the user::changed signal. We handle this signal and update the icon as expected - so far, so good. But for performance reasons we cache texture data, and this is what is triggering the issue here - all of texture type, file name and image size being equal, we return the cached texture.

Not sure off-hand what would be the best fix, I see various alternatives:

 - change accountsservice to not use a single per-user icon file, e.g.
   copy icons to /var/lib/AccountsService/icons/$USER-$UNIQUE_ID instead;
   this would require accountsservice to add some policy for cleaning up
   older icons, which might be undesirable

 - also use mtime in the cache key for URLs; while this should fix the issue
   rather transparently, stat isn't free ...

 - add explicit API to StTextureCache to enforce a cache policy for specific
   uris; I'm not a big fan of this approach, but I can't think of anything
   better if (1) is not an option ...
Comment 2 Colin Walters 2012-07-02 21:24:19 UTC
We could also have the texture cache automatically GFileMonitor watch on local paths.
Comment 3 Florian Müllner 2012-07-02 21:34:38 UTC
True, though we would still need to expose some API to StThemeNode (we try to only emit widget::style-changed signals on actual style changes, which means we'll miss a "change" from "background-image: url(/var/lib/AccountsService/icons/$USER);" to "background-image: url(/var/lib/AccountsService/icons/$USER);")
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-07-02 21:38:40 UTC
No we won't -- as soon as accountsservice updates the picture, the file monitor will fire, the texture-cache will invalidate the picture in the cache, fire a cache-invalidated (a renamed icon-theme-changed signal), and we should be good.
Comment 5 Florian Müllner 2012-07-02 21:44:01 UTC
Ugh, so you are suggesting to throw away *all* cached textures because a *single* file changed?
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-07-02 21:47:04 UTC
No? It will only evict one texture. It will traverse the entire scene graph sending "UPDATED", though. I don't see any other way to do it, other than tying a backreference to a widget to a texture.
Comment 7 Matthias Clasen 2012-07-06 00:37:32 UTC
> But for
> performance reasons we cache texture data, and this is what is triggering the
> issue here - all of texture type, file name and image size being equal, we
> return the cached texture.

Presumably the mtime of the file does change though, no ?
Comment 8 Matthias Clasen 2012-09-20 10:04:31 UTC
With the avatar in the lock dialog, this is rather visible. Can we not do the simple fix of including the timestamp in the key here, please ?
Comment 9 Florian Müllner 2012-09-20 19:40:06 UTC
(In reply to comment #8)
> With the avatar in the lock dialog, this is rather visible. Can we not do the
> simple fix of including the timestamp in the key here, please ?

No - it is not that simple. Apart from disk accesses that a cache is supposed to avoid, we do not necessarily hit the texture cache at all - on style changes, before the old style is cleared, state that is reusable is copied over to the new style. For background-images, that is obviously based on the the paths being the same.

Now the good news is that an implementation based on GFileMonitor is actually easier than the mtime approach - patch coming (once I write up the commit message).
Comment 10 Florian Müllner 2012-09-20 21:02:03 UTC
Created attachment 224858 [details] [review]
st-texture-cache: Add texture-file-changed signal

For textures loaded from files, the cache might hide image changes
by keeping the data of a previous version around indefinitely. For
instance AccountsService will notify of avatar changes, but as new
image is copied over the old one, we will continue to use the old
image data.
Install a file monitor for each file resource we load and clear
the corresponding data from the cache on changes, emitting the
new StTextureCache::texture-file-changed signal.
Comment 11 Florian Müllner 2012-09-20 21:02:06 UTC
Created attachment 224859 [details] [review]
st-theme-node: Add method to invalidate drawing state

StThemeNode caches its resources aggressively to keep the required
work on paint to a minimum - right now, resources are only recreated
on allocation changes.
In order to update the background-image property correctly when the
underlying file changes, resources need to be recreated without a
size change, so add an explicit method for that.
Comment 12 Florian Müllner 2012-09-20 21:02:11 UTC
Created attachment 224860 [details] [review]
st-widget: Keep background-image and border-image updated

Currently we miss changes to a file referenced in background-image
or border-image.
Connect to the StTextureCache::texture-file-changed signal to keep
up with file changes and update the drawing state if necessary.
Comment 13 Florian Müllner 2012-09-20 21:06:40 UTC
*** Bug 644125 has been marked as a duplicate of this bug. ***
Comment 14 Florian Müllner 2012-09-20 21:49:42 UTC
Created attachment 224867 [details] [review]
st-widget: Keep background-image and border-image updated

Gah, did some last-minute changes to apply to border-images as well and didn't retest.
Comment 15 Matthias Clasen 2012-09-20 23:52:28 UTC
This still seems much more complicated and expensive than necessary to fix avatars. 

What I would have expected: When you get a user::changed signal from accountsservice, you invalidate the cache entry for the user avatar. Problem solved.

What am I missing ?
Comment 16 Florian Müllner 2012-09-21 13:32:06 UTC
(In reply to comment #15)
> What I would have expected: When you get a user::changed signal from
> accountsservice, you invalidate the cache entry for the user avatar. Problem
> solved.

If you mean for 3.6, then I'm OK with that. I still like the more general approach, as it has the side effect of fixing a long-standing annoyance for the design team (e.g. having to restart the shell continuously while working on shell artwork).
Comment 17 Florian Müllner 2012-09-21 14:48:26 UTC
Comment on attachment 224858 [details] [review]
st-texture-cache: Add texture-file-changed signal

Marking "reviewed" to get this off the list for now
Comment 18 Florian Müllner 2012-09-21 14:48:44 UTC
Comment on attachment 224867 [details] [review]
st-widget: Keep background-image and border-image updated

Dto.
Comment 19 Florian Müllner 2012-09-21 14:50:07 UTC
Created attachment 224929 [details] [review]
st-texture-cache: Add API to remove cache data

The current API assumes that image data loaded from files remains
valid during the life time of the shell. This assumption is mostly
valid for image files we provide ourselves (with the exception being
designers working on those files), but not necessarily for "external"
files - provide API to explicitly remove cached data associated with
a URI for those cases.
Comment 20 Florian Müllner 2012-09-21 14:50:13 UTC
Created attachment 224930 [details] [review]
st-widget: Add method to clear background-image

For performance reasons, resources required to paint a widget are
aggressively cached; we know of at least one case where our caching
prevents updating the used background-image correctly, so add explicit
API to clear all associated cache data.
Comment 21 Florian Müllner 2012-09-21 14:50:18 UTC
Created attachment 224931 [details] [review]
userMenu: Force reload of background-image on icon changes

When changing the user's avatar image, AccountsService will
overwrite the old image with the new one, so the location
returned by get_icon_file() is always the same.
In order to pick up the change, we need to make sure to clear the
previous image from both StTextureCache and StThemeNode's paint
cache.
Comment 22 Colin Walters 2012-09-21 19:49:23 UTC
Review of attachment 224929 [details] [review]:

::: src/st/st-texture-cache.c
@@ +97,3 @@
+void
+st_texture_cache_clear_uri (StTextureCache *cache,
+                            const char     *uri)

Could use a bit of gtk-doc, like:

/**
 * st_texture_cache_clear_uri:
 * @cache: a #StTextureCache
 * @uri: URI of cached object
 *
 * If the given @uri is known to have been modified
 * externally, this function may be used to invalidate
 * the in-memory cache.
 */
Comment 23 Colin Walters 2012-09-21 19:51:49 UTC
Review of attachment 224930 [details] [review]:

Makes sense.
Comment 24 Colin Walters 2012-09-21 19:53:12 UTC
Review of attachment 224931 [details] [review]:

Could use a comment above (link to this bug?)

Otherwise looks obviously right.
Comment 25 Florian Müllner 2012-09-21 19:54:36 UTC
(In reply to comment #22)
> Could use a bit of gtk-doc

Yeah, though I'd like to switch to the generic approach post-3.6, which would make this function obsolete ...
Comment 26 Florian Müllner 2012-09-21 21:03:27 UTC
Created attachment 224956 [details] [review]
st-texture-cache: Add API to remove cache data

Added the suggested comment.
Comment 27 Florian Müllner 2012-09-21 21:04:24 UTC
Created attachment 224957 [details] [review]
userMenu: Force reload of background-image on icon changes

Added the suggested comment - note that

  st-theme-node: Add method to invalidate drawing state

is still required by the non-generic approach, so review appreciated :-)
Comment 28 Jasper St. Pierre (not reading bugmail) 2012-09-22 14:02:06 UTC
Review of attachment 224956 [details] [review]:

Sane enough.
Comment 29 Jasper St. Pierre (not reading bugmail) 2012-09-22 14:02:47 UTC
Review of attachment 224859 [details] [review]:

Can we make this part of st-theme-node-private.h?
Comment 30 Jasper St. Pierre (not reading bugmail) 2012-09-22 14:03:00 UTC
Review of attachment 224957 [details] [review]:

Sure.
Comment 31 Matthias Clasen 2012-09-24 19:43:27 UTC
Works fine in brief testing.
Comment 32 Florian Müllner 2012-09-24 20:08:07 UTC
Attachment 224859 [details] pushed as c4c470c - st-theme-node: Add method to invalidate drawing state
Attachment 224930 [details] pushed as cd024e2 - st-widget: Add method to clear background-image
Attachment 224956 [details] pushed as dc9ad8d - st-texture-cache: Add API to remove cache data
Attachment 224957 [details] pushed as 6f3cf0a - userMenu: Force reload of background-image on icon changes

(In reply to comment #29)
> Can we make this part of st-theme-node-private.h?

st-theme-node-private.h is used to share code between st-theme-node.c and st-theme-node-drawing.c - the method in question is intended to be used from st-widget.c, so I'm leaving it in the regular header.
Comment 33 Florian Müllner 2012-09-24 20:11:50 UTC
Created attachment 225098 [details] [review]
userMenu: Rely on automatic texture changes

This reverts commit 6f3cf0ae50af0f7a9cd68760abe695009605c0d7.
Comment 34 Florian Müllner 2012-09-24 20:11:55 UTC
Created attachment 225099 [details] [review]
st: Remove unused methods

This reverts commits cd024e21f0d and dc9ad8df8074.
Comment 35 Jasper St. Pierre (not reading bugmail) 2012-10-02 23:40:24 UTC
Review of attachment 224867 [details] [review]:

::: src/st/st-widget.c
@@ +284,3 @@
+    return;
+
+  path = strstr (uri, "://") + strlen ("://");

g_filename_from_uri? _st_theme_resolve_url? This won't unescape the URI.

@@ +286,3 @@
+  path = strstr (uri, "://") + strlen ("://");
+
+  changed = g_strcmp0 (st_theme_node_get_background_image (node), path) == 0;

st_theme_node_get_background_image is a direct translation of the CSS. If it's relative, then this won't work. We should probably make _st_theme_resolve_url call realpath.
Comment 36 Jasper St. Pierre (not reading bugmail) 2012-10-02 23:46:19 UTC
Review of attachment 224858 [details] [review]:

::: src/st/st-texture-cache.c
@@ +45,3 @@
+
+  /* File monitors to evict cache data on changes */
+  GHashTable *file_monitors; /* GFile * -> GFileMonitor * */

char * -> GFileMonitor *

@@ +1034,3 @@
+  char *uri, *key;
+
+  if (event_type != G_FILE_MONITOR_EVENT_CREATED &&

EVENT_CHANGED?
Comment 37 Jasper St. Pierre (not reading bugmail) 2012-10-02 23:46:42 UTC
Review of attachment 225099 [details] [review]:

Sure.
Comment 38 Jasper St. Pierre (not reading bugmail) 2012-10-02 23:46:48 UTC
Review of attachment 225098 [details] [review]:

Sure.
Comment 39 Florian Müllner 2012-10-17 19:09:58 UTC
Created attachment 226676 [details] [review]
st-texture-cache: Add texture-file-changed signal

Updated according to review.
Comment 40 Jasper St. Pierre (not reading bugmail) 2012-10-17 19:12:22 UTC
Review of attachment 226676 [details] [review]:

Sure.
Comment 41 Florian Müllner 2012-10-17 19:13:16 UTC
Created attachment 226678 [details] [review]
st-widget: Keep background-image and border-image updated

(In reply to comment #35)
> g_filename_from_uri? _st_theme_resolve_url? This won't unescape the URI.

Sure.


> @@ +286,3 @@
> +  path = strstr (uri, "://") + strlen ("://");
> +
> +  changed = g_strcmp0 (st_theme_node_get_background_image (node), path) == 0;
> 
> st_theme_node_get_background_image is a direct translation of the CSS. If it's
> relative, then this won't work. We should probably make _st_theme_resolve_url
> call realpath.

Uh? Either _st_theme_resolve_url() translates the CSS to an absolute path, or returns NULL on failure - so st_theme_node_get_background_image() will never be relative (FWIW, given our current style, the patch was *only* tested with relative URLs ...)
Comment 42 Florian Müllner 2012-10-17 19:13:46 UTC
Created attachment 226679 [details] [review]
st: Fix handling of file:// URIs in _st_theme_resolve_uri()

Unrelated drive-by fix.
Comment 43 Jasper St. Pierre (not reading bugmail) 2012-10-17 19:40:47 UTC
Review of attachment 226679 [details] [review]:

Sure.
Comment 44 Jasper St. Pierre (not reading bugmail) 2012-10-17 19:43:16 UTC
(In reply to comment #41)
> Uh? Either _st_theme_resolve_url() translates the CSS to an absolute path, or
> returns NULL on failure - so st_theme_node_get_background_image() will never be
> relative (FWIW, given our current style, the patch was *only* tested with
> relative URLs ...)

As far as I can tell, _st_theme_resolve_url does not actually translate to an absolute path. If we have url(../one/two.png);, it will lookup the stylesheet URL, get the basedir of that, and then use g_build_filename on that, which doesn't do any sort of resolution:

>>> GLib.build_filenamev(['directory/containing/a/stylesheet', '../one/two.png'])
'directory/containing/a/stylesheet/../one/two.png'
Comment 45 Florian Müllner 2012-10-17 20:41:54 UTC
Created attachment 226688 [details] [review]
st: Canonicalize URLs in stylesheets

Make _st_theme_resolve_url() a bit smarter by canonicalizing the
resulting path (e.g. resolving references to /./ and /../).


(In reply to comment #44)
> As far as I can tell, _st_theme_resolve_url does not actually translate to an
> absolute path. If we have url(../one/two.png);, it will lookup the stylesheet
> URL, get the basedir of that, and then use g_build_filename on that, which
> doesn't do any sort of resolution:
> 
> >>> GLib.build_filenamev(['directory/containing/a/stylesheet', '../one/two.png'])
> 'directory/containing/a/stylesheet/../one/two.png'

Which is an absolute path (e.g. it's actually /directory/containing ...) :-)

But yes, there's no canonicalization, so we miss updates in this case. To be honest, I don't think that'll be much of a problem in practice - images changing at runtime and references to . or .. in urls are both uncommon cases, more so the combination of the two. Not to mention that the effect will almost always be nothing but a bit less convenience for the developer ...

In any case, here's a patch to handle this anyway.
Comment 46 Jasper St. Pierre (not reading bugmail) 2012-10-17 20:49:37 UTC
Review of attachment 226688 [details] [review]:

::: src/st/st-theme.c
@@ +1019,3 @@
   char *dirname;
   char *filename;
+  char canonicalized_path[PATH_MAX];

See the BUGS section in realpath(3) for why this isn't acceptable.
Comment 47 Florian Müllner 2012-10-17 21:01:28 UTC
Created attachment 226690 [details] [review]
st: Canonicalize URLs in stylesheets

Ugh, the memory handling is getting so ugly that I begin to prefer not handling the case ...
Comment 48 Florian Müllner 2012-10-17 21:03:42 UTC
Comment on attachment 226679 [details] [review]
st: Fix handling of file:// URIs in _st_theme_resolve_uri()

Attachment 226679 [details] pushed as 0ea8217 - st: Fix handling of file:// URIs in _st_theme_resolve_uri()
Comment 49 Jasper St. Pierre (not reading bugmail) 2012-10-17 21:13:30 UTC
Review of attachment 226690 [details] [review]:

I was thinking we could just rely on the standard allocator being there in glib, since I don't know any program out there that changes the malloc tables of glib anymore.
Comment 50 Jasper St. Pierre (not reading bugmail) 2012-10-17 21:14:02 UTC
Review of attachment 226678 [details] [review]:

This makes sense to me.
Comment 51 Florian Müllner 2012-10-17 21:27:58 UTC
Created attachment 226695 [details] [review]
st: Canonicalize URLs in stylesheets

(In reply to comment #49)
> Review of attachment 226690 [details] [review]:
> 
> I was thinking we could just rely on the standard allocator being there in
> glib

*and* no one changing the standard allocator from malloc, so no, let's not do this unconditionally.
Comment 52 Jasper St. Pierre (not reading bugmail) 2012-10-17 21:43:30 UTC
(In reply to comment #51)
> Created an attachment (id=226695) [details] [review]
> st: Canonicalize URLs in stylesheets

I was thinking of this in terms of a code cleanup, but that's not what this is. Both patches are ACN from me, choose whichever one you prefer.

> *and* no one changing the standard allocator from malloc, so no, let's not do
> this unconditionally.

Do people do this, and how does it happen. Now that g_type_init is automatic, and glibc's allocator has gotten better, I wonder if it makes sense to deprecate the gmem (and possibly gslice) infrastructure entirely.
Comment 53 Florian Müllner 2012-10-17 21:58:58 UTC
Attachment 225098 [details] pushed as f18fd8d - userMenu: Rely on automatic texture changes
Attachment 225099 [details] pushed as d3ba002 - st: Remove unused methods
Attachment 226676 [details] pushed as 9c8b752 - st-texture-cache: Add texture-file-changed signal
Attachment 226678 [details] pushed as d54f7b1 - st-widget: Keep background-image and border-image updated
Attachment 226695 [details] pushed as 15273c7 - st: Canonicalize URLs in stylesheets



(In reply to comment #52)
> I was thinking of this in terms of a code cleanup, but that's not what this is.

That's why I chose to rely on certain libc implementation details originally :-)


> > *and* no one changing the standard allocator from malloc, so no, let's not do
> > this unconditionally.
> 
> Do people do this, and how does it happen.

I was referring to the hypothetical case where someone changes the allocator in glib itself, let's say 2 years from now - I can guarantee you that I won't remember that this breaks an old assumption we made and requires updating old code.