From 926b2440057d70e48be4ce4533aa72335d067e90 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sat, 22 Mar 2025 12:13:17 +0000 Subject: [PATCH] add private threadpools for thumbnail rendering In the old model, there was a single rendering thread that moved between the active sink_screen tasks by priority. If a thumbnail and a main image window both needed updating, the thumbnail render (which can take a long time since it is effectively single-threaded) could block all high-priority updates. This PR adds private threadpools for low priority vips_sink_screen() rendering tasks. Now all thumbnails will update in the background as single-threaded tasks, and will not block foreground updates. --- ChangeLog | 1 + libvips/iofuncs/sinkscreen.c | 171 ++++++++++++++++++++++++++--------- 2 files changed, 130 insertions(+), 42 deletions(-) diff --git a/ChangeLog b/ChangeLog index c2bc1a32eb..3d7bb0e1c0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,6 +14,7 @@ - heifsave: improve alpha channel detection [lovell] - convi: ensure double sum precision for floats [lovell] - improve guard against corrupt ICC profiles with older lcms2 versions [kleisauke] +- improve vips_sink_screen() thumbnail rendering 8.16.1 diff --git a/libvips/iofuncs/sinkscreen.c b/libvips/iofuncs/sinkscreen.c index fd5c9d43f2..f8873ab2f6 100644 --- a/libvips/iofuncs/sinkscreen.c +++ b/libvips/iofuncs/sinkscreen.c @@ -125,6 +125,14 @@ typedef struct _Render { VipsSinkNotify notify; /* Tell caller about paints here */ void *a; + /* This render has it's own threadpool and is not on the shared list. + * + * This private threadpool needs a semaphore to wait on for dirty tiles + * to arrive. + */ + gboolean private_threadpool; + VipsSemaphore dirty_sem; + /* Lock here before reading or modifying the tile structure. */ GMutex lock; @@ -246,6 +254,9 @@ render_free(Render *render) #endif g_mutex_clear(&render->lock); + if (render->private_threadpool) + vips_semaphore_destroy(&render->dirty_sem); + vips_slist_map2(render->all, (VipsSListMap2Fn) tile_free, NULL, NULL); VIPS_FREEF(g_slist_free, render->all); render->ntiles = 0; @@ -384,6 +395,7 @@ render_allocate(VipsThreadState *state, void *a, gboolean *stop) { Render *render = (Render *) a; RenderThreadState *rstate = (RenderThreadState *) state; + Tile *tile; g_mutex_lock(&render->lock); @@ -468,22 +480,28 @@ render_dirty_sort(Render *a, Render *b, void *user_data) static void render_dirty_put(Render *render) { - g_mutex_lock(&render_dirty_lock); - - if (render->dirty) { - if (!g_slist_find(render_dirty_all, render)) { - render_dirty_all = g_slist_prepend(render_dirty_all, render); - render_dirty_all = g_slist_sort(render_dirty_all, - (GCompareFunc) render_dirty_sort); + if (render->private_threadpool) + // set our private renderer going + vips_semaphore_up(&render->dirty_sem); + else { + // add to the worklist for thwe global renderer + g_mutex_lock(&render_dirty_lock); - /* Tell the bg render thread we have one more dirty - * render on there. - */ - vips_semaphore_up(&n_render_dirty_sem); + if (render->dirty) { + if (!g_slist_find(render_dirty_all, render)) { + render_dirty_all = g_slist_prepend(render_dirty_all, render); + render_dirty_all = g_slist_sort(render_dirty_all, + (GCompareFunc) render_dirty_sort); + + /* Tell the bg render thread we have one more dirty + * render on there. + */ + vips_semaphore_up(&n_render_dirty_sem); + } } - } - g_mutex_unlock(&render_dirty_lock); + g_mutex_unlock(&render_dirty_lock); + } } static guint @@ -519,13 +537,21 @@ render_close_cb(VipsImage *image, Render *render) */ render->shutdown = TRUE; - render_unref(render); + if (render->private_threadpool) { + /* Nudge the bg thread (if any) for this pool. + */ + VIPS_DEBUG_MSG_GREEN("render_close_cb: nudge private worker\n"); + vips_semaphore_up(&render->dirty_sem); + } + else { + /* If this render is being worked on, we want to jog the bg thread, + * make it drop it's ref and think again. + */ + VIPS_DEBUG_MSG_GREEN("render_close_cb: reschedule\n"); + render_reschedule = TRUE; + } - /* If this render is being worked on, we want to jog the bg thread, - * make it drop it's ref and think again. - */ - VIPS_DEBUG_MSG_GREEN("render_close_cb: reschedule\n"); - render_reschedule = TRUE; + render_unref(render); } static Render * @@ -564,6 +590,11 @@ render_new(VipsImage *in, VipsImage *out, VipsImage *mask, render->notify = notify; render->a = a; + if (render->priority < 0) { + render->private_threadpool = TRUE; + vips_semaphore_init(&render->dirty_sem, 0, "dirty_sem"); + } + g_mutex_init(&render->lock); render->all = NULL; @@ -1025,6 +1056,58 @@ render_thread_main(void *client) return NULL; } +static int +render_allocate_private(VipsThreadState *state, void *a, gboolean *stop) +{ + Render *render = (Render *) a; + RenderThreadState *rstate = (RenderThreadState *) state; + + /* Wait for a dirty tile to arrive on this render. + */ + vips_semaphore_down(&render->dirty_sem); + + /* The mask or image is closing, we must exit. + */ + if (render->shutdown) { + *stop = TRUE; + rstate->tile = NULL; + return 0; + } + + /* Get the dirty tile, if any. + */ + g_mutex_lock(&render->lock); + rstate->tile = render_tile_dirty_get(render); + VIPS_DEBUG_MSG_AMBER("render_allocate_private: allocated tile %p\n", + rstate->tile); + g_mutex_unlock(&render->lock); + + return 0; +} + +/* A worker for a private render. + */ +static void +render_work_private(void *data, void *null) +{ + VIPS_DEBUG_MSG_AMBER("render_work_private: start\n"); + + Render *render = (Render *) data; + + // this will quit on ->shutdown == TRUE + if (vips_threadpool_run(render->in, + render_thread_state_new, + render_allocate_private, + render_work, + NULL, + render)) + VIPS_DEBUG_MSG_RED("render_work_private: threadpool_run failed\n"); + + render_unref(render); + + VIPS_DEBUG_MSG_AMBER("render_work_private: stop\n"); +} + static void * vips__sink_screen_once(void *data) { @@ -1052,34 +1135,33 @@ vips__sink_screen_once(void *data) * @notify_fn: (scope call) (nullable): pixels are ready notification callback * @a: (closure notify_fn) (nullable): client data for callback * - * This operation renders @in in the background, making pixels available on - * @out as they are calculated. The @notify_fn callback is run every time a new - * set of pixels are available. Calculated pixels are kept in a cache with - * tiles sized @tile_width by @tile_height pixels and with at most @max_tiles - * tiles. - * If @max_tiles is -1, the cache is of unlimited size (up to the maximum image - * size). - * The @mask image is a one-band uchar image and has 255 for pixels which are - * currently in cache and 0 for uncalculated pixels. + * This operation renders @in in the background, making pixels available + * on @out as they are calculated. The @notify_fn callback is run every + * time a new set of pixels are available. Calculated pixels are kept in + * a cache with tiles sized @tile_width by @tile_height pixels and with at + * most @max_tiles tiles. If @max_tiles is -1, the cache is of unlimited + * size (up to the maximum image * size). The @mask image is a one-band + * uchar image and has 255 for pixels which are currently in cache and 0 + * for uncalculated pixels. + * + * Renders with a positive priority are assumed to be large, gh-priority, + * foreground images. Although there can be many of these, only one is ever + * active, to avoid overcommitting threads. * - * Only a single sink is calculated at any one time, though many may be - * alive. Use @priority to indicate which renders are more important: - * zero means normal - * priority, negative numbers are low priority, positive numbers high - * priority. + * Renders with a negative priority are assumed to be small, thumbnail images + * consisting of a single tile. Single tile images are effectively + * single-threaded, so all these renders are evaluated together. * * Calls to vips_region_prepare() on @out return immediately and hold - * whatever is - * currently in cache for that #VipsRect (check @mask to see which parts of the - * #VipsRect are valid). Any pixels in the #VipsRect which are not in - * cache are added - * to a queue, and the @notify_fn callback will trigger when those pixels are - * ready. + * whatever is currently in cache for that #VipsRect (check @mask to see + * which parts of the #VipsRect are valid). Any pixels in the #VipsRect + * which are not in cache are added to a queue, and the @notify_fn + * callback will trigger when those pixels are ready. * * The @notify_fn callback is run from one of the background threads. In the - * callback - * you need to somehow send a message to the main thread that the pixels are - * ready. In a glib-based application, this is easily done with g_idle_add(). + * callback you need to somehow send a message to the main thread that the + * pixels are ready. In a glib-based application, this is easily done with + * g_idle_add(). * * If @notify_fn is %NULL then vips_sink_screen() runs synchronously. * vips_region_prepare() on @out will always block until the pixels have been @@ -1129,6 +1211,11 @@ vips_sink_screen(VipsImage *in, VipsImage *out, VipsImage *mask, VIPS_DEBUG_MSG("vips_sink_screen: max = %d, %p\n", max_tiles, render); + if (render->private_threadpool) { + render_ref(render); + vips_thread_execute("private threadpool", render_work_private, render); + } + if (vips_image_generate(out, vips_start_one, image_fill, vips_stop_one, in, render)) return -1;