fixes to get paintbox working again

TODO

@@ -1,3 +1,12 @@
+- im_prepare() will clear invalidate (by calling im_region_buffer() before
+  running the pipeline), but im_prepare_to() will not
+
+  maybe always call im_region_buffer() on dest before we call im_prepare_to(),
+  this gives region a chance to drop the cache
+
+  but then we have to drop/take ownership on every write, argh
+
+  :n im_pre	
 
 
 

libvips/include/vips/region.h

@@ -83,6 +83,7 @@ int im_region_position( REGION *reg1, int x, int y );
 
 void im_region_paint( REGION *reg, Rect *r, int value );
 void im_region_black( REGION *reg );
+void im_region_copy( REGION *reg, REGION *dest, Rect *r, int x, int y );
 
 /* Macros on REGIONs.
  *	IM_REGION_LSKIP()		add to move down line

libvips/iofuncs/im_prepare.c

@@ -194,48 +194,6 @@ im_prepare( REGION *reg, Rect *r )
 	return( 0 );
 }
 
-/* Copy from one region to another. Copy area r from inside reg to dest,
- * positioning the area of pixels at x, y.
- */
-void
-im__copy_region( REGION *reg, REGION *dest, Rect *r, int x, int y )
-{
-	int z;
-	int len = IM_IMAGE_SIZEOF_PEL( reg->im ) * r->width;
-	char *p = IM_REGION_ADDR( reg, r->left, r->top );
-	char *q = IM_REGION_ADDR( dest, x, y );
-	int plsk = IM_REGION_LSKIP( reg );
-	int qlsk = IM_REGION_LSKIP( dest );
-
-#ifdef DEBUG
-	/* Find the area we will write to in dest.
-	 */
-	Rect output;
-
-	printf( "im__copy_region: sanity check\n" );
-
-	output.left = x;
-	output.top = y;
-	output.width = r->width;
-	output.height = r->height;
-
-	/* Must be inside dest->valid.
-	 */
-	g_assert( im_rect_includesrect( &dest->valid, &output ) );
-
-	/* Check the area we are reading from in reg.
-	 */
-	g_assert( im_rect_includesrect( &reg->valid, r ) );
-#endif /*DEBUG*/
-
-	for( z = 0; z < r->height; z++ ) {
-		memcpy( q, p, len );
-
-		p += plsk;
-		q += qlsk;
-	}
-}
-
 /* We need to make pixels using reg's generate function, and write the result
  * to dest.
  */
@@ -246,8 +204,7 @@ im_prepare_to_generate( REGION *reg, REGION *dest, Rect *r, int x, int y )
 	char *p;
 
 	if( !im->generate ) {
-		im_error( "im_prepare_to", 
+		im_error( "im_prepare_to", "%s", _( "incomplete header" ) );
-			"%s", _( "incomplete header" ) );
 		return( -1 );
 	}
 
@@ -268,7 +225,7 @@ im_prepare_to_generate( REGION *reg, REGION *dest, Rect *r, int x, int y )
 	 * we need an extra copy operation.
 	 */
 	if( IM_REGION_ADDR( reg, reg->valid.left, reg->valid.top ) != p )
-		im__copy_region( reg, dest, r, x, y );
+		im_region_copy( reg, dest, r, x, y );
 
 	return( 0 );
 }
@@ -286,7 +243,13 @@ im_prepare_to_generate( REGION *reg, REGION *dest, Rect *r, int x, int y )
  * result, we guarantee that we will fill the pixels in @dest at offset @x, @y.
  * In other words, we generate an extra copy operation if necessary. 
  *
- * See also: im_prepare().
+ * Also unlike im_prepare(), @dest is not set up for writing for you with
+ * im_region_buffer(). You can
+ * point @dest at anything, and pixels really will be written there. 
+ * This makes im_prepare_to() useful for making the ends of pipelines, since
+ * it (effectively) makes a break in the pipe.
+ *
+ * See also: im_prepare(), vips_sink_disc().
  *
  * Returns: 0 on success, or -1 on error
  */
@@ -305,7 +268,8 @@ im_prepare_to( REGION *reg, REGION *dest, Rect *r, int x, int y )
 
 	/* Sanity check.
 	 */
