add private threadpools for thumbnail rendering (#4430)

jcupitt · web-flow · commit 881c4ca61cde · 2025-03-26T10:21:57.000Z
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.
diff --git a/ChangeLog b/ChangeLog
@@ -17,9 +17,9 @@ master
 - 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
 - heifload: improve detection of seek beyond EOF [lovell]
 - add "oneshot" option to jp2kload [mbklein]
-- improved scheduling for vips_sink_screen()
 
 8.16.1
 
diff --git 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;
