vips_thumbnail produces meaningless image on s390x

[mtasaka]

Using released libvips v8.18.0, https://github.com/libvips/libvips/releases/tag/v8.18.0
at least vips_thumbnail (just tried this) produces meaningless image on s390x.

Note that s390x is big endian.

C source:
$ cat resize-to-limit-2.c

#include <stdlib.h>
#include <sys/stat.h>

#define MY_ASSERT(expr) \
do { \
	if (!(expr)) { \
		fprintf(stderr, "LINE[%d] : assertion \"" #expr "\" failed.\n", __LINE__); \
		abort(); \
	} \
} while(0)


int main(void) {
	VipsImage *normal_image;
	int ret;

	const char *input_path = "./portrait.jpg";

	ret = vips_thumbnail(
			input_path, &normal_image,
			400,
			"height", 400,
			"size", VIPS_SIZE_DOWN,
			NULL
		);
	MY_ASSERT(!ret);

	vips_image_write_to_file(normal_image, "resized.jpg", NULL);
}

[mtasaka]

Original portrait.jpg

[mtasaka]

result resized.jpg on x86_64 (and actually on i686, ppc64le, aarch64: all same)

[mtasaka]

On the other hand, result resized.jpg on s390x

[kleisauke]

What does the output of vips --targets show? Does setting the VIPS_NOVECTOR environment variable prevent this?

[mtasaka]

on s390x:

$ vips --targets
builtin targets:   SCALAR
supported targets: Z15 Z14

And yes, env VIPS_NOVECTOR=1 seems to suppress this issue.

[mtasaka]

(Note that this test is extracted from https://github.com/janko/image_processing/blob/8185ea61518d8cf477594505cd4b5ab478b28aea/test/vips_test.rb#L179 and https://github.com/janko/image_processing/blob/8185ea61518d8cf477594505cd4b5ab478b28aea/test/fixtures/portrait.jpg )

[kleisauke]

Thanks for the info. It looks like libvips should use the HWY_SUPPORTED_TARGETS marco instead of hwy::SupportedTargets() to ensure it will fallback to the C path if only a single fallback emulation target is enabled.

Candidate patch:

diff --git a/libvips/iofuncs/vector.cpp b/libvips/iofuncs/vector.cpp
index e1c3119d3..1c23001e0 100644
--- a/libvips/iofuncs/vector.cpp
+++ b/libvips/iofuncs/vector.cpp
@@ -141,7 +141,7 @@ gint64
 vips_vector_get_supported_targets()
 {
 #ifdef HAVE_HWY
-	return hwy::SupportedTargets() & ~(HWY_EMU128 | HWY_SCALAR);
+	return HWY_SUPPORTED_TARGETS & ~(HWY_EMU128 | HWY_SCALAR);
 #elif defined(HAVE_ORC)
 	return orc_target_get_default_flags(orc_target_get_default());
 #else

And:

diff --git a/meson.build b/meson.build
index 7b3705a33..721733359 100644
--- a/meson.build
+++ b/meson.build
@@ -491,6 +491,8 @@ if libhwy_dep.found()
     endif
     # Optionally, build without AVX512 support (helps to reduce binary size at the cost of performance)
     #disabled_targets += ['HWY_AVX3']
+    # Ensure fallback emulation targets are always disabled
+    disabled_targets += ['HWY_EMU128', 'HWY_SCALAR']
     add_project_arguments('-DHWY_DISABLED_TARGETS=@0@'.format('|'.join(disabled_targets)), language: ['cpp'])
     simd_package = libhwy_dep
 endif

Though, the second patch is a bit risky due to this code:
https://github.com/google/highway/blob/1.3.0/hwy/detect_targets.h#L688-L690
(which was later relaxed in commit google/highway@f146510)

[kleisauke]

Actually, it seems there's just a missing mask there. I just opened PR #4816 for this.

Is this for Fedora? I could try to include that patch in https://src.fedoraproject.org/rpms/vips/pull-request/16.