-	if( !dest->data || dest->im->BandFmt != reg->im->BandFmt ||
+	if( !dest->data || 
+		dest->im->BandFmt != reg->im->BandFmt ||
 		dest->im->Bands != reg->im->Bands ) {
 		im_error( "im_prepare_to", 
 			"%s", _( "inappropriate region type" ) );
@@ -333,8 +297,7 @@ im_prepare_to( REGION *reg, REGION *dest, Rect *r, int x, int y )
 	/* Test that dest->valid is large enough.
 	 */
 	if( !im_rect_includesrect( &dest->valid, &wanted ) ) {
-		im_error( "im_prepare_to", 
+		im_error( "im_prepare_to", "%s", _( "dest too small" ) );
-			"%s", _( "dest too small" ) );
 		return( -1 );
 	}
 
@@ -380,7 +343,7 @@ im_prepare_to( REGION *reg, REGION *dest, Rect *r, int x, int y )
 		 */
 		if( im_region_image( reg, &final ) )
 			return( -1 );
-		im__copy_region( reg, dest, &final, x, y );
+		im_region_copy( reg, dest, &final, x, y );
 
 		break;
 
@@ -396,7 +359,7 @@ im_prepare_to( REGION *reg, REGION *dest, Rect *r, int x, int y )
 		else {
 			if( im_region_image( reg, &final ) )
 				return( -1 );
-			im__copy_region( reg, dest, &final, x, y );
+			im_region_copy( reg, dest, &final, x, y );
 		}
 
 		break;
@@ -407,6 +370,14 @@ im_prepare_to( REGION *reg, REGION *dest, Rect *r, int x, int y )
 		return( -1 );
 	}
 
+	/* We've written fresh pixels to dest, it's no longer invalid (if it
+	 * was).
+	 *
+	 * We need this extra thing here because, unlike im_prepare(), we
+	 * don't im_region_buffer() dest before writing it.
+	 */
+	dest->invalid = FALSE;
+
 	return( 0 );
 }
 

libvips/iofuncs/region.c

@@ -836,3 +836,61 @@ im_region_black( REGION *reg )
 {
 	im_region_paint( reg, &reg->valid, 0 );
 }
+
+/**
+ * im_region_copy:
+ * @reg: source region 
+ * @dest: destination region 
+ * @r: #Rect of pixels you need to copy
+ * @x: postion of @r in @dest
+ * @y: postion of @r in @dest
+ *
+ * Copy from one region to another. Copy area @r from inside @reg to @dest,
+ * positioning the area of pixels at @x, @y. The two regions must have pixels
+ * which are the same size.
+ *
+ * See also: im_region_paint().
+ */
+void
+im_region_copy( REGION *reg, REGION *dest, Rect *r, int x, int y )
+{
+	int z;
+	int len = IM_IMAGE_SIZEOF_PEL( reg->im ) * r->width;
+	char *p = IM_REGION_ADDR( reg, r->left, r->top );
+	char *q = IM_REGION_ADDR( dest, x, y );
+	int plsk = IM_REGION_LSKIP( reg );
+	int qlsk = IM_REGION_LSKIP( dest );
+
+#ifdef DEBUG
+	/* Find the area we will write to in dest.
+	 */
+	Rect output;
+
+	printf( "im_region_copy: sanity check\n" );
+
+	output.left = x;
+	output.top = y;
+	output.width = r->width;
+	output.height = r->height;
+
+	/* Must be inside dest->valid.
+	 */
+	g_assert( im_rect_includesrect( &dest->valid, &output ) );
+
+	/* Check the area we are reading from in reg.
+	 */
+	g_assert( im_rect_includesrect( &reg->valid, r ) );
+
+	/* PEL size must be the same.
+	 */
+	g_assert( IM_IMAGE_SIZEOF_PEL( reg->im ) == 
+		IM_IMAGE_SIZEOF_PEL( dest->im ) );
+#endif /*DEBUG*/
+
+	for( z = 0; z < r->height; z++ ) {
+		memcpy( q, p, len );
+
+		p += plsk;
+		q += qlsk;
+	}
+}

libvips/iofuncs/sinkscreen.c

@@ -81,9 +81,18 @@ typedef struct {
 	struct _Render *render;
 
 	Rect area;		/* Place here (unclipped) */
-	gboolean painted;	/* Tile contains valid pixels (ie. not dirty) */
 	REGION *region;		/* REGION with the pixels */
 
+	/* The tile contains calculated pixels. Though the region may have been
+	 * invalidated behind our backs: we have to check that too.
+	 */
+	gboolean painted;
+
+	/* The tile is on the dirty list. This saves us having to search the
+	 * dirty list all the time.
+	 */
+	gboolean dirty;
+
 	int ticks;		/* Time of last use, for LRU flush */
 } Tile;
 
@@ -296,27 +305,100 @@ render_dirty_get( void )
 	return( render );
 }
 
