Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

color tests failing on s390x: QuantumType is "float_t" for HDRI=1 and can be float or double depending on arch and compiler #504

Closed
mhillenbrand opened this issue Nov 16, 2020 · 9 comments
Milestone

Comments

@mhillenbrand
Copy link

When using an Imagemagick compiled with HDRI=1, many color-related test cases fail.

tests/color_test.py ..............FFFFFFFFFFFFFFFF...s...F
...
    def test_red_quantum():                                                                                                                                                                                                                                   
        q = 2 ** QUANTUM_DEPTH - 1                             
        assert Color('black').red_quantum == 0                                                                                                                                                                                                                
>       assert Color('red').red_quantum == q                                                                                   
E       AssertionError: assert 7.4999847412109375 == 65535                                                                     
E        +  where 7.4999847412109375 = wand.color.Color('srgb(255,0,0)').red_quantum                                           
E        +    where wand.color.Color('srgb(255,0,0)') = Color('red')                                                           
                                                                                                                               
tests/color_test.py:135: AssertionError

The root cause is that python-wand assumes that QuantumType would be float or double when using an HDRI variant of the ImageMagick libraries, see

QuantumType = c_float if IM_HDRI else c_ubyte

However, with HDRI=1 ImageMagick derives QuantumType from float_t (for 8-bit and 16-bit color depths) and double_t (above that). Those types can differ from float and double, depending on your architecture and compiler settings. For example, on 32-bit x86, float_t would become double when compiling with -ffp-math=x87; on s390x, float_t today is defined as double. Thus, the binary libraries double values don't match python-wand's float.

When I modify pixel_wand.py to set QuantumType = c_float, these tests pass.

@emcconville
Copy link
Owner

Is there a way for ctypes to identify this use-case? I would guess sizeof(c_float) == sizeof(c_double), but without access to the architecture - I'm in the dark.

What's the output of...

from ctypes import *

print('pointer', sizeof(c_void_p))
print('float', sizeof(c_float))
print('double', sizeof(c_double))

I'm also wondering if size_t might have issues.

@emcconville
Copy link
Owner

Essentially, we just need to resolve the following preprocessor

#if MAGICKCORE_SIZEOF_FLOAT_T == 0
typedef float MagickFloatType;
#elif (MAGICKCORE_SIZEOF_FLOAT_T == MAGICKCORE_SIZEOF_FLOAT)
typedef float MagickFloatType;
#elif (MAGICKCORE_SIZEOF_FLOAT_T == MAGICKCORE_SIZEOF_DOUBLE)
typedef double MagickFloatType;
#elif (MAGICKCORE_SIZEOF_FLOAT_T == MAGICKCORE_SIZEOF_LONG_DOUBLE)
typedef double MagickFloatType;
#else
#error Your MagickFloatType type is neither a float, nor a double, nor a long double
#endif
#if MAGICKCORE_SIZEOF_DOUBLE_T == 0
typedef double MagickDoubleType;
#elif (MAGICKCORE_SIZEOF_DOUBLE_T == MAGICKCORE_SIZEOF_DOUBLE)
typedef double MagickDoubleType;
#elif (MAGICKCORE_SIZEOF_DOUBLE_T == MAGICKCORE_SIZEOF_LONG_DOUBLE)
typedef double MagickDoubleType;
#else
#error Your MagickDoubleType type is neither a float, nor a double, nor a long double
#endif

@mhillenbrand
Copy link
Author

mhillenbrand commented Nov 16, 2020

Unfortunately, sizeof(c_float) does not help -- float is still a 32-bit fp, even though float_t is a 64-bit just like double.

your snippet

from ctypes import *

print('pointer', sizeof(c_void_p))
print('float', sizeof(c_float))
print('double', sizeof(c_double))

produces

pointer 8
float 4
double 8

Compare to C:

#include <math.h>

int main() {
  printf("pointer %zd", sizeof(void *));
  printf("float %zd", sizeof(float));
  printf("float_t %zd", sizeof(float_t));
  printf("double %zd", sizeof(double));
}
---
pointer 8
float 4
float_t 8  <- this is the one
double 8

The part of the header that you quoted needs additional input from magick-baseconfig.h, which is filled at compile-time:

/* The size of `float_t', as computed by sizeof. */                                                                                                                                                                                                           
#ifndef MAGICKCORE_SIZEOF_FLOAT_T                                                                                                                                                                                                                             
#define MAGICKCORE_SIZEOF_FLOAT_T 8                                                                                                                                                                                                                           
#endif

@mhillenbrand
Copy link
Author

The Linux distributions I looked at do not ship that header file only in the "development" packages for ImageMagick's libraries, not in the normal binary-only library packages. So, that would mean more dependencies when parsing at run-time.

@emcconville
Copy link
Owner

Hmm.. Thanks for post this. The best thing I can think of is testing platform.machine(). But I'm not 100% sure how reliable that check would be. Might be worth digging around other ctypes applications to see how they evaluate the size of float_t.

emcconville added a commit that referenced this issue Nov 17, 2020
@emcconville
Copy link
Owner

emcconville commented Nov 17, 2020

@mhillenbrand please review pull request #505. If you're able to run & confirm fix on hardware, I should be able to include the patch with Wand 0.6.4 release.

@mhillenbrand
Copy link
Author

Thanks for preparing that patch. I did test on hardware and could verify that it addresses the issue. Found a typo that needs fixing + minor suggestion in PR.

emcconville added a commit that referenced this issue Nov 17, 2020
@emcconville emcconville added this to the Wand 0.6.4 milestone Nov 17, 2020
@emcconville
Copy link
Owner

Closing this issue. Thanks for reporting this & following up with testing!

@mhillenbrand
Copy link
Author

Cool, thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants