[cairo] glyph caching bug

[TOKUNAGA Hiroyuki]

Hi,

On Mon, 27 Dec 2004 01:46:31 -0800
Keith Packard <keithp at keithp.com> wrote:

> I've noticed a bug somewhere in the cairo glyph caching code -- it's
> using  the wrong glyphs at times, and most often with large glyphs.
> 
> Here's an example nickle program which reproduces the bug in my 
> environment.  It spins "CAIRO" around at a scale of 100; at some 
> rotations, various letters are mistakenly replaced with other letters
> or  blanks.  Very disconcerting; you'll note that the code takes care
> to  double buffer the results and should be completely smooth.
> 
> I haven't tried to debug this very far; it looks hard to find to me...

I could reproduced the bug. This seems because there's no cache lock
implementation. Sometimes created Glyph was destroyed before used, since
_cairo_cache_lookup destroys entries randomly.

Attached patch is a tentative implementation of cache locking. While a
cache is locked, lookup function doesn't call destroy_entry function. I
could confirm that the bug doesn't occur if this patch was applied. But
cache will consume much memory than restricted by cache->max_memory
untill unlock function is called. It may be a problem.


Regards,

-- 
TOKUNAGA Hiroyuki
tkng at xem.jp
http://kodou.net/
-------------- next part --------------
Index: cairo_cache.c
===================================================================
RCS file: /cvs/cairo/cairo/src/cairo_cache.c,v
retrieving revision 1.5
diff -u -r1.5 cairo_cache.c
--- cairo_cache.c	20 Dec 2004 17:43:59 -0000	1.5
+++ cairo_cache.c	28 Dec 2004 19:04:34 -0000
@@ -121,7 +121,8 @@
     assert (cache->entries != NULL);
     assert (cache->backend != NULL);
     assert (cache->arrangement != NULL);
-    assert (cache->used_memory <= cache->max_memory);
+    if(cache->locked == 0)
+       assert (cache->used_memory <= cache->max_memory);
     assert (cache->live_entries <= cache->arrangement->size);   
 }
 #else
@@ -409,16 +410,17 @@
     /* Store the hash value in case the backend forgot. */
     new_entry->hashcode = cache->backend->hash (cache, key);
 
-    /* Make some entries die if we're under memory pressure. */
-    while (cache->live_entries > 0 &&
-	   ((cache->max_memory - cache->used_memory) < new_entry->memory)) {
-	idx = _random_live_entry (cache);
-	assert (idx < cache->arrangement->size);
-	_entry_destroy (cache, idx);
+    if(cache->locked == 0) {
+       /* Make some entries die if we're under memory pressure. */
+       while (cache->locked == 0 && cache->live_entries > 0 &&
+	      ((cache->max_memory - cache->used_memory) < new_entry->memory)) {
+	 idx = _random_live_entry (cache);
+	 assert (idx < cache->arrangement->size);
+	 _entry_destroy (cache, idx);
+       }
+       assert(cache->max_memory >= (cache->used_memory + new_entry->memory));
     }
-    
-    assert(cache->max_memory >= (cache->used_memory + new_entry->memory));
-    
+   
     /* Make room in the table for a new slot. */
     status = _resize_cache (cache, cache->live_entries + 1);
     if (status != CAIRO_STATUS_SUCCESS) {
@@ -440,6 +442,35 @@
     return status;
 }
 
+/*
+ *  _cairo_cache_lookup may call destroy_entry function. This will be a problem
+ *  if you want to use multiple entries at the same time. (i.e. a created
+ *  entry may be destroyed before used.)
+ *  If this function is called, _cairo_cache_lookup doesn't call destroy_entry
+ *  function. You must call this function before multiple calls of
+ *  _cairo_cache_lookup.
+ */
+void
+_cairo_cache_lock (cairo_cache_t *cache)
+{
+    cache->locked = 1;
+}
+
+/* You must call _cairo_cache_unlock at the end of the function which uses
+   _cairo_cache_lock. */
+void
+_cairo_cache_unlock (cairo_cache_t *cache)
+{
+    cache->locked = 0;
+    /* Make some entries die if we're under memory pressure. */
+    while (cache->live_entries > 0 &&
+	   ((cache->max_memory - cache->used_memory) < 0)) {
+	  unsigned long idx = _random_live_entry (cache);
+	  assert (idx < cache->arrangement->size);
+	  _entry_destroy (cache, idx);
+    }
+}
+
 unsigned long
 _cairo_hash_string (const char *c)
 {
Index: cairo_xlib_surface.c
===================================================================
RCS file: /cvs/cairo/cairo/src/cairo_xlib_surface.c,v
retrieving revision 1.31
diff -u -r1.31 cairo_xlib_surface.c
--- cairo_xlib_surface.c	21 Dec 2004 21:14:46 -0000	1.31
+++ cairo_xlib_surface.c	28 Dec 2004 19:04:35 -0000
@@ -958,14 +958,22 @@
 
 static void
 _lock_xlib_glyphset_caches (void)
-{
-    /* FIXME: implement locking */
+{ 
+    cairo_cache_t *cache;
+    if(_xlib_glyphset_caches != NULL) {
+      cache = &(_xlib_glyphset_caches->base);
+      _cairo_cache_lock (cache);
+    }
 }
 
 static void
 _unlock_xlib_glyphset_caches (void)
 {
-    /* FIXME: implement locking */
+    cairo_cache_t *cache;
+    if(_xlib_glyphset_caches != NULL) {
+      cache = &(_xlib_glyphset_caches->base);
+      _cairo_cache_unlock (cache);
+    }
 }
 
 static glyphset_cache_t *
Index: cairoint.h
===================================================================
RCS file: /cvs/cairo/cairo/src/cairoint.h,v
retrieving revision 1.78
diff -u -r1.78 cairoint.h
--- cairoint.h	21 Dec 2004 21:22:44 -0000	1.78
+++ cairoint.h	28 Dec 2004 19:04:35 -0000
@@ -317,6 +317,7 @@
     unsigned long max_memory;
     unsigned long used_memory;
     unsigned long live_entries;
+    unsigned int  locked;
 
 #ifdef CAIRO_MEASURE_CACHE_PERFORMANCE
     unsigned long hits;
@@ -341,6 +342,12 @@
 		     void *key,
 		     void **entry_return);
 
+cairo_private void
+_cairo_cache_lock (cairo_cache_t *cache);
+
+cairo_private void
+_cairo_cache_unlock (cairo_cache_t *cache);
+
 cairo_private unsigned long
 _cairo_hash_string (const char *c);