+/* Get the next tile to paint off the dirty list.
+ */
+static Tile *
+render_tile_dirty_get( Render *render )
+{
+	Tile *tile;
+
+	if( !render->dirty )
+		tile = NULL;
+	else {
+		tile = (Tile *) render->dirty->data;
+		g_assert( tile->dirty );
+		render->dirty = g_slist_remove( render->dirty, tile );
+		tile->dirty = FALSE;
+	}
+
+	return( tile );
+}
+
+/* Pick a dirty tile to reuse. We could potentially get the tile that
+ * render_work() is working on in the background :-( but I don't think we'll
+ * get a crash, just a mis-paint. It should be vanishingly impossible anyway.
+ */
+static Tile *
+render_tile_dirty_reuse( Render *render )
+{
+	Tile *tile;
+
+	if( !render->dirty )
+		tile = NULL;
+	else {
+		tile = (Tile *) g_slist_last( render->dirty )->data;
+		render->dirty = g_slist_remove( render->dirty, tile );
+		g_assert( tile->dirty );
+		tile->dirty = FALSE;
+
+		VIPS_DEBUG_MSG( "render_tile_get_dirty_reuse: "
+			"reusing dirty %p\n", tile );
+	}
+
+	return( tile );
+}
+
+/* Add a tile to the dirty list.
+ */
+static void
+tile_dirty_set( Tile *tile )
+{
+	Render *render = tile->render;
+
+	if( !tile->dirty ) {
+		g_assert( !g_slist_find( render->dirty, tile ) );
+		render->dirty = g_slist_prepend( render->dirty, tile );
+		tile->dirty = TRUE;
+		tile->painted = FALSE;
+	}
+	else
+		g_assert( g_slist_find( render->dirty, tile ) );
+}
+
+/* Bump a tile to the front of the dirty list, if it's there.
+ */
+static void
+tile_dirty_bump( Tile *tile )
+{
+	Render *render = tile->render;
+
+	if( tile->dirty ) {
+		g_assert( g_slist_find( render->dirty, tile ) );
+
+		render->dirty = g_slist_remove( render->dirty, tile );
+		render->dirty = g_slist_prepend( render->dirty, tile );
+	}
+	else
+		g_assert( !g_slist_find( render->dirty, tile ) );
+}
+
 static int 
 render_allocate( VipsThreadState *state, void *a, gboolean *stop )
 {
 	Render *render = (Render *) a;
 	RenderThreadState *rstate = (RenderThreadState *) state;
+	Tile *tile;
 
 	g_mutex_lock( render->lock );
 
-	if( render_reschedule || !render->dirty ) {
+	if( render_reschedule || 
+		!(tile = render_tile_dirty_get( render )) ) {
 		VIPS_DEBUG_MSG_GREEN( "render_allocate: stopping\n" );
 		*stop = TRUE;
 		rstate->tile = NULL;
 	}
-	else {
+	else 
-		Tile *tile;
-
-		tile = (Tile *) render->dirty->data;
-		g_assert( !tile->painted );
-		render->dirty = g_slist_remove( render->dirty, tile );
 		rstate->tile = tile;
-	}
 
 	g_mutex_unlock( render->lock );
 
@@ -331,7 +413,6 @@ render_work( VipsThreadState *state, void *a )
 	Tile *tile = rstate->tile;
 
 	g_assert( tile );
-	g_assert( !tile->painted );
 
 	VIPS_DEBUG_MSG( "calculating tile %dx%d\n", 
 		tile->area.left, tile->area.top );
@@ -569,12 +650,13 @@ tile_new( Render *render )
 		return( NULL );
 
 	tile->render = render;
-	tile->region = NULL;
 	tile->area.left = 0;
 	tile->area.top = 0;
 	tile->area.width = render->tile_width;
 	tile->area.height = render->tile_height;
+	tile->region = NULL;
 	tile->painted = FALSE;
+	tile->dirty = FALSE;
 	tile->ticks = render->ticks;
 
 	if( !(tile->region = im_region_create( render->in )) ) {
@@ -605,12 +687,14 @@ render_tile_add( Tile *tile, Rect *area )
 
 	g_assert( !render_tile_lookup( render, area ) );
 
-	tile->painted = FALSE;
 	tile->area = *area;
-	/* Ignore buffer allocate errors, not much we could do with them.
+	tile->painted = FALSE;
+
+	/* Ignore buffer allocate errors, there's not much we could do with 
+	 * them.
 	 */
-	if( im_region_buffer( tile->region, area ) )
+	if( im_region_buffer( tile->region, &tile->area ) )
-		VIPS_DEBUG_MSG( "render_tile_add: buffer allocate failed\n" ); 
+		VIPS_DEBUG_MSG( "render_work: buffer allocate failed\n" ); 
 
 	g_hash_table_insert( render->tiles, &tile->area, tile );
 }
@@ -642,16 +726,7 @@ tile_touch( Tile *tile )
 
 	tile->ticks = render->ticks;
 	render->ticks += 1;
-
+	tile_dirty_bump( tile );
-	if( g_slist_find( render->dirty, tile ) ) {
-		g_assert( !tile->painted );
-
-		VIPS_DEBUG_MSG( "tile_bump_dirty: bumping tile %dx%d\n",
-			tile->area.left, tile->area.top );
-
-		render->dirty = g_slist_remove( render->dirty, tile );
-		render->dirty = g_slist_prepend( render->dirty, tile );
-	}
 }
 
 /* Queue a tile for calculation. 
@@ -664,13 +739,15 @@ tile_queue( Tile *tile )
 	VIPS_DEBUG_MSG( "tile_queue: adding tile %dx%d to dirty\n",
 		tile->area.left, tile->area.top );
 
+	tile->painted = FALSE;
+	tile_touch( tile );
+
 	if( render->notify && have_threads ) {
 		/* Add to the list of renders with dirty tiles. The bg 
-		 * thread will pick it up and paint it.
+		 * thread will pick it up and paint it. It can be already on
+		 * the dirty list.
 		 */
-		g_assert( !g_slist_find( render->dirty, tile ) );
+		tile_dirty_set( tile ); 
-		tile->painted = FALSE;
-		render->dirty = g_slist_prepend( render->dirty, tile );
 		render_dirty_put( render );
 	}
 	else {
@@ -707,8 +784,6 @@ render_tile_get_painted( Render *render )
 		(GHFunc) tile_test_clean_ticks, &tile );
 
 	if( tile ) {
-		g_assert( tile->painted );
-
 		VIPS_DEBUG_MSG( "render_tile_get_painted: "
 			"reusing painted %p\n", tile );
 	}
