From 3dcd3729a0a2dd3765dcfd213ddfd71bebaaf2dc Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 17 Feb 2012 17:59:09 +0000 Subject: [PATCH] fix interlaced png read also remove old sinkmemory --- ChangeLog | 1 + TODO | 7 +- libvips/foreign/foreign.c | 2 +- libvips/foreign/vipspng.c | 18 +- libvips/iofuncs/Makefile.am | 1 - libvips/iofuncs/generate.c | 3 +- libvips/iofuncs/sinkmemory.c | 285 +++++++++++++++++++++++----- libvips/iofuncs/sinkmemory2.c | 345 ---------------------------------- 8 files changed, 256 insertions(+), 406 deletions(-) delete mode 100644 libvips/iofuncs/sinkmemory2.c diff --git a/ChangeLog b/ChangeLog index 59864f0b..b31cbc34 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,6 +6,7 @@ - sequential read mode - better im_shrink() - added vips_sequential() +- new vips_sink_memory() keeps read ordering 20/8/11 started 7.27.0 - version bump for new dev cycle diff --git a/TODO b/TODO index 8950bfaa..ce0eda66 100644 --- a/TODO +++ b/TODO @@ -1,12 +1,11 @@ -- sinkmemory needs something to stop some threads being horribly delayed - - - add a sequential mode to all readers tiff, jpg are the obvious ones - test progressive png + + + - argh diff --git a/libvips/foreign/foreign.c b/libvips/foreign/foreign.c index c2f651f8..cadc42ca 100644 --- a/libvips/foreign/foreign.c +++ b/libvips/foreign/foreign.c @@ -31,8 +31,8 @@ */ /* - */ #define DEBUG + */ #ifdef HAVE_CONFIG_H #include diff --git a/libvips/foreign/vipspng.c b/libvips/foreign/vipspng.c index 1cbab8b8..95727c12 100644 --- a/libvips/foreign/vipspng.c +++ b/libvips/foreign/vipspng.c @@ -65,8 +65,8 @@ */ /* - */ #define DEBUG + */ #ifdef HAVE_CONFIG_H #include @@ -157,6 +157,12 @@ read_new( const char *name, VipsImage *out ) if( !(read->pInfo = png_create_info_struct( read->pPng )) ) return( NULL ); + /* Read enough of the file that png_get_interlace_type() will start + * working. + */ + png_init_io( read->pPng, read->fp ); + png_read_info( read->pPng, read->pInfo ); + return( read ); } @@ -179,8 +185,6 @@ png2vips_header( Read *read, VipsImage *out ) if( setjmp( png_jmpbuf( read->pPng ) ) ) return( -1 ); - png_init_io( read->pPng, read->fp ); - png_read_info( read->pPng, read->pInfo ); png_get_IHDR( read->pPng, read->pInfo, &width, &height, &bit_depth, &color_type, &interlace_type, NULL, NULL ); @@ -324,8 +328,6 @@ png2vips_interlace( Read *read, VipsImage *out ) for( y = 0; y < out->Ysize; y++ ) read->row_pointer[y] = VIPS_IMAGE_ADDR( out, 0, y ); - png_set_interlace_handling( read->pPng ); - png_read_image( read->pPng, read->row_pointer ); return( 0 ); @@ -392,6 +394,7 @@ vips__png_read( const char *name, VipsImage *out ) vips_object_local_array( VIPS_OBJECT( out ), 3 ); Read *read; + int interlace_type; #ifdef DEBUG printf( "png2vips: reading \"%s\"\n", name ); @@ -400,8 +403,9 @@ vips__png_read( const char *name, VipsImage *out ) if( !(read = read_new( name, out )) ) return( -1 ); - if( png_get_interlace_type( read->pPng, read->pInfo ) != - PNG_INTERLACE_NONE ) { + interlace_type = png_get_interlace_type( read->pPng, read->pInfo ); + + if( interlace_type != PNG_INTERLACE_NONE ) { /* Arg awful interlaced image. We have to load to a huge mem * buffer, then copy to out. */ diff --git a/libvips/iofuncs/Makefile.am b/libvips/iofuncs/Makefile.am index d6c80258..6674a666 100644 --- a/libvips/iofuncs/Makefile.am +++ b/libvips/iofuncs/Makefile.am @@ -15,7 +15,6 @@ libiofuncs_la_SOURCES = \ sink.h \ sink.c \ sinkmemory.c \ - sinkmemory2.c \ sinkdisc.c \ sinkscreen.c \ memory.c \ diff --git a/libvips/iofuncs/generate.c b/libvips/iofuncs/generate.c index f978a59a..2a3e1d9e 100644 --- a/libvips/iofuncs/generate.c +++ b/libvips/iofuncs/generate.c @@ -666,8 +666,7 @@ vips_image_generate( VipsImage *image, res = vips_sink_disc( image, (VipsRegionWrite) write_vips, NULL ); else - // res = vips_sink_memory( image ); - res = vips_sink_memory2( image ); + res = vips_sink_memory( image ); /* Error? */ diff --git a/libvips/iofuncs/sinkmemory.c b/libvips/iofuncs/sinkmemory.c index f74f85de..018d5464 100644 --- a/libvips/iofuncs/sinkmemory.c +++ b/libvips/iofuncs/sinkmemory.c @@ -1,7 +1,11 @@ -/* Write an image to a memory buffer. +/* SinkMemory an image to a memory buffer, keeping top-to-bottom ordering. + * + * For sequential operations we need to keep requests reasonably ordered: we + * can't let some tiles get very delayed. So we need to stall starting new + * threads if the last thread gets too far behind. * - * 16/4/10 - * - from vips_sink() + * 17/2/12 + * - from sinkdisc.c */ /* @@ -31,8 +35,8 @@ */ /* - */ #define VIPS_DEBUG + */ #ifdef HAVE_CONFIG_H #include @@ -43,42 +47,253 @@ #include #include -#include #include +#include +#include #include #include "sink.h" +/* A part of the image we are writing. + */ +typedef struct _SinkMemoryArea { + struct _SinkMemory *memory; + + VipsRect rect; /* Part of image this area covers */ + VipsSemaphore nwrite; /* Number of threads writing to this area */ +} SinkMemoryArea; + /* Per-call state. */ typedef struct _SinkMemory { SinkBase sink_base; - /* A big region for the image memory. All the threads write to this. + /* We are current writing tiles to area, we'll delay starting a new + * area if old_area (the previous position) hasn't completed. */ - VipsRegion *all; + SinkMemoryArea *area; + SinkMemoryArea *old_area; + + /* A region covering the whole of the output image ... we write to + * this from many workers with vips_region_prepare_to(). + */ + VipsRegion *region; } SinkMemory; +/* Our per-thread state ... we need to also track the area that pos is + * supposed to write to. + */ +typedef struct _SinkMemoryThreadState { + VipsThreadState parent_object; + + SinkMemoryArea *area; +} SinkMemoryThreadState; + +typedef struct _SinkMemoryThreadStateClass { + VipsThreadStateClass parent_class; + +} SinkMemoryThreadStateClass; + +G_DEFINE_TYPE( SinkMemoryThreadState, + sink_memory_thread_state, VIPS_TYPE_THREAD_STATE ); + +static void +sink_memory_thread_state_class_init( SinkMemoryThreadStateClass *class ) +{ + VipsObjectClass *object_class = VIPS_OBJECT_CLASS( class ); + + object_class->nickname = "sinkmemorythreadstate"; + object_class->description = _( "per-thread state for sinkmemory" ); +} + +static void +sink_memory_thread_state_init( SinkMemoryThreadState *state ) +{ +} + +static VipsThreadState * +sink_memory_thread_state_new( VipsImage *image, void *a ) +{ + return( VIPS_THREAD_STATE( vips_object_new( + sink_memory_thread_state_get_type(), + vips_thread_state_set, image, a ) ) ); +} + +static void +sink_memory_area_free( SinkMemoryArea *area ) +{ + vips_semaphore_destroy( &area->nwrite ); + vips_free( area ); +} + +static SinkMemoryArea * +sink_memory_area_new( SinkMemory *memory ) +{ + SinkMemoryArea *area; + + if( !(area = VIPS_NEW( NULL, SinkMemoryArea )) ) + return( NULL ); + area->memory = memory; + vips_semaphore_init( &area->nwrite, 0, "nwrite" ); + + return( area ); +} + +/* Move an area to a position. + */ +static void +sink_memory_area_position( SinkMemoryArea *area, int top, int height ) +{ + SinkMemory *memory = area->memory; + + VipsRect all, rect; + + all.left = 0; + all.top = 0; + all.width = memory->sink_base.im->Xsize; + all.height = memory->sink_base.im->Ysize; + + rect.left = 0; + rect.top = top; + rect.width = memory->sink_base.im->Xsize; + rect.height = height; + + vips_rect_intersectrect( &all, &rect, &area->rect ); +} + +/* Our VipsThreadpoolAllocate function ... move the thread to the next tile + * that needs doing. If we fill the current area, we block until the previous + * area is finished, then swap areas. + * If all tiles are done, we return FALSE to end + * iteration. + */ +static gboolean +sink_memory_area_allocate_fn( VipsThreadState *state, void *a, gboolean *stop ) +{ + SinkMemoryThreadState *wstate = (SinkMemoryThreadState *) state; + SinkMemory *memory = (SinkMemory *) a; + SinkBase *sink_base = (SinkBase *) memory; + + VipsRect image; + VipsRect tile; + + VIPS_DEBUG_MSG( "sink_memory_area_allocate_fn:\n" ); + + /* Is the state x/y OK? New line or maybe new buffer or maybe even + * all done. + */ + if( sink_base->x >= memory->area->rect.width ) { + sink_base->x = 0; + sink_base->y += sink_base->tile_height; + + if( sink_base->y >= VIPS_RECT_BOTTOM( &memory->area->rect ) ) { + /* Block until the previous area is done. + */ + if( memory->area->rect.top > 0 ) + vips_semaphore_downn( + &memory->old_area->nwrite, 0 ); + + /* End of image? + */ + if( sink_base->y >= sink_base->im->Ysize ) { + *stop = TRUE; + return( 0 ); + } + + /* Swap buffers. + */ + VIPS_SWAP( SinkMemoryArea *, + memory->area, memory->old_area ); + + /* Position buf at the new y. + */ + sink_memory_area_position( memory->area, + sink_base->y, sink_base->nlines ); + } + } + + /* x, y and buf are good: save params for thread. + */ + image.left = 0; + image.top = 0; + image.width = sink_base->im->Xsize; + image.height = sink_base->im->Ysize; + tile.left = sink_base->x; + tile.top = sink_base->y; + tile.width = sink_base->tile_width; + tile.height = sink_base->tile_height; + vips_rect_intersectrect( &image, &tile, &state->pos ); + + /* The thread needs to know which area it's writing to. + */ + wstate->area = memory->area; + + VIPS_DEBUG_MSG( " allocated %d x %d:\n", tile.left, tile.top ); + + /* Add to the number of writers on the area. + */ + vips_semaphore_upn( &memory->area->nwrite, -1 ); + + /* Move state on. + */ + sink_base->x += sink_base->tile_width; + + return( 0 ); +} + +/* Our VipsThreadpoolWork function ... generate a tile! + */ +static int +sink_memory_area_work_fn( VipsThreadState *state, void *a ) +{ + SinkMemory *memory = (SinkMemory *) a; + SinkMemoryThreadState *wstate = (SinkMemoryThreadState *) state; + SinkMemoryArea *area = wstate->area; + + VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p %d x %d\n", + state, state->pos.left, state->pos.top ); + + if( vips_region_prepare_to( state->reg, memory->region, + &state->pos, state->pos.left, state->pos.top ) ) { + VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p error!\n", + state ); + return( -1 ); + } + VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p done\n", state ); + + /* Tell the allocator we're done. + */ + vips_semaphore_upn( &area->nwrite, 1 ); + + return( 0 ); +} + static void sink_memory_free( SinkMemory *memory ) { - VIPS_UNREF( memory->all ); + VIPS_FREEF( sink_memory_area_free, memory->area ); + VIPS_FREEF( sink_memory_area_free, memory->old_area ); + VIPS_UNREF( memory->region ); } static int -sink_memory_init( SinkMemory *memory, VipsImage *image ) +sink_memory_init( SinkMemory *memory, VipsImage *image ) { VipsRect all; vips_sink_base_init( &memory->sink_base, image ); + memory->area = NULL; + memory->old_area = NULL; all.left = 0; all.top = 0; all.width = image->Xsize; all.height = image->Ysize; - if( !(memory->all = vips_region_new( image )) || - vips_region_image( memory->all, &all ) ) { + if( !(memory->region = vips_region_new( image )) || + vips_region_image( memory->region, &all ) || + !(memory->area = sink_memory_area_new( memory )) || + !(memory->old_area = sink_memory_area_new( memory )) ) { sink_memory_free( memory ); return( -1 ); } @@ -86,28 +301,8 @@ sink_memory_init( SinkMemory *memory, VipsImage *image ) return( 0 ); } -static int -sink_memory_work( VipsThreadState *state, void *a ) -{ - SinkMemory *memory = (SinkMemory *) a; - - VIPS_DEBUG_MSG( "sink_memory_work: %p " - "left = %d, top = %d, width = %d, height = %d\n", - memory, - state->pos.left, - state->pos.top, - state->pos.width, - state->pos.height ); - - if( vips_region_prepare_to( state->reg, memory->all, - &state->pos, state->pos.left, state->pos.top ) ) - return( -1 ); - - return( 0 ); -} - /** - * vips_sink_memory: + * vips_sink_memory2: * @im: generate this image to memory * * Loops over an image, generating it to a memory buffer attached to the @@ -118,35 +313,33 @@ sink_memory_work( VipsThreadState *state, void *a ) * Returns: 0 on success, or -1 on error. */ int -vips_sink_memory( VipsImage *image ) +vips_sink_memory2( VipsImage *image ) { SinkMemory memory; int result; - VIPS_DEBUG_MSG( "vips_sink_memory: %p\n", image ); + VIPS_DEBUG_MSG( "vips_sink_memory2:\n" ); - g_assert( vips_object_sanity( VIPS_OBJECT( image ) ) ); - - /* We don't use this, but make sure it's set in case any old binaries - * are expecting it. - */ - image->Bbits = vips_format_sizeof( image->BandFmt ) << 3; - if( sink_memory_init( &memory, image ) ) return( -1 ); vips_image_preeval( image ); - result = vips_threadpool_run( image, - vips_thread_state_new, - vips_sink_base_allocate, - sink_memory_work, + result = 0; + sink_memory_area_position( memory.area, 0, memory.sink_base.nlines ); + if( vips_threadpool_run( image, + sink_memory_thread_state_new, + sink_memory_area_allocate_fn, + sink_memory_area_work_fn, vips_sink_base_progress, - &memory ); + &memory ) ) + result = -1; vips_image_posteval( image ); sink_memory_free( &memory ); + VIPS_DEBUG_MSG( "vips_sink_memory2: done\n" ); + return( result ); } diff --git a/libvips/iofuncs/sinkmemory2.c b/libvips/iofuncs/sinkmemory2.c deleted file mode 100644 index 40fbaa7b..00000000 --- a/libvips/iofuncs/sinkmemory2.c +++ /dev/null @@ -1,345 +0,0 @@ -/* SinkMemory an image to a memory buffer, keeping top-to-bottom ordering. - * - * For sequential operations we need to keep requests reasonably ordered: we - * can't let some tiles get very delayed. So we need to stall starting new - * threads if the last thread gets too far behind. - * - * 17/2/12 - * - from sinkdisc.c - */ - -/* - - This file is part of VIPS. - - VIPS is free software; you can redistribute it and/or modify - it under the terms of the GNU Lesser General Public License as published by - the Free Software Foundation; either version 2 of the License, or - (at your option) any later version. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public License - along with this program; if not, write to the Free Software - Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - - */ - -/* - - These files are distributed with VIPS - http://www.vips.ecs.soton.ac.uk - - */ - -/* - */ -#define VIPS_DEBUG - -#ifdef HAVE_CONFIG_H -#include -#endif /*HAVE_CONFIG_H*/ -#include - -#include -#include - -#include -#include -#include -#include -#include - -#include "sink.h" - -/* A part of the image we are writing. - */ -typedef struct _SinkMemoryArea { - struct _SinkMemory *memory; - - VipsRect rect; /* Part of image this area covers */ - VipsSemaphore nwrite; /* Number of threads writing to this area */ -} SinkMemoryArea; - -/* Per-call state. - */ -typedef struct _SinkMemory { - SinkBase sink_base; - - /* We are current writing tiles to area, we'll delay starting a new - * area if old_area (the previous position) hasn't completed. - */ - SinkMemoryArea *area; - SinkMemoryArea *old_area; - - /* A region covering the whole of the output image ... we write to - * this from many workers with vips_region_prepare_to(). - */ - VipsRegion *region; -} SinkMemory; - -/* Our per-thread state ... we need to also track the area that pos is - * supposed to write to. - */ -typedef struct _SinkMemoryThreadState { - VipsThreadState parent_object; - - SinkMemoryArea *area; -} SinkMemoryThreadState; - -typedef struct _SinkMemoryThreadStateClass { - VipsThreadStateClass parent_class; - -} SinkMemoryThreadStateClass; - -G_DEFINE_TYPE( SinkMemoryThreadState, - sink_memory_thread_state, VIPS_TYPE_THREAD_STATE ); - -static void -sink_memory_thread_state_class_init( SinkMemoryThreadStateClass *class ) -{ - VipsObjectClass *object_class = VIPS_OBJECT_CLASS( class ); - - object_class->nickname = "sinkmemorythreadstate"; - object_class->description = _( "per-thread state for sinkmemory" ); -} - -static void -sink_memory_thread_state_init( SinkMemoryThreadState *state ) -{ -} - -static VipsThreadState * -sink_memory_thread_state_new( VipsImage *image, void *a ) -{ - return( VIPS_THREAD_STATE( vips_object_new( - sink_memory_thread_state_get_type(), - vips_thread_state_set, image, a ) ) ); -} - -static void -sink_memory_area_free( SinkMemoryArea *area ) -{ - vips_semaphore_destroy( &area->nwrite ); - vips_free( area ); -} - -static SinkMemoryArea * -sink_memory_area_new( SinkMemory *memory ) -{ - SinkMemoryArea *area; - - if( !(area = VIPS_NEW( NULL, SinkMemoryArea )) ) - return( NULL ); - area->memory = memory; - vips_semaphore_init( &area->nwrite, 0, "nwrite" ); - - return( area ); -} - -/* Move an area to a position. - */ -static void -sink_memory_area_position( SinkMemoryArea *area, int top, int height ) -{ - SinkMemory *memory = area->memory; - - VipsRect all, rect; - - all.left = 0; - all.top = 0; - all.width = memory->sink_base.im->Xsize; - all.height = memory->sink_base.im->Ysize; - - rect.left = 0; - rect.top = top; - rect.width = memory->sink_base.im->Xsize; - rect.height = height; - - vips_rect_intersectrect( &all, &rect, &area->rect ); -} - -/* Our VipsThreadpoolAllocate function ... move the thread to the next tile - * that needs doing. If we fill the current area, we block until the previous - * area is finished, then swap areas. - * If all tiles are done, we return FALSE to end - * iteration. - */ -static gboolean -sink_memory_area_allocate_fn( VipsThreadState *state, void *a, gboolean *stop ) -{ - SinkMemoryThreadState *wstate = (SinkMemoryThreadState *) state; - SinkMemory *memory = (SinkMemory *) a; - SinkBase *sink_base = (SinkBase *) memory; - - VipsRect image; - VipsRect tile; - - VIPS_DEBUG_MSG( "sink_memory_area_allocate_fn:\n" ); - - /* Is the state x/y OK? New line or maybe new buffer or maybe even - * all done. - */ - if( sink_base->x >= memory->area->rect.width ) { - sink_base->x = 0; - sink_base->y += sink_base->tile_height; - - if( sink_base->y >= VIPS_RECT_BOTTOM( &memory->area->rect ) ) { - /* Block until the previous area is done. - */ - if( memory->area->rect.top > 0 ) - vips_semaphore_downn( - &memory->old_area->nwrite, 0 ); - - /* End of image? - */ - if( sink_base->y >= sink_base->im->Ysize ) { - *stop = TRUE; - return( 0 ); - } - - /* Swap buffers. - */ - VIPS_SWAP( SinkMemoryArea *, - memory->area, memory->old_area ); - - /* Position buf at the new y. - */ - sink_memory_area_position( memory->area, - sink_base->y, sink_base->nlines ); - } - } - - /* x, y and buf are good: save params for thread. - */ - image.left = 0; - image.top = 0; - image.width = sink_base->im->Xsize; - image.height = sink_base->im->Ysize; - tile.left = sink_base->x; - tile.top = sink_base->y; - tile.width = sink_base->tile_width; - tile.height = sink_base->tile_height; - vips_rect_intersectrect( &image, &tile, &state->pos ); - - /* The thread needs to know which area it's writing to. - */ - wstate->area = memory->area; - - VIPS_DEBUG_MSG( " allocated %d x %d:\n", tile.left, tile.top ); - - /* Add to the number of writers on the area. - */ - vips_semaphore_upn( &memory->area->nwrite, -1 ); - - /* Move state on. - */ - sink_base->x += sink_base->tile_width; - - return( 0 ); -} - -/* Our VipsThreadpoolWork function ... generate a tile! - */ -static int -sink_memory_area_work_fn( VipsThreadState *state, void *a ) -{ - SinkMemory *memory = (SinkMemory *) a; - SinkMemoryThreadState *wstate = (SinkMemoryThreadState *) state; - SinkMemoryArea *area = wstate->area; - - VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p %d x %d\n", - state, state->pos.left, state->pos.top ); - - if( vips_region_prepare_to( state->reg, memory->region, - &state->pos, state->pos.left, state->pos.top ) ) { - VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p error!\n", - state ); - return( -1 ); - } - VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p done\n", state ); - - /* Tell the allocator we're done. - */ - vips_semaphore_upn( &area->nwrite, 1 ); - - return( 0 ); -} - -static void -sink_memory_free( SinkMemory *memory ) -{ - VIPS_FREEF( sink_memory_area_free, memory->area ); - VIPS_FREEF( sink_memory_area_free, memory->old_area ); - VIPS_UNREF( memory->region ); -} - -static int -sink_memory_init( SinkMemory *memory, VipsImage *image ) -{ - VipsRect all; - - vips_sink_base_init( &memory->sink_base, image ); - memory->area = NULL; - memory->old_area = NULL; - - all.left = 0; - all.top = 0; - all.width = image->Xsize; - all.height = image->Ysize; - - if( !(memory->region = vips_region_new( image )) || - vips_region_image( memory->region, &all ) || - !(memory->area = sink_memory_area_new( memory )) || - !(memory->old_area = sink_memory_area_new( memory )) ) { - sink_memory_free( memory ); - return( -1 ); - } - - return( 0 ); -} - -/** - * vips_sink_memory2: - * @im: generate this image to memory - * - * Loops over an image, generating it to a memory buffer attached to the - * image. - * - * See also: vips_sink(), vips_get_tile_size(). - * - * Returns: 0 on success, or -1 on error. - */ -int -vips_sink_memory2( VipsImage *image ) -{ - SinkMemory memory; - int result; - - VIPS_DEBUG_MSG( "vips_sink_memory2:\n" ); - - if( sink_memory_init( &memory, image ) ) - return( -1 ); - - vips_image_preeval( image ); - - result = 0; - sink_memory_area_position( memory.area, 0, memory.sink_base.nlines ); - if( vips_threadpool_run( image, - sink_memory_thread_state_new, - sink_memory_area_allocate_fn, - sink_memory_area_work_fn, - vips_sink_base_progress, - &memory ) ) - result = -1; - - vips_image_posteval( image ); - - sink_memory_free( &memory ); - - VIPS_DEBUG_MSG( "vips_sink_memory2: done\n" ); - - return( result ); -}