From ed2054dbbcab5e31c816a229f509e086ab9b7157 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Wed, 21 Aug 2019 10:35:48 +0100 Subject: [PATCH] revise arithmetic with const there's a problem with out of bounds values, for example: vips relational_const k2.jpg x.v equal 1000 actually finds pixels == 255, since 1000 is saturated converted to 255 before the test starts. This patch reworks arithmetic against const values to fix this. --- ChangeLog | 2 + libvips/arithmetic/boolean.c | 15 ++-- libvips/arithmetic/math2.c | 5 +- libvips/arithmetic/relational.c | 82 ++++++++++++------- libvips/arithmetic/remainder.c | 8 +- libvips/arithmetic/unaryconst.c | 136 +++++++------------------------- libvips/arithmetic/unaryconst.h | 15 ++-- 7 files changed, 100 insertions(+), 163 deletions(-) diff --git a/ChangeLog b/ChangeLog index 871fc395..162ba56f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,8 @@ - loaders use "minimise" to close input files earlier - integrate support for oss-fuzz [omira-sch] - add vips_switch() / vips_case() ... fast many-way ifthenelse +- better const handling for arithmetic operators fixes comparisons against out + of range values 9/7/19 started 8.8.2 - better early shutdown in readers diff --git a/libvips/arithmetic/boolean.c b/libvips/arithmetic/boolean.c index 1df7dab3..c4fcf18d 100644 --- a/libvips/arithmetic/boolean.c +++ b/libvips/arithmetic/boolean.c @@ -458,14 +458,11 @@ vips_boolean_const_build( VipsObject *object ) { VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); VipsUnary *unary = (VipsUnary *) object; - VipsUnaryConst *uconst = (VipsUnaryConst *) object; if( unary->in && vips_check_noncomplex( class->nickname, unary->in ) ) return( -1 ); - uconst->const_format = VIPS_FORMAT_INT; - if( VIPS_OBJECT_CLASS( vips_boolean_const_parent_class )-> build( object ) ) return( -1 ); @@ -474,9 +471,9 @@ vips_boolean_const_build( VipsObject *object ) } #define LOOPC( TYPE, OP ) { \ - TYPE *p = (TYPE *) in[0]; \ - TYPE *q = (TYPE *) out; \ - int *c = (int *) uconst->c_ready; \ + TYPE * restrict p = (TYPE *) in[0]; \ + TYPE * restrict q = (TYPE *) out; \ + int * restrict c = uconst->c_int; \ \ for( i = 0, x = 0; x < width; x++ ) \ for( b = 0; b < bands; b++, i++ ) \ @@ -484,9 +481,9 @@ vips_boolean_const_build( VipsObject *object ) } #define FLOOPC( TYPE, OP ) { \ - TYPE *p = (TYPE *) in[0]; \ - int *q = (int *) out; \ - int *c = (int *) uconst->c_ready; \ + TYPE * restrict p = (TYPE *) in[0]; \ + int * restrict q = (int *) out; \ + int * restrict c = uconst->c_int; \ \ for( i = 0, x = 0; x < width; x++ ) \ for( b = 0; b < bands; b++, i++ ) \ diff --git a/libvips/arithmetic/math2.c b/libvips/arithmetic/math2.c index 35ed16d0..cbcd2b99 100644 --- a/libvips/arithmetic/math2.c +++ b/libvips/arithmetic/math2.c @@ -338,14 +338,11 @@ vips_math2_const_build( VipsObject *object ) { VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); VipsUnary *unary = (VipsUnary *) object; - VipsUnaryConst *uconst = (VipsUnaryConst *) object; if( unary->in && vips_check_noncomplex( class->nickname, unary->in ) ) return( -1 ); - uconst->const_format = VIPS_FORMAT_DOUBLE; - if( VIPS_OBJECT_CLASS( vips_math2_const_parent_class )-> build( object ) ) return( -1 ); @@ -356,7 +353,7 @@ vips_math2_const_build( VipsObject *object ) #define LOOPC( IN, OUT, OP ) { \ IN * restrict p = (IN *) in[0]; \ OUT * restrict q = (OUT *) out; \ - double * restrict c = (double *) uconst->c_ready; \ + double * restrict c = uconst->c_double; \ \ for( i = 0, x = 0; x < width; x++ ) \ for( b = 0; b < bands; b++, i++ ) \ diff --git a/libvips/arithmetic/relational.c b/libvips/arithmetic/relational.c index c975f2af..7d18e6ab 100644 --- a/libvips/arithmetic/relational.c +++ b/libvips/arithmetic/relational.c @@ -457,25 +457,18 @@ typedef VipsUnaryConstClass VipsRelationalConstClass; G_DEFINE_TYPE( VipsRelationalConst, vips_relational_const, VIPS_TYPE_UNARY_CONST ); -static int -vips_relational_const_build( VipsObject *object ) -{ - VipsUnary *unary = (VipsUnary *) object; - VipsUnaryConst *uconst = (VipsUnaryConst *) object; - - if( unary->in ) - uconst->const_format = unary->in->BandFmt; - - if( VIPS_OBJECT_CLASS( vips_relational_const_parent_class )-> - build( object ) ) - return( -1 ); - - return( 0 ); +#define RLOOPCI( TYPE, OP ) { \ + TYPE * restrict p = (TYPE *) in[0]; \ + int * restrict c = uconst->c_int; \ + \ + for( i = 0, x = 0; x < width; x++ ) \ + for( b = 0; b < bands; b++, i++ ) \ + out[i] = (p[i] OP c[b]) ? 255 : 0; \ } -#define RLOOPC( TYPE, OP ) { \ +#define RLOOPCF( TYPE, OP ) { \ TYPE * restrict p = (TYPE *) in[0]; \ - TYPE * restrict c = (TYPE *) uconst->c_ready; \ + double * restrict c = uconst->c_double; \ \ for( i = 0, x = 0; x < width; x++ ) \ for( b = 0; b < bands; b++, i++ ) \ @@ -486,7 +479,7 @@ vips_relational_const_build( VipsObject *object ) TYPE * restrict p = (TYPE *) in[0]; \ \ for( i = 0, x = 0; x < width; x++ ) { \ - TYPE * restrict c = (TYPE *) uconst->c_ready; \ + double * restrict c = uconst->c_double; \ \ for( b = 0; b < bands; b++, i++ ) { \ out[i] = OP( p[0], p[1], c[0], c[1]) ? 255 : 0; \ @@ -505,32 +498,64 @@ vips_relational_const_buffer( VipsArithmetic *arithmetic, VipsRelationalConst *rconst = (VipsRelationalConst *) arithmetic; VipsImage *im = arithmetic->ready[0]; int bands = im->Bands; + gboolean is_int = uconst->is_int && + vips_band_format_isint( im->BandFmt ); int i, x, b; switch( rconst->relational ) { - case VIPS_OPERATION_RELATIONAL_EQUAL: - SWITCH( RLOOPC, CLOOPC, ==, CEQUAL ); + case VIPS_OPERATION_RELATIONAL_EQUAL: + if( is_int ) { + SWITCH( RLOOPCI, CLOOPC, ==, CEQUAL ); + } + else { + SWITCH( RLOOPCF, CLOOPC, ==, CEQUAL ); + } break; case VIPS_OPERATION_RELATIONAL_NOTEQ: - SWITCH( RLOOPC, CLOOPC, !=, CNOTEQ ); + if( is_int ) { + SWITCH( RLOOPCI, CLOOPC, !=, CNOTEQ ); + } + else { + SWITCH( RLOOPCF, CLOOPC, !=, CNOTEQ ); + } break; - case VIPS_OPERATION_RELATIONAL_LESS: - SWITCH( RLOOPC, CLOOPC, <, CLESS ); + case VIPS_OPERATION_RELATIONAL_LESS: + if( is_int ) { + SWITCH( RLOOPCI, CLOOPC, <, CLESS ); + } + else { + SWITCH( RLOOPCF, CLOOPC, <, CLESS ); + } break; - case VIPS_OPERATION_RELATIONAL_LESSEQ: - SWITCH( RLOOPC, CLOOPC, <=, CLESSEQ ); + case VIPS_OPERATION_RELATIONAL_LESSEQ: + if( is_int ) { + SWITCH( RLOOPCI, CLOOPC, <=, CLESSEQ ); + } + else { + SWITCH( RLOOPCF, CLOOPC, <=, CLESSEQ ); + } break; - case VIPS_OPERATION_RELATIONAL_MORE: - SWITCH( RLOOPC, CLOOPC, >, CMORE ); + case VIPS_OPERATION_RELATIONAL_MORE: + if( is_int ) { + SWITCH( RLOOPCI, CLOOPC, >, CMORE ); + } + else { + SWITCH( RLOOPCF, CLOOPC, >, CMORE ); + } break; - case VIPS_OPERATION_RELATIONAL_MOREEQ: - SWITCH( RLOOPC, CLOOPC, >=, CMOREEQ ); + case VIPS_OPERATION_RELATIONAL_MOREEQ: + if( is_int ) { + SWITCH( RLOOPCI, CLOOPC, >=, CMOREEQ ); + } + else { + SWITCH( RLOOPCF, CLOOPC, >=, CMOREEQ ); + } break; default: @@ -551,7 +576,6 @@ vips_relational_const_class_init( VipsRelationalConstClass *class ) object_class->nickname = "relational_const"; object_class->description = _( "relational operations against a constant" ); - object_class->build = vips_relational_const_build; aclass->process_line = vips_relational_const_buffer; diff --git a/libvips/arithmetic/remainder.c b/libvips/arithmetic/remainder.c index b845b37e..75df8f4d 100644 --- a/libvips/arithmetic/remainder.c +++ b/libvips/arithmetic/remainder.c @@ -238,15 +238,11 @@ vips_remainder_const_build( VipsObject *object ) { VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); VipsUnary *unary = (VipsUnary *) object; - VipsUnaryConst *uconst = (VipsUnaryConst *) object; if( unary->in && vips_check_noncomplex( class->nickname, unary->in ) ) return( -1 ); - if( unary->in ) - uconst->const_format = unary->in->BandFmt; - if( VIPS_OBJECT_CLASS( vips_remainder_const_parent_class )-> build( object ) ) return( -1 ); @@ -259,7 +255,7 @@ vips_remainder_const_build( VipsObject *object ) #define IREMAINDERCONST( TYPE ) { \ TYPE * restrict p = (TYPE *) in[0]; \ TYPE * restrict q = (TYPE *) out; \ - TYPE * restrict c = (TYPE *) uconst->c_ready; \ + int * restrict c = uconst->c_int; \ \ for( i = 0, x = 0; x < width; x++ ) \ for( b = 0; b < bands; b++, i++ ) \ @@ -271,7 +267,7 @@ vips_remainder_const_build( VipsObject *object ) #define FREMAINDERCONST( TYPE ) { \ TYPE * restrict p = (TYPE *) in[0]; \ TYPE * restrict q = (TYPE *) out; \ - TYPE * restrict c = (TYPE *) uconst->c_ready; \ + int * restrict c = uconst->c_int; \ \ for( i = 0, x = 0; x < width; x++ ) \ for( b = 0; b < bands; b++, i++ ) { \ diff --git a/libvips/arithmetic/unaryconst.c b/libvips/arithmetic/unaryconst.c index b5cbf797..133ede58 100644 --- a/libvips/arithmetic/unaryconst.c +++ b/libvips/arithmetic/unaryconst.c @@ -49,100 +49,6 @@ G_DEFINE_ABSTRACT_TYPE( VipsUnaryConst, vips_unary_const, VIPS_TYPE_UNARY ); -/* Cast a vector of double to a vector of TYPE, clipping to a range. - */ -#define CAST_CLIP( TYPE, N, X ) { \ - TYPE * restrict tq = (TYPE *) q; \ - \ - for( i = 0; i < m; i++ ) { \ - double v = p[VIPS_MIN( n - 1, i )]; \ - \ - tq[i] = (TYPE) VIPS_FCLIP( N, v, X ); \ - } \ -} - -/* Cast a vector of double to a vector of TYPE. - */ -#define CAST( TYPE ) { \ - TYPE * restrict tq = (TYPE *) q; \ - \ - for( i = 0; i < m; i++ ) \ - tq[i] = (TYPE) p[VIPS_MIN( n - 1, i )]; \ -} - -/* Cast a vector of double to a complex vector of TYPE. - */ -#define CASTC( TYPE ) { \ - TYPE * restrict tq = (TYPE *) q; \ - \ - for( i = 0; i < m; i++ ) { \ - tq[0] = (TYPE) p[VIPS_MIN( n - 1, i )]; \ - tq[1] = 0; \ - \ - tq += 2; \ - } \ -} - -/* Cast a n-band vector of double to a m-band vector in another format. - */ -static VipsPel * -make_pixel( VipsObject *obj, - int m, VipsBandFormat fmt, int n, double * restrict p ) -{ - VipsPel *q; - int i; - - if( !(q = VIPS_ARRAY( obj, m * vips_format_sizeof( fmt ), VipsPel )) ) - return( NULL ); - - switch( fmt ) { - case VIPS_FORMAT_CHAR: - CAST_CLIP( signed char, SCHAR_MIN, SCHAR_MAX ); - break; - - case VIPS_FORMAT_UCHAR: - CAST_CLIP( unsigned char, 0, UCHAR_MAX ); - break; - - case VIPS_FORMAT_SHORT: - CAST_CLIP( signed short, SCHAR_MIN, SCHAR_MAX ); - break; - - case VIPS_FORMAT_USHORT: - CAST_CLIP( unsigned short, 0, USHRT_MAX ); - break; - - case VIPS_FORMAT_INT: - CAST_CLIP( signed int, INT_MIN, INT_MAX ); - break; - - case VIPS_FORMAT_UINT: - CAST_CLIP( unsigned int, 0, UINT_MAX ); - break; - - case VIPS_FORMAT_FLOAT: - CAST( float ); - break; - - case VIPS_FORMAT_DOUBLE: - CAST( double ); - break; - - case VIPS_FORMAT_COMPLEX: - CASTC( float ); - break; - - case VIPS_FORMAT_DPCOMPLEX: - CASTC( double ); - break; - - default: - g_assert_not_reached(); - } - - return( q ); -} - static int vips_unary_const_build( VipsObject *object ) { @@ -161,27 +67,43 @@ vips_unary_const_build( VipsObject *object ) uconst->n = VIPS_MAX( uconst->n, unary->in->Bands ); arithmetic->base_bands = uconst->n; - if( unary->in && uconst->c ) { + if( unary->in && + uconst->c ) { if( vips_check_vector( class->nickname, uconst->c->n, unary->in ) ) return( -1 ); } - /* Some operations need the vector in the input type (eg. - * im_equal_vec() where the output type is always uchar and is useless - * for comparisons), some need it in the output type (eg. - * im_andimage_vec() where we want to get the double to an int so we - * can do bitwise-and without having to cast for each pixel), some - * need a fixed type (eg. im_powtra_vec(), where we want to keep it as - * double). + /* Some operations need int constants, for example boolean AND, SHIFT + * etc. * - * Therefore pass in the desired vector type as a param. + * Some can use int constants as an optimisation, for example (x < + * 12). It depends on the value though: obviously (x < 12.5) should + * not use the int form. */ + if( uconst->c ) { + int i; - if( uconst->c ) - uconst->c_ready = make_pixel( (VipsObject *) uconst, - uconst->n, uconst->const_format, - uconst->c->n, (double *) uconst->c->data ); + uconst->c_int = VIPS_ARRAY( object, uconst->n, int ); + uconst->c_double = VIPS_ARRAY( object, uconst->n, double ); + if( !uconst->c_int || + !uconst->c_double ) + return( -1 ); + + for( i = 0; i < uconst->n; i++ ) + uconst->c_double[i] = ((double *) uconst->c->data) + [VIPS_MIN( i, uconst->c->n - 1)]; + + for( i = 0; i < uconst->n; i++ ) + uconst->c_int[i] = uconst->c_double[i]; + + uconst->is_int = TRUE; + for( i = 0; i < uconst->n; i++ ) + if( uconst->c_int[i] != uconst->c_double[i] ) { + uconst->is_int = FALSE; + break; + } + } if( VIPS_OBJECT_CLASS( vips_unary_const_parent_class )-> build( object ) ) diff --git a/libvips/arithmetic/unaryconst.h b/libvips/arithmetic/unaryconst.h index 2d204426..b2c26135 100644 --- a/libvips/arithmetic/unaryconst.h +++ b/libvips/arithmetic/unaryconst.h @@ -59,16 +59,15 @@ typedef struct _VipsUnaryConst { */ VipsArea *c; - /* The format the constant should be cast to. Subclasses set this - * ready for unaryconst's build method. - */ - VipsBandFormat const_format; - - /* Our constant expanded to match arith->ready in size and - * const_format in type. + /* Our constant expanded to match arith->ready in size. We need int + * and double versions. + * + * is_int is TRUE if the two arrays are equal for every element. */ int n; - VipsPel *c_ready; + int *c_int; + double *c_double; + gboolean is_int; } VipsUnaryConst;