@@ -716,37 +791,15 @@ render_tile_get_painted( Render *render )
 	return( tile );
 }
 
-/* Pick a dirty tile to reuse. We could potentially get the tile that
- * render_work() is working on in the background :-( but I don't think we'll
- * get a crash, just a mis-paint. It should be vanishingly impossible anyway.
- */
-static Tile *
-render_tile_get_dirty( Render *render )
-{
-	Tile *tile;
-
-	if( !render->dirty )
-		tile = NULL;
-	else {
-		tile = (Tile *) g_slist_last( render->dirty )->data;
-		render->dirty = g_slist_remove( render->dirty, tile );
-
-		VIPS_DEBUG_MSG( "render_tile_get_dirty: "
-			"reusing dirty %p\n", tile );
-	}
-
-	return( tile );
-}
-
 /* Ask for an area of calculated pixels. Get from cache, request calculation,
  * or if we've no threads or no notify, calculate immediately.
  */
 static Tile *
-tile_request( Render *render, Rect *area )
+render_tile_request( Render *render, Rect *area )
 {
 	Tile *tile;
 
-	VIPS_DEBUG_MSG( "tile_request: asking for %dx%d\n",
+	VIPS_DEBUG_MSG( "render_tile_request: asking for %dx%d\n",
 		area->left, area->top );
 
 	if( (tile = render_tile_lookup( render, area )) ) {
@@ -755,6 +808,8 @@ tile_request( Render *render, Rect *area )
 		 */
 		if( tile->region->invalid ) 
 			tile_queue( tile );
+		else
+			tile_touch( tile );
 	}
 	else if( render->ntiles < render->max_tiles || 
 		render->max_tiles == -1 ) {
@@ -773,8 +828,9 @@ tile_request( Render *render, Rect *area )
 		 * then if that fails, reuse a dirty tile. 
 		 */
 		if( !(tile = render_tile_get_painted( render )) &&
-			!(tile = render_tile_get_dirty( render )) ) {
+			!(tile = render_tile_dirty_reuse( render )) ) {
-			VIPS_DEBUG_MSG( "tile_request: no tiles to reuse\n" );
+			VIPS_DEBUG_MSG( "render_tile_request: "
+				"no tiles to reuse\n" );
 			return( NULL );
 		}
 
@@ -783,8 +839,6 @@ tile_request( Render *render, Rect *area )
 		tile_queue( tile );
 	}
 
-	tile_touch( tile );
-
 	return( tile );
 }
 
@@ -863,7 +917,7 @@ region_fill( REGION *out, void *seq, void *a, void *b )
 			area.width = render->tile_width;
 			area.height = render->tile_height;
 
-			tile = tile_request( render, &area );
+			tile = render_tile_request( render, &area );
 			if( tile )
 				tile_copy( tile, out );
 		}