From 5d58e7ee66166b19e673c247fe41eae51f72fd92 Mon Sep 17 00:00:00 2001 From: Carl Worth Date: Mon, 5 Feb 2007 16:45:25 -0800 Subject: [PATCH] Add scaled_font->mutex to allow locking for all subordinate objects A cairo_scaled_font_t can be implicitly shared among multiple threads as the same cairo_scaled_font_t can be returned from different calls to cairo_scaled_font_create. To retain the illusion that these different calls produce distinct objects, cairo must internally lock access when modifying them. Each glyph in the scaled font is represented by a cairo_surface_t which is used when rendering the glyph. Instead of attempting to push fine-grained locking of these surfaces down to the backend rendering functions, a simple per-cairo_scaled_font_t lock has been introduced which protects the entire rendering path against re-entrancy. Some care was required to ensure that existing re-entrancy was handled appropriately; these cases are in the wrapping surfaces (cairo-paginated, test-meta and test-paginated). Thanks to Vladimir Vukicev and Peter Weilbacher for testing/providing the mutex definitions for win32 and os2 (respectively). --- src/cairo-paginated-surface.c | 21 +++++++++++-- src/cairo-scaled-font.c | 17 +++++++--- src/cairo-surface.c | 19 +++++++----- src/cairoint.h | 58 +++++++++++++++++++++++++++++++++-- src/test-meta-surface.c | 21 +++++++++++-- src/test-paginated-surface.c | 19 ++++++++++-- 6 files changed, 132 insertions(+), 23 deletions(-) diff --git a/src/cairo-paginated-surface.c b/src/cairo-paginated-surface.c index b313ac1f5..6c1ff6ab1 100644 --- a/src/cairo-paginated-surface.c +++ b/src/cairo-paginated-surface.c @@ -442,6 +442,7 @@ _cairo_paginated_surface_show_glyphs (void *abstract_surface, cairo_scaled_font_t *scaled_font) { cairo_paginated_surface_t *surface = abstract_surface; + cairo_int_status_t status; /* Optimize away erasing of nothing. */ if (surface->page_is_blank && op == CAIRO_OPERATOR_CLEAR) @@ -449,9 +450,23 @@ _cairo_paginated_surface_show_glyphs (void *abstract_surface, surface->page_is_blank = FALSE; - return _cairo_surface_show_glyphs (surface->meta, op, source, - glyphs, num_glyphs, - scaled_font); + /* Since this is a "wrapping" surface, we're calling back into + * _cairo_surface_show_glyphs from within a call to the same. + * Since _cairo_surface_show_glyphs acquires a mutex, we release + * and re-acquire the mutex around this nested call. + * + * Yes, this is ugly, but we consider it pragmatic as compared to + * adding locking code to all 18 surface-backend-specific + * show_glyphs functions, (which would get less testing and likely + * lead to bugs). + */ + CAIRO_MUTEX_UNLOCK (scaled_font->mutex); + status = _cairo_surface_show_glyphs (surface->meta, op, source, + glyphs, num_glyphs, + scaled_font); + CAIRO_MUTEX_LOCK (scaled_font->mutex); + + return status; } static cairo_surface_t * diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c index ba84316eb..cdabb6264 100755 --- a/src/cairo-scaled-font.c +++ b/src/cairo-scaled-font.c @@ -84,6 +84,7 @@ static const cairo_scaled_font_t _cairo_scaled_font_nil = { CAIRO_HINT_METRICS_DEFAULT} , { 1., 0., 0., 1., 0, 0}, /* scale */ { 0., 0., 0., 0., 0. }, /* extents */ + CAIRO_MUTEX_NIL_INITIALIZER,/* mutex */ NULL, /* glyphs */ NULL, /* surface_backend */ NULL, /* surface_private */ @@ -361,6 +362,7 @@ _cairo_scaled_font_init (cairo_scaled_font_t *scaled_font, &scaled_font->font_matrix, &scaled_font->ctm); + CAIRO_MUTEX_INIT (&scaled_font->mutex); scaled_font->glyphs = _cairo_cache_create (_cairo_scaled_glyph_keys_equal, _cairo_scaled_glyph_destroy, max_glyphs_cached_per_font); @@ -416,6 +418,8 @@ _cairo_scaled_font_fini (cairo_scaled_font_t *scaled_font) if (scaled_font->glyphs != NULL) _cairo_cache_destroy (scaled_font->glyphs); + CAIRO_MUTEX_FINI (&scaled_font->mutex); + if (scaled_font->surface_backend != NULL && scaled_font->surface_backend->scaled_font_fini != NULL) scaled_font->surface_backend->scaled_font_fini (scaled_font); @@ -710,6 +714,8 @@ cairo_scaled_font_glyph_extents (cairo_scaled_font_t *scaled_font, if (scaled_font->status) return; + CAIRO_MUTEX_LOCK (scaled_font->mutex); + for (i = 0; i < num_glyphs; i++) { double left, top, right, bottom; @@ -719,7 +725,7 @@ cairo_scaled_font_glyph_extents (cairo_scaled_font_t *scaled_font, &scaled_glyph); if (status) { _cairo_scaled_font_set_error (scaled_font, status); - return; + goto UNLOCK; } /* "Ink" extents should skip "invisible" glyphs */ @@ -773,6 +779,9 @@ cairo_scaled_font_glyph_extents (cairo_scaled_font_t *scaled_font, extents->x_advance = 0.0; extents->y_advance = 0.0; } + + UNLOCK: + CAIRO_MUTEX_UNLOCK (scaled_font->mutex); } slim_hidden_def (cairo_scaled_font_glyph_extents); @@ -1337,6 +1346,8 @@ _cairo_scaled_glyph_set_path (cairo_scaled_glyph_t *scaled_glyph, * If the desired info is not available, (for example, when trying to * get INFO_PATH with a bitmapped font), this function will return * CAIRO_INT_STATUS_UNSUPPORTED. + * + * NOTE: This function must be called with scaled_font->mutex held. **/ cairo_int_status_t _cairo_scaled_glyph_lookup (cairo_scaled_font_t *scaled_font, @@ -1352,8 +1363,6 @@ _cairo_scaled_glyph_lookup (cairo_scaled_font_t *scaled_font, if (scaled_font->status) return scaled_font->status; - CAIRO_MUTEX_LOCK (cairo_scaled_font_map_mutex); - key.hash = index; /* * Check cache for glyph @@ -1424,8 +1433,6 @@ _cairo_scaled_glyph_lookup (cairo_scaled_font_t *scaled_font, *scaled_glyph_ret = scaled_glyph; } - CAIRO_MUTEX_UNLOCK (cairo_scaled_font_map_mutex); - return status; } diff --git a/src/cairo-surface.c b/src/cairo-surface.c index 1541e6d9a..6f48a6857 100644 --- a/src/cairo-surface.c +++ b/src/cairo-surface.c @@ -1846,19 +1846,22 @@ _cairo_surface_show_glyphs (cairo_surface_t *surface, cairo_font_options_destroy (font_options); } - if (surface->backend->show_glyphs) { + CAIRO_MUTEX_LOCK (dev_scaled_font->mutex); + + status = CAIRO_INT_STATUS_UNSUPPORTED; + + if (surface->backend->show_glyphs) status = surface->backend->show_glyphs (surface, op, &dev_source.base, glyphs, num_glyphs, dev_scaled_font); - if (status != CAIRO_INT_STATUS_UNSUPPORTED) - goto FINISH; - } - status = _cairo_surface_fallback_show_glyphs (surface, op, &dev_source.base, - glyphs, num_glyphs, - dev_scaled_font); + if (status == CAIRO_INT_STATUS_UNSUPPORTED) + status = _cairo_surface_fallback_show_glyphs (surface, op, &dev_source.base, + glyphs, num_glyphs, + dev_scaled_font); + + CAIRO_MUTEX_UNLOCK (dev_scaled_font->mutex); -FINISH: if (dev_scaled_font != scaled_font) cairo_scaled_font_destroy (dev_scaled_font); diff --git a/src/cairoint.h b/src/cairoint.h index 060b98812..16f031663 100755 --- a/src/cairoint.h +++ b/src/cairoint.h @@ -137,9 +137,14 @@ CAIRO_BEGIN_DECLS #if HAVE_PTHREAD_H # include # define CAIRO_MUTEX_DECLARE(name) static pthread_mutex_t name = PTHREAD_MUTEX_INITIALIZER -#define CAIRO_MUTEX_DECLARE_GLOBAL(name) pthread_mutex_t name = PTHREAD_MUTEX_INITIALIZER -# define CAIRO_MUTEX_LOCK(name) pthread_mutex_lock (&name) +# define CAIRO_MUTEX_DECLARE_GLOBAL(name) pthread_mutex_t name = PTHREAD_MUTEX_INITIALIZER +# define CAIRO_MUTEX_LOCK(name) \ +do { pthread_mutex_lock (&name); CAIRO_LOCK_FILE = __FILE__; CAIRO_LOCK_LINE = __LINE__; } while (0) # define CAIRO_MUTEX_UNLOCK(name) pthread_mutex_unlock (&name) +typedef pthread_mutex_t cairo_mutex_t; +# define CAIRO_MUTEX_INIT(mutex) pthread_mutex_init ((mutex), NULL) +# define CAIRO_MUTEX_FINI(mutex) pthread_mutex_destroy (mutex) +# define CAIRO_MUTEX_NIL_INITIALIZER PTHREAD_MUTEX_INITIALIZER #endif #if !defined(CAIRO_MUTEX_DECLARE) && defined CAIRO_HAS_WIN32_SURFACE @@ -159,6 +164,10 @@ CAIRO_BEGIN_DECLS # define CAIRO_MUTEX_DECLARE_GLOBAL(name) extern LPCRITICAL_SECTION name; # define CAIRO_MUTEX_LOCK(name) EnterCriticalSection (&name) # define CAIRO_MUTEX_UNLOCK(name) LeaveCriticalSection (&name) +typedef CRITICAL_SECTION cairo_mutex_t; +# define CAIRO_MUTEX_INIT(mutex) InitializeCriticalSection (mutex) +# define CAIRO_MUTEX_FINI(mutex) DeleteCriticalSection (mutex) +# define CAIRO_MUTEX_NIL_INITIALIZER { 0 } #endif #if defined(__OS2__) && !defined(CAIRO_MUTEX_DECLARE) @@ -170,6 +179,10 @@ CAIRO_BEGIN_DECLS # define CAIRO_MUTEX_DECLARE_GLOBAL(name) extern HMTX name # define CAIRO_MUTEX_LOCK(name) DosRequestMutexSem(name, SEM_INDEFINITE_WAIT) # define CAIRO_MUTEX_UNLOCK(name) DosReleaseMutexSem(name) +typedef HMTX cairo_mutex_t; +# define CAIRO_MUTEX_INIT(mutex) DosCreateMutexSem (NULL, mutex, 0L, TRUE) +# define CAIRO_MUTEX_FINI(mutex) DosCloseMutexSem (*(mutex)) +# define CAIRO_MUTEX_NIL_INITIALIZER { 0 } #endif #if !defined(CAIRO_MUTEX_DECLARE) && defined CAIRO_HAS_BEOS_SURFACE @@ -180,6 +193,13 @@ cairo_private void _cairo_beos_unlock(void*); # define CAIRO_MUTEX_DECLARE_GLOBAL(name) extern void* name; # define CAIRO_MUTEX_LOCK(name) _cairo_beos_lock (&name) # define CAIRO_MUTEX_UNLOCK(name) _cairo_beos_unlock (&name) +# error "XXX: Someone who understands BeOS needs to add definitions for" \ + " cairo_mutex_t, CAIRO_MUTEX_INIT, and CAIRO_MUTEX_FINI," \ + " to cairoint.h" +typedef ??? cairo_mutex_t; +# define CAIRO_MUTEX_INIT(mutex) ??? +# define CAIRO_MUTEX_FINI(mutex) ??? +# define CAIRO_MUTEX_NIL_INITIALIZER {} #endif #ifndef CAIRO_MUTEX_DECLARE @@ -523,6 +543,36 @@ typedef struct _cairo_scaled_glyph { #define _cairo_scaled_glyph_set_index(g,i) ((g)->cache_entry.hash = (i)) struct _cairo_scaled_font { + /* For most cairo objects, the rule for multiple threads is that + * the user is responsible for any locking if the same object is + * manipulated from multiple threads simultaneously. + * + * However, with the caching that cairo does for scaled fonts, a + * user can easily end up with the same cairo_scaled_font object + * being manipulated from multiple threads without the user ever + * being aware of this, (and in fact, unable to control it). + * + * So, as a special exception, the cairo implementation takes care + * of all locking needed for cairo_scaled_font_t. Most of what is + * in the scaled font is immutable, (which is what allows for the + * sharing in the first place). The things that are modified and + * the locks protecting them are as follows: + * + * 1. The reference count (scaled_font->ref_count) + * + * Modifications to the reference count are protected by the + * cairo_scaled_font_map_mutex. This is because the reference + * count of a scaled font is intimately related with the font + * map itself, (and the magic holdovers array). + * + * 2. The cache of glyphs (scaled_font->glyphs) + * 3. The backend private data (scaled_font->surface_backend, + * scaled_font->surface_private) + * + * Modifications to these fields are protected with locks on + * scaled_font->mutex in the generic scaled_font code. + */ + /* must be first to be stored in a hash table */ cairo_hash_entry_t hash_entry; @@ -539,6 +589,10 @@ struct _cairo_scaled_font { /* "live" scaled_font members */ cairo_matrix_t scale; /* font space => device space */ cairo_font_extents_t extents; /* user space */ + + /* The mutex protects modification to all subsequent fields. */ + cairo_mutex_t mutex; + cairo_cache_t *glyphs; /* glyph index -> cairo_scaled_glyph_t */ /* diff --git a/src/test-meta-surface.c b/src/test-meta-surface.c index 7939b6e93..5fea76610 100644 --- a/src/test-meta-surface.c +++ b/src/test-meta-surface.c @@ -252,12 +252,27 @@ _test_meta_surface_show_glyphs (void *abstract_surface, cairo_scaled_font_t *scaled_font) { test_meta_surface_t *surface = abstract_surface; + cairo_int_status_t status; surface->image_reflects_meta = FALSE; - return _cairo_surface_show_glyphs (surface->meta, op, source, - glyphs, num_glyphs, - scaled_font); + /* Since this is a "wrapping" surface, we're calling back into + * _cairo_surface_show_glyphs from within a call to the same. + * Since _cairo_surface_show_glyphs acquires a mutex, we release + * and re-acquire the mutex around this nested call. + * + * Yes, this is ugly, but we consider it pragmatic as compared to + * adding locking code to all 18 surface-backend-specific + * show_glyphs functions, (which would get less testing and likely + * lead to bugs). + */ + CAIRO_MUTEX_UNLOCK (scaled_font->mutex); + status = _cairo_surface_show_glyphs (surface->meta, op, source, + glyphs, num_glyphs, + scaled_font); + CAIRO_MUTEX_LOCK (scaled_font->mutex); + + return status; } static cairo_surface_t * diff --git a/src/test-paginated-surface.c b/src/test-paginated-surface.c index 544efcf5a..1950052c9 100644 --- a/src/test-paginated-surface.c +++ b/src/test-paginated-surface.c @@ -233,12 +233,27 @@ _test_paginated_surface_show_glyphs (void *abstract_surface, cairo_scaled_font_t *scaled_font) { test_paginated_surface_t *surface = abstract_surface; + cairo_int_status_t status; if (surface->paginated_mode == CAIRO_PAGINATED_MODE_ANALYZE) return CAIRO_STATUS_SUCCESS; - return _cairo_surface_show_glyphs (surface->target, op, source, - glyphs, num_glyphs, scaled_font); + /* Since this is a "wrapping" surface, we're calling back into + * _cairo_surface_show_glyphs from within a call to the same. + * Since _cairo_surface_show_glyphs acquires a mutex, we release + * and re-acquire the mutex around this nested call. + * + * Yes, this is ugly, but we consider it pragmatic as compared to + * adding locking code to all 18 surface-backend-specific + * show_glyphs functions, (which would get less testing and likely + * lead to bugs). + */ + CAIRO_MUTEX_UNLOCK (scaled_font->mutex); + status = _cairo_surface_show_glyphs (surface->target, op, source, + glyphs, num_glyphs, scaled_font); + CAIRO_MUTEX_LOCK (scaled_font->mutex); + + return status; } static void