Fix the pixel shift within reduce (#703) #1592

kleisauke · 2020-03-24T11:38:09Z

See: #703 and libvips/pyvips#148 for context. We might need to notify some people (for e.g. #659) after this is done.

~~Marked this PR as draft since it still produces a small pixel shift (within reducev and perhaps also in reduceh) that I can't quite figure out (it might be a rounding error). See this testcase:~~

reducev testcase

vips reducev input/Landscape_1.jpg output-patch/lanczos3/Landscape_1.jpg[strip,Q=85] 5.624992969 --kernel lanczos3 --centre

vips reducev input/Landscape_2.jpg temp.v 5.624992969 --kernel lanczos3 --centre
vips flip temp.v output-patch/lanczos3/Landscape_2.jpg[strip,Q=85] horizontal

vips reducev input/Landscape_3.jpg temp.v 5.624992969 --kernel lanczos3 --centre
vips rot temp.v output-patch/lanczos3/Landscape_3.jpg[strip,Q=85] d180

vips reducev input/Landscape_4.jpg temp.v 5.624992969 --kernel lanczos3 --centre
vips rot temp.v temp2.v d180
vips flip temp2.v output-patch/lanczos3/Landscape_4.jpg[strip,Q=85] horizontal

vips rot input/Landscape_5.jpg temp.v d270
vips reducev temp.v temp2.v 5.624992969 --kernel lanczos3 --centre
vips flip temp2.v output-patch/lanczos3/Landscape_5.jpg[strip,Q=85] vertical

vips rot input/Landscape_6.jpg temp.v d90
vips reducev temp.v output-patch/lanczos3/Landscape_6.jpg[strip,Q=85] 5.624992969 --kernel lanczos3 --centre

vips rot input/Landscape_7.jpg temp.v d90
vips reducev temp.v temp2.v 5.624992969 --kernel lanczos3 --centre
vips flip temp2.v output-patch/lanczos3/Landscape_7.jpg[strip,Q=85] vertical

vips rot input/Landscape_8.jpg temp.v d270
vips reducev temp.v output-patch/lanczos3/Landscape_8.jpg[strip,Q=85] 5.624992969 --kernel lanczos3 --centre

rm {temp,temp2}.v

Output:
https://github.com/kleisauke/vips-issue-703/raw/master/output-patch/Landscape-vertical.webp

kleisauke · 2020-05-06T17:46:01Z

After a few hours of debugging I discovered when that small pixel shift occurs and I suspect what's causing that.

Because this pixel shift only occurs when resizing to certain dimensions, I've started to refactor/simplify a number of things. I removed the round-to-nearest behavior with commit ff8d397 but for some reason the tests revealed that the resized image was the same with or without this change.

When I continued to make some things simpler, I noticed something strange. I changed vips_reduce_get_points to just return the kernel radius (so without the rint and 2 * ... + 1) with commit kleisauke@b732cf2 and swapped the reducev->n_point / 2 - 1 to that kernel radius. Then I realized that I was passing a double to the x and y positions of vips_embed here:
b732cf2#diff-d81f64fd1fa063fc9364e0d3572d6e70R492
b732cf2#diff-88601bd8b4ec37b43308ce64a6e6e1a7R873

So, I did a sanity check and returned an error if the kernel radius is not a whole number
b732cf2#diff-d81f64fd1fa063fc9364e0d3572d6e70R447-R451
b732cf2#diff-88601bd8b4ec37b43308ce64a6e6e1a7R850-R853

It turned out that the target dimensions that caused small pixel shifts were now producing errors. 😅

For example:

image = image.reducev(
    1.5, VImage::option()->set("kernel", VIPS_KERNEL_LANCZOS3));

image = image.reduceh(
    1.5, VImage::option()->set("kernel", VIPS_KERNEL_LANCZOS3));

Produces a small pixel shift since 3 * 1.5 is not a whole number. However, swapping the kernel with VIPS_KERNEL_CUBIC won't cause any pixel shift (since kernel radius is 2 * 1.5).

@jcupitt Do you have any ideas how to fix this? I don't think we can remove the vips_embed in reducev / reduceh. I've updated the https://github.com/kleisauke/vips-issue-703 repository to facilitate debugging.

jcupitt · 2020-05-08T19:25:07Z

This is a really good idea!

Look at this thing (calculate coeffs for a cubic filter):

https://github.com/libvips/libvips/blob/master/libvips/resample/templates.h#L346

We calculate the filter width (number of points to compute the filter over, eg. 17 for factor 4 shrink) like this:

        const int n_points = rint( 4 * shrink ) + 1;

For shrink 4.3 (for example), it'll be rint(17.2)+1 or 18, not an odd number (the point of the +1), so you'll get an asymmettry there. It should probably be 4 * rint( shrink ) + 1.

Then the rint will throw away some information: a 4.3 shrink will have the same centre as a 4.0 shrink. Really, the fractional part of the shrink should be added to the x somehow (I think) so you get a displacement. Or perhaps this is included in the calculation of xp?

I'll try getting it to output some graphs.

jcupitt · 2020-05-10T11:57:01Z

Here's a standalone test program. It's copy-pasted from master with minimal changes to make it compile and run.

/* compile with
 *
 * 	gcc -g -Wall mask.c -lm
 */

#include <stdlib.h>
#include <stdio.h>
#include <math.h>

typedef enum {
        VIPS_KERNEL_NEAREST,
        VIPS_KERNEL_LINEAR,
        VIPS_KERNEL_CUBIC,
        VIPS_KERNEL_MITCHELL,
        VIPS_KERNEL_LANCZOS2,
        VIPS_KERNEL_LANCZOS3,
        VIPS_KERNEL_LAST
} VipsKernel;

#define VIPS_FABS( V ) __builtin_fabs( V )
#define g_assert_not_reached() abort()

/* Get n points. @shrink is the shrink factor, so 2 for a 50% reduction. 
 */
static int
vips_reduce_get_points( VipsKernel kernel, double shrink )
{
        switch( kernel ) {
        case VIPS_KERNEL_NEAREST:
                return( 1 );

        case VIPS_KERNEL_LINEAR:
                return( rint( 2 * shrink ) + 1 ); 

        case VIPS_KERNEL_CUBIC:
        case VIPS_KERNEL_MITCHELL:
                return( rint( 4 * shrink ) + 1 ); 

        case VIPS_KERNEL_LANCZOS2:
                /* Needs to be in sync with calculate_coefficients_lanczos().
                 */
                return( rint( 2 * 2 * shrink ) + 1 );

        case VIPS_KERNEL_LANCZOS3:
                return( rint( 2 * 3 * shrink ) + 1 );

        default:
                g_assert_not_reached();
                return( 0 );
        }
}

/* Generate a cubic filter. See:
 *
 * Mitchell and Netravali, Reconstruction Filters in Computer Graphics
 * Computer Graphics, Volume 22, Number 4, August 1988.
 *
 * B = 1,   C = 0   - cubic B-spline
 * B = 1/3, C = 1/3 - Mitchell
 * B = 0,   C = 1/2 - Catmull-Rom spline
 */
static void 
calculate_coefficients_cubic( double *c,
        const double shrink, const double x, double B, double C )
{
        /* Needs to be in sync with vips_reduce_get_points().
         */
        const int n_points = rint( 4 * shrink ) + 1;

        int i;
        double sum;

        sum = 0;
        for( i = 0; i < n_points; i++ ) {
                const double xp = (i - (2 * shrink - 1) - x) / shrink;
                const double axp = VIPS_FABS( xp );
                const double axp2 = axp * axp;
                const double axp3 = axp2 * axp;

                double l;

                if( axp <= 1 )
                        l = ((12 - 9 * B - 6 * C) * axp3 +
                             (-18 + 12 * B + 6 * C) * axp2 +
                             (6 - 2 * B)) / 6;
                else if( axp <= 2 )
                        l = ((-B - 6 * C) * axp3 +
                             (6 * B + 30 * C) * axp2 +
                             (-12 * B - 48 * C) * axp +
                             (8 * B + 24 * C)) / 6;
                else
                        l = 0.0;

                c[i] = l;
                sum += l;
        }

        for( i = 0; i < n_points; i++ )
                c[i] /= sum;
}

/* Calculate a mask element.
 */
static void
vips_reduce_make_mask( double *c, VipsKernel kernel, double shrink, double x )
{
        switch( kernel ) {
        case VIPS_KERNEL_CUBIC:
                /* Catmull-Rom.
                 */
                calculate_coefficients_cubic( c, shrink, x, 0.0, 0.5 );
                break;

        case VIPS_KERNEL_MITCHELL:
                calculate_coefficients_cubic( c, shrink, x,
                        1.0 / 3.0, 1.0 / 3.0 );
                break;

	/* Removed non-cubic filters.
	 */

        default:
                g_assert_not_reached();
                break;
        }
}

int 
main( int argc, char **argv )
{
	double shrink;

	for( shrink = 1.0; shrink < 10; shrink += 0.1 ) {
		int n_points;
		double mask[100];
		int i;

		n_points = vips_reduce_get_points( VIPS_KERNEL_MITCHELL, 
			shrink );
		if( n_points > 100 ) {
			printf( "too large!\n" );
			abort();
		}

		vips_reduce_make_mask( mask, 
			VIPS_KERNEL_MITCHELL, shrink, 0.5 );
		printf( "%g", shrink );
		for( i = 0; i < n_points; i++ )
			printf( ", %g", mask[i] );
		printf( "\n" ); 
	}

	return( 0 );
}

It generates the mask for x == 0.5 for Mitchell for every shrink factor from 1 to 10 in 0.1 steps. Here's the mask for shrink == 10 to show the shape we expect:

Features:

small negatives either side of a central peak, zero at the edges -- this is the classic Mitchell shape
peak should be in the centre of the graph -- at 0.5, we expect the "energy" of the filter to be at the half-way point, ie. we have n_points samples, n_points is always odd, we have n_points / 2 samples left of the peak, then the peak, then points / 2 right of the peak
should be symmetrical -- the n_points / 2 samples each side should be mirror images

Now have a look at shrink == 1.4

The peak is not central -- we've missed by ~0.5 pixels or so. I think this could be at least part of your shift. It's not symmetrical either, but that's because of the extra shift we've incorporated.

That's actually one of the better ones -- the error in mask size calculation makes the shift even worse in some cases.

I'll make a PR that fixes up mask generation, since this is a simple and clear error, then let's look at shifts on rotate again.

jcupitt · 2020-05-10T17:40:42Z

Actually, it's really easy, here's a fixed up version of that mask.c:

/* compile with
 *
 * 	gcc -g -Wall mask.c -lm
 *
 * run with
 *
 * 	gcc -g -Wall mask.c -lm && ./a.out > masks.csv && gnumeric masks.csv 
 */

#include <stdlib.h>
#include <stdio.h>
#include <math.h>

#define VIPS_FABS( V ) __builtin_fabs( V )
#define VIPS_MAX( A, B ) ((A) > (B) ? (A) : (B))
#define VIPS_PI (3.14159265358979323846)
#define g_assert_not_reached() abort()

typedef enum {
        VIPS_KERNEL_NEAREST,
        VIPS_KERNEL_LINEAR,
        VIPS_KERNEL_CUBIC,
        VIPS_KERNEL_MITCHELL,
        VIPS_KERNEL_LANCZOS2,
        VIPS_KERNEL_LANCZOS3,
        VIPS_KERNEL_LAST
} VipsKernel;

/* Get n points. @shrink is the shrink factor, so 2 for a 50% reduction. 
 */
static int
vips_reduce_get_points( VipsKernel kernel, double shrink )
{
        switch( kernel ) {
        case VIPS_KERNEL_NEAREST:
                return( 1 );

        case VIPS_KERNEL_LINEAR:
                return( 2 * rint( shrink ) + 1 ); 

        case VIPS_KERNEL_CUBIC:
        case VIPS_KERNEL_MITCHELL:
                return( 2 * rint( 2 * shrink ) + 1 ); 

        case VIPS_KERNEL_LANCZOS2:
                return( 2 * rint( 2 * shrink ) + 1 );

        case VIPS_KERNEL_LANCZOS3:
                return( 2 * rint( 3 * shrink ) + 1 );

        default:
                g_assert_not_reached();
                return( 0 );
        }
}

static void 
calculate_coefficients_triangle( double *c,
        const double shrink, const double x )
{
        /* Needs to be in sync with vips_reduce_get_points().
         */
        const int n_points = 2 * rint( shrink ) + 1;
        const double half = x + n_points / 2.0 - 1;

        int i;
        double sum;

        sum = 0;
        for( i = 0; i < n_points; i++ ) {
                double xp = (i - half) / shrink;

                double l;

                l = 1.0 - VIPS_FABS( xp );
                l = VIPS_MAX( 0.0, l );

                c[i] = l;
                sum += l;
        }

        for( i = 0; i < n_points; i++ )
                c[i] /= sum;
}

static void 
calculate_coefficients_cubic( double *c,
        const double shrink, const double x, double B, double C )
{
        /* Needs to be in sync with vips_reduce_get_points().
         */
        const int n_points = 2 * rint( 2 * shrink ) + 1;
        const double half = x + n_points / 2.0 - 1;

        int i;
        double sum;

        sum = 0;
        for( i = 0; i < n_points; i++ ) {
                const double xp = (i - half) / shrink;
                const double axp = VIPS_FABS( xp );
                const double axp2 = axp * axp;
                const double axp3 = axp2 * axp;

                double l;

                if( axp <= 1 )
                        l = ((12 - 9 * B - 6 * C) * axp3 +
                             (-18 + 12 * B + 6 * C) * axp2 +
                             (6 - 2 * B)) / 6;
                else if( axp <= 2 )
                        l = ((-B - 6 * C) * axp3 +
                             (6 * B + 30 * C) * axp2 +
                             (-12 * B - 48 * C) * axp +
                             (8 * B + 24 * C)) / 6;
                else
                        l = 0.0;

                c[i] = l;
                sum += l;
        }

        for( i = 0; i < n_points; i++ )
                c[i] /= sum;
}

static void 
calculate_coefficients_lanczos( double *c,
        const int a, const double shrink, const double x )
{
        /* Needs to be in sync with vips_reduce_get_points().
         */
        const int n_points = 2 * rint( a * shrink ) + 1;
        const double half = x + n_points / 2.0 - 1;

        int i;
        double sum;

        sum = 0;
        for( i = 0; i < n_points; i++ ) {
                double xp = (i - half) / shrink;
                double l;

                if( xp == 0.0 )
                        l = 1.0;
                else if( xp < -a )
                        l = 0.0;
                else if( xp > a )
                        l = 0.0;
                else
                        l = (double) a * sin( VIPS_PI * xp ) *
                                sin( VIPS_PI * xp / (double) a ) /
                                (VIPS_PI * VIPS_PI * xp * xp);

		//printf( "%d, %g, %g\n", i, xp, l );

                c[i] = l;
                sum += l;
        }

        for( i = 0; i < n_points; i++ )
                c[i] /= sum;
}

static void
vips_reduce_make_mask( double *c, VipsKernel kernel, double shrink, double x )
{
        switch( kernel ) {
        case VIPS_KERNEL_NEAREST:
                c[0] = 1.0;
                break;

        case VIPS_KERNEL_LINEAR:
                calculate_coefficients_triangle( c, shrink, x );
                break;

        case VIPS_KERNEL_CUBIC:
                /* Catmull-Rom.
                 */
                calculate_coefficients_cubic( c, shrink, x, 0.0, 0.5 );
                break;

        case VIPS_KERNEL_MITCHELL:
                calculate_coefficients_cubic( c, shrink, x,
                        1.0 / 3.0, 1.0 / 3.0 );
                break;

        case VIPS_KERNEL_LANCZOS2:
                calculate_coefficients_lanczos( c, 2, shrink, x );
                break;

        case VIPS_KERNEL_LANCZOS3:
                calculate_coefficients_lanczos( c, 3, shrink, x );
                break;

        default:
                g_assert_not_reached();
                break;
        }
}


int 
main( int argc, char **argv )
{
	//const VipsKernel kernel = VIPS_KERNEL_LINEAR;
	//const VipsKernel kernel = VIPS_KERNEL_MITCHELL;
	//const VipsKernel kernel = VIPS_KERNEL_LANCZOS2;
	const VipsKernel kernel = VIPS_KERNEL_LANCZOS3;

	double shrink;
	double mask[100];
	int i;

	for( shrink = 1.0; shrink < 10; shrink += 0.1 ) {
		const int n_points = vips_reduce_get_points( kernel, shrink );

		if( n_points > 100 ) {
			printf( "too large!\n" );
			abort();
		}

		vips_reduce_make_mask( mask, kernel, shrink, 0.5 );

		printf( "%g", shrink );
		for( i = 0; i < n_points; i++ )
			printf( ", %g", mask[i] );
		printf( "\n" ); 
	}

	return( 0 );
}

So this has improved mask size calculation, and improved positioning of the peak. It seems to generate the correct mask for all cases.

The two changes are:

        case VIPS_KERNEL_LANCZOS3:
                return( 2 * rint( 3 * shrink ) + 1 );

Mask size is always calculated with this pattern: 2 * rint( mask_range * shrink ) + 1, where the range is 1 for linear, 2 for cubic, 3 for lanczos3.

And something to get the peak in the correct place:

        const int n_points = ...
        const double half = x + n_points / 2.0 - 1;

...

                double xp = (i - half) / shrink;

ie. we offset the mask by a amount based on the rint() in the mask size.

I'll make a PR with this change.

We were seeing small displacements at some shrink factors because of rounding in the mask size calcualation. This PR takes the rounding into account when positioning the mask. See #1592 (comment)

jcupitt · 2020-05-10T17:49:08Z

OK, PR with fixed up kernel calculation. If you rebase on top of that, it might fix some of the rotation shifts you are seeing.

kleisauke · 2020-05-10T18:44:54Z

Thanks! I've rebased this PR on top of the revise-kernel-calculation branch. Unfortunately, I still see some small pixel shifts. See:
https://github.com/kleisauke/vips-issue-703/tree/2181164820af5f3915a35a421ed3c0b3c2f7689b#output-after-pull-request

fwiw, the pixel shifts now seems to apply to both portrait and landscape images, so we might be on the right track. Here are the results before the rebase:
https://github.com/kleisauke/vips-issue-703/tree/4e478446c7a1bad32080c7d76bfa2c5763270495#output-after-patching

Could issue #1512 also affect these small pixel shifts?

kleisauke · 2020-05-10T19:43:56Z

I had a quick go at testing if #1512 could also affect this by commenting out the shrink part from resize.c:

Details

diff --git a/libvips/resample/resize.c b/libvips/resample/resize.c
index 1111111..2222222 100644
--- a/libvips/resample/resize.c
+++ b/libvips/resample/resize.c
@@ -194,8 +194,8 @@ vips_resize_build( VipsObject *object )
 
 	/* The int part of our scale.
 	 */
-	int_hshrink = vips_resize_int_shrink( resize, hscale );
-	int_vshrink = vips_resize_int_shrink( resize, vscale );
+	/*int_hshrink = vips_resize_int_shrink( resize, hscale );
+	int_vshrink = vips_resize_int_shrink( resize, vscale );*/
 
 	/* Unpack for processing.
 	 */
@@ -203,7 +203,7 @@ vips_resize_build( VipsObject *object )
 		return( -1 );
 	in = t[5];
 
-	if( int_vshrink > 1 ) { 
+	/*if( int_vshrink > 1 ) { 
 		g_info( "shrinkv by %d", int_vshrink );
 		if( vips_shrinkv( in, &t[0], int_vshrink, NULL ) )
 			return( -1 );
@@ -219,7 +219,7 @@ vips_resize_build( VipsObject *object )
 		in = t[1];
 
 		hscale *= int_hshrink;
-	}
+	}*/
 
 	/* Don't let either axis drop below 1 px.
 	 */

It seems that this only affects the landscape images, see:
kleisauke/vips-issue-703@703cccf
https://github.com/kleisauke/vips-issue-703/tree/703cccfa2d0a1c34bef1e0316d6cc62a374297eb#output-after-pull-request

jcupitt · 2020-05-11T10:00:08Z

Yes, I think you're right.

Block shrink will round to nearest, so it can add or remove up to 0.5 pixels from the output image. These added or removed pixels will always be along the right or bottom edge, so you'll get a small displacement.

Eg. the embed in shrinkh does this:

        /* We need new pixels at the right so that we don't have small chunks
         * to average down the right edge.
         */
        if( vips_embed( in, &t[1],
                0, 0,
                in->Xsize + shrink->hshrink, in->Ysize,
                "extend", VIPS_EXTEND_COPY,
                NULL ) )
                return( -1 );

Instead of 0, 0,, we should perhaps offset by half of the number of input pixels we are rounding up or down by. We'd need matching changes to shrinkv, reduceh, reducev.

Shall I make a PR?

kleisauke · 2020-05-11T10:27:32Z

Feel free to create a new PR.

BTW, do you know how to properly test this commit?:
0d00794

I get exactly the same image with or without that change. It looks like this behavior was introduced in 7fd672f, but the removed TODO-items suggests that it only applies to the upscale path (i.e. vips_affine).

int bicubic needs more precision? try a 10x upscale and compare to the
double path

yes, looks like a rounding problem?

upscale 10x, you get a small offset if you compare the double and int paths

and slight banding

jcupitt · 2020-05-11T11:11:13Z

Actually, thinking about it again, this will make the pixels at the top-left of an image shift about as you adjust the parameters to shrink, which sounds awful. This will need to be enabled by a centre option.

jcupitt · 2020-05-11T14:20:10Z

I started adding a centre option to shrink, then realized there would need to be something similar on JPEG shrink-on-load as well.

Rather than adding a centre option to all these operators, it's probably better if the top-level driver program (thumbnail, in this case) calculates the overall shift and does the adjustment in one place as part of the final reduce step.

The code I wrote for shrinkh, for reference:

        /* Output size. We need to always round to nearest, so round(), not
         * rint().
         */
        width = VIPS_ROUND_UINT( 
                (double) resample->in->Xsize / shrink->hshrink );

        /* We need new pixels around the edges so that we don't have to
         * average half-pixels anywhere.
         *
         * In centre mode, we add pixels around all the edges, otherwise, only
         * to the right.
         */
        if( shrinkh->centre ) {
                /* How many pixels we are inventing in the input, -ve for
                 * discarding.
                 */ 
                int extra_pixels = 
                        width * shrink->hshrink - resample->in->Xsize;

                /* If we are rounding down, we are not using some input
                 * pixels. We need to move the origin *inside* the input image
                 * by half that distance so that we discard pixels equally
                 * from left and right. 
                 */
                left = (1 + extra_pixels) / 2;
        }
        else
                left = 0;

kleisauke · 2020-05-11T14:32:02Z

Great! Isn't it possible to centre by default? This matches the centre option in resize (which is deprecated now).

For reference, here's the output with the test image from libvips/pyvips#148 before and after this PR (at the time of writing).

Details

VIPS shrink()

before	after	after with VIPS_NOVECTOR=1

No changes. We still need to fix #1512. Here's the Pillow image for reference.

VIPS reduce()

before	after	after with VIPS_NOVECTOR=1

The 0.5 pixel shift seems to be gone and the vector patch introduces some additional moire (issue #1518). Here's the Pillow image for reference (if compared to libvips, there still seems to be a tiny pixel shift).

VIPS resize()

before	after	after with VIPS_NOVECTOR=1

If compared to Pillow, the bottom red border is more intensive (this could be due to different edges handling and is probably OK). However, the right border is gone (which could happen as a result of the above issues).

We were seeing small displacements at some shrink factors because of rounding in the mask size calcualation. This PR takes the rounding into account when positioning the mask. See libvips#1592 (comment) (cherry picked from commit d64385a)

kleisauke · 2020-05-23T11:08:43Z

I had a look at your reference implementation and wondered if the left variable is a double? If so, the vips_embed will truncate it to an integer and discard some information.

I did some printf() debugging within commit 55e525a and saw:

Details

$ seq 2 20 | xargs -I{} vips shrinkv input/Landscape_1.jpg output-patch/lanczos3/Landscape_1.jpg[strip,Q=85] {}
vshrink: 2, top: 0.500000
vshrink: 3, top: 0.500000
vshrink: 4, top: 0.500000
vshrink: 5, top: 0.500000
vshrink: 6, top: 0.500000
vshrink: 7, top: -1.000000
vshrink: 8, top: 0.500000
vshrink: 9, top: -1.000000
vshrink: 10, top: 0.500000
vshrink: 11, top: 0.000000
vshrink: 12, top: 0.500000
vshrink: 13, top: -1.500000
vshrink: 14, top: 2.500000
vshrink: 15, top: 0.500000
vshrink: 16, top: 0.500000
vshrink: 17, top: 4.000000
vshrink: 18, top: 3.500000
vshrink: 19, top: -1.000000
vshrink: 20, top: 0.500000
$ seq 2 20 | xargs -I{} vips shrinkh input/Landscape_1.jpg output-patch/lanczos3/Landscape_1.jpg[strip,Q=85] {}
hshrink: 2, left: 0.500000
hshrink: 3, left: 0.500000
hshrink: 4, left: 0.500000
hshrink: 5, left: 0.500000
hshrink: 6, left: 0.500000
hshrink: 7, left: 0.000000
hshrink: 8, left: 0.500000
hshrink: 9, left: 0.500000
hshrink: 10, left: 0.500000
hshrink: 11, left: 2.500000
hshrink: 12, left: 0.500000
hshrink: 13, left: -2.500000
hshrink: 14, left: 3.500000
hshrink: 15, left: 0.500000
hshrink: 16, left: 4.500000
hshrink: 17, left: 1.500000
hshrink: 18, left: 0.500000
hshrink: 19, left: 3.000000
hshrink: 20, left: 0.500000

So there's usually a fractional part present.

jcupitt · 2020-05-23T13:00:49Z

Yes, exactly, reduce gets around this by displacing the mask by the opposite fractional amount, but shrink can't do this sub-pixel movement, of course. JPEG shrink-on-load will have the same problem, but even worse.

I think the best solution is probably to accumulate the errors from the three stages (shrink on load always rounds down, block shrink rounds to nearest, reduce rounds to nearest) and do a final fractional displacement to correct all the errors as part of reduce. What do you think?

You're right that #1512 can be fixed at the same time.

We were seeing small displacements at some shrink factors because of rounding in the mask size calcualation. This PR takes the rounding into account when positioning the mask. See libvips#1592 (comment) (cherry picked from commit d64385a)

Portrait images seems to be fixed.

`nearest_Portrait.webp` seems to be fixed as well.

kleisauke · 2020-05-25T14:11:03Z

Sounds good! I had a go with implementing a fractional displacement within reduce, see:
e1f1fad
e7d8e85

This seems to fix all the portrait images on the vips-issue-703 repository. For example this image:
https://raw.githubusercontent.com/kleisauke/vips-issue-703/master/output-patch/lanczos3_Portrait.webp

Does not produce any pixel shifts. 🎉

I've a look to the rounding errors, feel free to commit on this branch as well.

All pixel shifts are gone!

jcupitt · 2020-06-04T13:33:41Z

/me falls off his chair

kleisauke · 2020-06-04T14:08:33Z

Yeah, I had to check it twice because I couldn't believe it. 😅

All pixel shifts are gone with commit 793f90b (which is squashed within f51336c). The output can be viewed here:
https://github.com/kleisauke/vips-issue-703/tree/master/output-patch

There's still a Travis error within the test suite that needs to be investigated, but otherwise this is ready for reviewing. Should I remove the draft status?

Note that this PR does not fully resolve libvips/pyvips#148, this will be addressed within the remaining issues (#1512 and #1518).

kleisauke · 2020-06-04T15:33:18Z

fwiw, I also checked whether shrink-on-load for JPEG images could lead to pixel shifts, see: kleisauke/vips-issue-703@af4e70b

It does not seem to cause any problems with these test images and target dimensions (before / after). I'll also have a look at WebP, PDF and SVG images, perhaps one of them needs an overall shift.

By not calling vips_vector_to_fixed_point repeatedly.

In line with reduceh.

In line with reducev.

It seems that it generates the same image, with or without this change. Tested with https://github.com/kleisauke/vips-issue-703.

kleisauke · 2020-06-06T12:42:51Z

OK, test suite failure is now fixed. I also cleaned up the commits a bit.

This is now ready for reviewing.

Found by UBSan.

jcupitt · 2020-06-12T11:36:08Z

Wow, I've read the whole thing as carefully as I can. I couldn't see anything wrong and I now have a terrible headache.

What a great job you've done here Kleis! Let's merge and do some wider testing.

lovell · 2020-06-12T14:04:32Z

Amazing work, thanks Kleis! As soon as we can meet in-person then beer/coffee/tea is on me to thank you for this and everything else. (I was supposed to be in the Netherlands next week but as you can imagine that's no longer happening.)

kleisauke changed the title ~~Partially fix the pixel shift within reduce (#703)~~ [WIP] Fix the pixel shift within reduce (#703) Mar 24, 2020

kleisauke added a commit to kleisauke/vips-issue-703 that referenced this pull request May 6, 2020

Update for libvips/libvips#1592

8bb0cf5

kleisauke force-pushed the issue-703 branch from ad568cf to ac4a0f2 Compare May 6, 2020 15:25

jcupitt mentioned this pull request May 10, 2020

revise kernel mask calculations #1647

Merged

kleisauke force-pushed the issue-703 branch from ac4a0f2 to 0d00794 Compare May 10, 2020 18:16

kleisauke added a commit to kleisauke/vips-issue-703 that referenced this pull request May 10, 2020

Update for libvips/libvips#1592

34377cb

kleisauke force-pushed the issue-703 branch from 0d00794 to 55e525a Compare May 23, 2020 11:04

kleisauke force-pushed the issue-703 branch from 55e525a to e1f1fad Compare May 25, 2020 13:38

kleisauke added a commit to kleisauke/vips-issue-703 that referenced this pull request May 25, 2020

Update for libvips/libvips#1592

d0d2f1d

Portrait images seems to be fixed.

kleisauke added a commit to kleisauke/vips-issue-703 that referenced this pull request May 25, 2020

Update for libvips/libvips#1592

26f3698

`nearest_Portrait.webp` seems to be fixed as well.

kleisauke added a commit to kleisauke/vips-issue-703 that referenced this pull request Jun 4, 2020

Update for libvips/libvips#1592

44114f9

All pixel shifts are gone!

kleisauke force-pushed the issue-703 branch from 793f90b to 97f83a3 Compare June 4, 2020 13:32

kleisauke force-pushed the issue-703 branch from 97f83a3 to 0169071 Compare June 4, 2020 13:43

kleisauke added 6 commits June 6, 2020 14:01

Fix the pixel shift within reduce (libvips#703)

ac358ff

Speed up the mask construction for uchar/ushort images

b6e4e9e

By not calling vips_vector_to_fixed_point repeatedly.

Ensure reducev is THINSTRIP

dfdf899

In line with reduceh.

Prefer an immediate calculation where possible

369b098

In line with reducev.

Formatting and whitespace changes

c0ed106

Remove round-to-nearest behaviour

ac30bad

It seems that it generates the same image, with or without this change. Tested with https://github.com/kleisauke/vips-issue-703.

kleisauke force-pushed the issue-703 branch from fadeed4 to ac30bad Compare June 6, 2020 12:32

kleisauke changed the title ~~[WIP] Fix the pixel shift within reduce (#703)~~ Fix the pixel shift within reduce (#703) Jun 6, 2020

kleisauke marked this pull request as ready for review June 6, 2020 12:42

reducev: Fix undefined-behaviour within the vector path

d7a7354

Found by UBSan.

jcupitt merged commit 75632a5 into libvips:master Jun 12, 2020

lovell mentioned this pull request Jul 19, 2020

Resizing image with a transparent background generates a thin white line in the output lovell/sharp#2299

Closed

kleisauke mentioned this pull request Nov 2, 2020

Resolve rounding issues in reduce #1872

Merged

Nakilon mentioned this pull request Mar 2, 2022

Comparison quality loss, probably due to libvips update Nakilon/dhash-vips#16

Closed

kleisauke deleted the issue-703 branch July 30, 2022 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the pixel shift within reduce (#703) #1592

Fix the pixel shift within reduce (#703) #1592

kleisauke commented Mar 24, 2020 •

edited

kleisauke commented May 6, 2020

jcupitt commented May 8, 2020

jcupitt commented May 10, 2020

jcupitt commented May 10, 2020

jcupitt commented May 10, 2020

kleisauke commented May 10, 2020

kleisauke commented May 10, 2020

jcupitt commented May 11, 2020

kleisauke commented May 11, 2020 •

edited

jcupitt commented May 11, 2020

jcupitt commented May 11, 2020 •

edited

kleisauke commented May 11, 2020

VIPS shrink()

VIPS reduce()

VIPS resize()

kleisauke commented May 23, 2020

jcupitt commented May 23, 2020

kleisauke commented May 25, 2020

jcupitt commented Jun 4, 2020

kleisauke commented Jun 4, 2020

kleisauke commented Jun 4, 2020

kleisauke commented Jun 6, 2020

jcupitt commented Jun 12, 2020

lovell commented Jun 12, 2020

Fix the pixel shift within reduce (#703) #1592

Fix the pixel shift within reduce (#703) #1592

Conversation

kleisauke commented Mar 24, 2020 • edited

kleisauke commented May 6, 2020

jcupitt commented May 8, 2020

jcupitt commented May 10, 2020

jcupitt commented May 10, 2020

jcupitt commented May 10, 2020

kleisauke commented May 10, 2020

kleisauke commented May 10, 2020

jcupitt commented May 11, 2020

kleisauke commented May 11, 2020 • edited

jcupitt commented May 11, 2020

jcupitt commented May 11, 2020 • edited

kleisauke commented May 11, 2020

VIPS shrink()

VIPS reduce()

VIPS resize()

kleisauke commented May 23, 2020

jcupitt commented May 23, 2020

kleisauke commented May 25, 2020

jcupitt commented Jun 4, 2020

kleisauke commented Jun 4, 2020

kleisauke commented Jun 4, 2020

kleisauke commented Jun 6, 2020

jcupitt commented Jun 12, 2020

lovell commented Jun 12, 2020

kleisauke commented Mar 24, 2020 •

edited

kleisauke commented May 11, 2020 •

edited

jcupitt commented May 11, 2020 •

edited