From 0ee8b1e8445d695a2ec249ed53c270b756a46011 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sat, 3 Oct 2020 18:25:24 +0100 Subject: [PATCH] improve seek on pipes There were a few issues in VipsSource around seeking on pipes. With this patch, EOF detection is better, and pipe sources automatically turn into memory sources when EOF is hit. see https://github.com/libvips/libvips/issues/1829 --- ChangeLog | 1 + libvips/foreign/heifload.c | 36 +++++++++---- libvips/include/vips/connection.h | 4 +- libvips/iofuncs/source.c | 86 +++++++++++++++++++++---------- 4 files changed, 88 insertions(+), 39 deletions(-) diff --git a/ChangeLog b/ChangeLog index bb6df2f7..4089928e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,7 @@ - fix vips_fractsurf() typo [kleisauke] - better heif EOF detection [lovell] - fix gir build with g-o-i 1.66+ [László] +- improve seek behaviour on pipes 9/8/20 started 8.10.1 - fix markdown -> xml conversion in doc generation diff --git a/libvips/foreign/heifload.c b/libvips/foreign/heifload.c index 02122f73..08cdc68c 100644 --- a/libvips/foreign/heifload.c +++ b/libvips/foreign/heifload.c @@ -213,6 +213,13 @@ vips_foreign_load_heif_build( VipsObject *object ) { VipsForeignLoadHeif *heif = (VipsForeignLoadHeif *) object; +#ifdef DEBUG + printf( "vips_foreign_load_heif_build:\n" ); +#endif /*DEBUG*/ + + if( vips_source_rewind( heif->source ) ) + return( -1 ); + if( !heif->ctx ) { struct heif_error error; @@ -225,6 +232,7 @@ vips_foreign_load_heif_build( VipsObject *object ) } } + if( VIPS_OBJECT_CLASS( vips_foreign_load_heif_parent_class )-> build( object ) ) return( -1 ); @@ -281,7 +289,7 @@ vips_foreign_load_heif_get_flags( VipsForeignLoad *load ) return( VIPS_FOREIGN_SEQUENTIAL ); } -/* We've selcted the page. Try to select the associated thumbnail instead, +/* We've selected the page. Try to select the associated thumbnail instead, * if we can. */ static int @@ -295,6 +303,10 @@ vips_foreign_load_heif_set_thumbnail( VipsForeignLoadHeif *heif ) double main_aspect; double thumb_aspect; +#ifdef DEBUG + printf( "vips_foreign_load_heif_set_thumbnail:\n" ); +#endif /*DEBUG*/ + n_thumbs = heif_image_handle_get_list_of_thumbnail_IDs( heif->handle, thumb_ids, 1 ); if( n_thumbs == 0 ) @@ -353,16 +365,16 @@ static int vips_foreign_load_heif_set_page( VipsForeignLoadHeif *heif, int page_no, gboolean thumbnail ) { -#ifdef DEBUG - printf( "vips_foreign_load_heif_set_page: %d, thumbnail = %d\n", - page_no, thumbnail ); -#endif /*DEBUG*/ - if( !heif->handle || page_no != heif->page_no || thumbnail != heif->thumbnail_set ) { struct heif_error error; +#ifdef DEBUG + printf( "vips_foreign_load_heif_set_page: %d, thumbnail = %d\n", + page_no, thumbnail ); +#endif /*DEBUG*/ + VIPS_FREEF( heif_image_handle_release, heif->handle ); VIPS_FREEF( heif_image_release, heif->img ); heif->data = NULL; @@ -579,6 +591,10 @@ vips_foreign_load_heif_header( VipsForeignLoad *load ) heif_item_id primary_id; int i; +#ifdef DEBUG + printf( "vips_foreign_load_heif_header:\n" ); +#endif /*DEBUG*/ + heif->n_top = heif_context_get_number_of_top_level_images( heif->ctx ); heif->id = VIPS_ARRAY( NULL, heif->n_top, heif_item_id ); heif_context_get_list_of_top_level_image_IDs( heif->ctx, @@ -940,9 +956,9 @@ vips_foreign_load_heif_seek( gint64 position, void *userdata ) { VipsForeignLoadHeif *heif = (VipsForeignLoadHeif *) userdata; - vips_source_seek( heif->source, position, SEEK_SET ); - - return( 0 ); + /* Return 0 on success. + */ + return( vips_source_seek( heif->source, position, SEEK_SET ) == -1 ); } /* libheif calls this to mean "I intend to read() to this position, please @@ -1171,7 +1187,7 @@ vips_foreign_load_heif_source_build( VipsObject *object ) g_object_ref( heif->source ); } - if( VIPS_OBJECT_CLASS( vips_foreign_load_heif_file_parent_class )-> + if( VIPS_OBJECT_CLASS( vips_foreign_load_heif_source_parent_class )-> build( object ) ) return( -1 ); diff --git a/libvips/include/vips/connection.h b/libvips/include/vips/connection.h index 17a070c9..3f5ab5e3 100644 --- a/libvips/include/vips/connection.h +++ b/libvips/include/vips/connection.h @@ -159,7 +159,7 @@ typedef struct _VipsSource { * we rewind and try again, serve data from this until it runs out. * * If we need to force the whole pipe into memory, read everything to - * this and put a copy pf the pointer in data. + * this and put a copy of the pointer in data. */ GByteArray *header_bytes; @@ -171,7 +171,7 @@ typedef struct _VipsSource { */ VipsBlob *blob; - /* If we mmaped the file, whet we need to unmmap on finalize. + /* If we mmaped the file, what we need to unmmap on finalize. */ void *mmap_baseaddr; size_t mmap_length; diff --git a/libvips/iofuncs/source.c b/libvips/iofuncs/source.c index a8d59261..b6cbc36b 100644 --- a/libvips/iofuncs/source.c +++ b/libvips/iofuncs/source.c @@ -5,6 +5,8 @@ * * 3/2/20 * - add vips_pipe_read_limit_set() + * 3/10/20 + * - improve behaviour with read and seek on pipes */ /* @@ -40,8 +42,8 @@ */ /* -#define VIPS_DEBUG #define TEST_SANITY +#define VIPS_DEBUG */ #ifdef HAVE_CONFIG_H @@ -759,34 +761,45 @@ vips_source_read( VipsSource *source, void *buffer, size_t length ) return( total_read ); } -/* Read to a position. -1 means read to end of source. Does not change - * read_position. +#ifdef DEBUG +static void +vips_source_print( VipsSource *source ) +{ + printf( "vips_source_print: %p\n", source ); + printf( " source->read_position = %zd\n", source->read_position ); + printf( " source->is_pipe = %d\n", source->is_pipe ); + printf( " source->length = %zd\n", source->length ); + printf( " source->data = %p\n", source->data ); + printf( " source->header_bytes = %p\n", source->header_bytes ); + if( source->header_bytes ) + printf( " source->header_bytes->len = %d\n", + source->header_bytes->len ); + printf( " source->sniff = %p\n", source->sniff ); + if( source->sniff ) + printf( " source->sniff->len = %d\n", source->sniff->len ); +} +#endif /*DEBUG*/ + +/* Read to a position. + * + * target == -1 means read to end of source -- useful for forcing a pipe into + * memory, for example. + * + * If we hit EOF, set length on the pipe. + * + * read_position is left somewhere indeterminate. */ static int vips_source_pipe_read_to_position( VipsSource *source, gint64 target ) { const char *nick = vips_connection_nick( VIPS_CONNECTION( source ) ); - gint64 old_read_position; unsigned char buffer[4096]; - VIPS_DEBUG_MSG( "vips_source_pipe_read_position: %" G_GINT64_FORMAT - "\n", target ); - - g_assert( !source->decode ); - g_assert( source->header_bytes ); - g_assert( source->is_pipe ); - - if( target != -1 && - (target < 0 || - (source->length != -1 && - target > source->length)) ) { - vips_error( nick, - _( "bad read to %" G_GINT64_FORMAT ), target ); - return( -1 ); - } - - old_read_position = source->read_position; + /* This is only useful for pipes (sources where we don't know the + * length). + */ + g_assert( source->length == -1 ); while( target == -1 || source->read_position < target ) { @@ -795,8 +808,13 @@ vips_source_pipe_read_to_position( VipsSource *source, gint64 target ) bytes_read = vips_source_read( source, buffer, 4096 ); if( bytes_read == -1 ) return( -1 ); - if( bytes_read == 0 ) + + if( bytes_read == 0 ) { + /* No more bytes available, we must be at EOF. + */ + source->length = source->read_position; break; + } if( target == -1 && vips__pipe_read_limit != -1 && @@ -806,8 +824,6 @@ vips_source_pipe_read_to_position( VipsSource *source, gint64 target ) } } - source->read_position = old_read_position; - return( 0 ); } @@ -1126,9 +1142,25 @@ vips_source_seek( VipsSource *source, gint64 offset, int whence ) /* For pipes, we have to fake seek by reading to that point. */ - if( source->is_pipe && - vips_source_pipe_read_to_position( source, new_pos ) ) - return( -1 ); + if( source->is_pipe ) { + if( vips_source_pipe_read_to_position( source, new_pos ) ) + return( -1 ); + + /* We've hit EOF in the pipe, and we've got the whole thing + * buffered. Steal the header_bytes pointer and turn into a + * memory source. + */ + if( source->length != -1 && + source->header_bytes ) { + source->length = source->header_bytes->len; + source->data = source->header_bytes->data; + source->is_pipe = FALSE; + + /* TODO ... we could close more fds here. + */ + vips_source_minimise( source ); + } + } source->read_position = new_pos;