Pixels shifts (EXIF orientation images)

[kleisauke]

Discovered a strange bug when testing the auto rotation of EXIF orientation images, it looks like the pixels are shifting for some images.

Test images:
https://github.com/recurser/exif-orientation-examples

Test case:
bug.zip

Usage:

• composer install
• php test.php
• see the output folder

Output as GIF slideshow:

^{It's GIF so horrible quality, but you clearly see the (random?) pixel shifts by the portrait images.}

Tested with libvips 8.6.0-alpha1 and 8.5.7, the output is the same.

[jcupitt]

Wow that is strange. I'll have a look.

[jcupitt]

Back on this after a break. It looks like it's a problem in reduce, I'll try to make a test case.

[kleisauke]

I could make a test case with vips_resize or vips_reduce, if you want.

[jcupitt]

I think we'll have to bump this to 8.7, 8.6 is already horribly delayed. What do you think?

[kleisauke]

I agree, let's bump this issue to 8.7. It's just a minor issue that I've encountered.

[kleisauke]

FWIW, there are fewer pixels shifts with commit jcupitt@7db1341 (which uses corner convention for down-sampling).

^{Again, it's a GIF image so horrible quality...}

https://github.com/jcupitt/libvips/issues/705 might be relevant here.

[kleisauke]

I've set-up a separate repo that reproduces this: https://github.com/kleisauke/vips-issue-703.

Output:


^{It's an animated WebP image, the quality should be better now.}

@jcupitt You may bump this to another milestone, debugging this issue is difficult. 😅

[kleisauke]

Quick update: Corner convention seems to produce more pixel shifts. The output can be viewed here:
https://github.com/kleisauke/vips-issue-703#corner-vs-center-convention-portrait

I also added some debugging notes to quickly reproduce this with the command line:
https://github.com/kleisauke/vips-issue-703#debugging-notes

[kleisauke]

This patch seems to partially fix this. Output:

The portrait image seems fixed, but the landscape image still produces some pixel shifts.

[kleisauke]

I tried to disable the JPEG shrink-on-load feature (commit kleisauke/vips-issue-703@f91737d). It seems to produce more pixel shifts when it's disabled, which makes sense because libvips now have to resize the whole image instead of the last 200%.

Shrink-on-load enabled:

Shrink-on-load disabled:

With this patch applied it generates no visual difference whether shrink-on-load is enabled or disabled. Unfortunately, it still produces some small pixel shifts.

Shrink-on-load enabled with patch:

Shrink-on-load disabled with patch:

[kleisauke]

I've made another attempt to fix this issue, and I may have found a proper solution for at least reduceh.

When I looked at ImageMagick's source code to see how they handle the centre convention, I found something interesting:
https://github.com/ImageMagick/ImageMagick6/blob/8ed8ee76dd5fcfc3d182cf00c5696508425176b4/magick/resize.c#L1752-L1755

From the above lines, I assume that the centre convention (+ 0.5) is applied before it's multiplied by the amount of scale. libvips does this after the multiplication, see:

libvips/libvips/resample/reduceh.cpp

Lines 337 to 339 in 10c4831

	X = r->left * reduceh->hshrink;
	if( reduceh->centre )
	X += 0.5;

So, I tried to do the same thing:

@@ -334,9 +324,7 @@ vips_reduceh_gen( VipsRegion *out_region, void *seq,
 
 		q = VIPS_REGION_ADDR( out_region, r->left, r->top + y );
 
-		X = r->left * reduceh->hshrink;
-		if( reduceh->centre )
-			X += 0.5;
+		X = (r->left + offset) * reduceh->hshrink - offset;
 
 		/* We want p0 to be the start (ie. x == 0) of the input 
 		 * scanline we are reading from. We can then calculate the p we

(where offset = 0.5 in case of centre convention)

This seems to solve the problems within reduceh. When I tried to do the same within reducev:

@@ -544,8 +545,8 @@ vips_reducev_gen( VipsRegion *out_region, void *vseq,
 	for( int y = 0; y < r->height; y ++ ) { 
 		VipsPel *q = 
 			VIPS_REGION_ADDR( out_region, r->left, r->top + y );
-		const double Y = (r->top + y) * reducev->vshrink + 
-			(reducev->centre ? 0.5 : 0.0); 
+		const double Y = (r->top + y + offset) * reducev->vshrink -
+			offset;
 		VipsPel *p = VIPS_REGION_ADDR( ir, r->left, (int) Y ); 
 		const int sy = Y * VIPS_TRANSFORM_SCALE * 2;
 		const int siy = sy & (VIPS_TRANSFORM_SCALE * 2 - 1);

It still produces some pixel shifts (unfortunately), see:
https://github.com/kleisauke/vips-issue-703#shrink-an-image-vertically-1

@jcupitt Do you have any ideas? The whole patch can be found here. You can safely ignore the VIPS_DEMAND_STYLE and reduce_sum changes (I tried to align reduceh and reducev).

[jcupitt]

Hello again, I'll have a few hours this weekend, I'll try to have a look then. Well done for digging on this.

[kleisauke]

Let me know if you need any assistance (I could perhaps perform a git bisect). fwiw, I also checked Pillow's source code, and it looks like they're taking a similar approach (with + 0.5 offset):
https://github.com/python-pillow/Pillow/blob/67f309cec8173aff2e069f4683f2861eec3578f6/src/libImaging/Resample.c#L179

But then within the coefficients calculations, so perhaps we'll need to check calculate_coefficients_lanczos:

libvips/libvips/resample/templates.h

Line 404 in 29d9673

calculate_coefficients_lanczos( double *c,

(I tried to change values in there, and I saw an increase in the number of pixel shifts)

Also, the above mentioned patch doesn't work when I change the kernel to VIPS_KERNEL_LINEAR, so the other kernels need to be checked as well:

libvips/libvips/resample/resize.c

Line 398 in c7f98ad

resize->kernel = VIPS_KERNEL_LANCZOS3;

[kleisauke]

I just stopped the git bisect command that ran for a few hours (the full log is available here). I think this bug is present in all versions of libvips starting from commit fa7d593 (where vipsthumbnail was added).

The test script is available here. It's a simple test where the average value is calculated (rounded to one decimal place) after the EXIF orientation image (without labels) is thumbnailed and converted to greyscale. If all images have the same average value, it will exit with 0, otherwise with 125 (which causes git bisect to skip that commit).

[kleisauke]

fwiw, this bug still seems to be present on the master branch. I'm going to check the coefficients calculations to see if I can find any issues.

[kleisauke]

I'm not sure why I didn't test the other resize kernels earlier. VIPS_KERNEL_NEAREST doesn't need any resizing coefficients, so it's a useful way of checking whether the coefficients calculations should also be adapted.

I added this kernel (+ the other resize kernels) with commit kleisauke/vips-issue-703@be734f3 and regenerated the output before and after applying this patch:

VIPS_KERNEL_NEAREST - Portrait - before/after

VIPS_KERNEL_NEAREST - Landscape - before/after

At first sight, it looks like they're all shifting. However, if you look more closely (try to zoom-in 500%) the landscape before image is the only image that is affected by pixel shifts. All other images are not affected. This implicitly means that the (partial) patch is working, but in order to solve this issue completely, a change in coefficients calculations is also necessary.

To be continued (I've some spare time tomorrow).

[kleisauke]

I managed to (partially) fix the cubic, linear and Mitchell kernel with commit kleisauke/vips-issue-703@19b628e.

The idea was to first calculate the kernel radius (rounded to the nearest integer). This integer is then used:

• to calculate the number of points within the kernel (usually 2 * kernel_radius + 1);
• within the coefficients calculations (usually (i - kernel_radius - x) / shrink);
• for adding new pixels to the image before reducing (so we can interpolate at the edges)

The whole patch can be found here. Here are the before/after results for these kernels:

VIPS_KERNEL_LINEAR - before/after

Landscape before:

Landscape after:

Portrait before:

Portrait after:

VIPS_KERNEL_CUBIC - before/after

Landscape before:

Landscape after:

Portrait before:

Portrait after:

VIPS_KERNEL_MITCHELL - before/after

Landscape before:

Landscape after:

Portrait before:

Portrait after:

All portrait images seems to be properly fixed. The landscape images still produce a small pixel shift (unfortunately). This can be clearly spotted if an image is only vertically reduced:
https://raw.githubusercontent.com/kleisauke/vips-issue-703/master/output-patch/Landscape-vertical.webp

TL;DR: the only thing that needs to be resolved is the small pixel shift within reducev.

[jcupitt]

This should be fixed in 8.10. I'll close.