test_png.test_pngsuite.test fails on ppc64 (big-endian)

[AdamWill]

Recently I've been looking at errors in the matplotlib test suite when building on Fedora's ppc64 arch, which is big-endian. I've sent a few PRs so far, but there are still some remaining issues, so I'm filing tickets for them.

This ticket is for a failure of the test_png.test_pngsuite.test test. In my ppc64 build, this is the image produced by the test:

Here's the expected image:

The fifth, seventh, thirteenth and fifteenth images clearly differ. Haven't yet dug into why this is.

There is another png-related test that fails, the two may share a common cause:

======================================================================
FAIL: matplotlib.tests.test_png.test_imread_png_uint16
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/builddir/build/BUILDROOT/python-matplotlib-2.0.0-0.7.rc2.fc26.ppc64/usr/lib64/python2.7/site-packages/matplotlib/tests/test_png.py", line 49, in test_imread_png_uint16
    assert np.sum(img.flatten()) == 134184960
AssertionError

[AdamWill]

@QuLogic

[AdamWill]

The incorrect answer it gets for the np.sum assertion is 126351360 . No idea what that means as of yet.

[AdamWill]

The four images that are incorrectly rendered are all the 16-bit images. All the images that render correctly are 1, 2, 4 or 8-bit. There's a few if (bit_depth == 16) clauses in src/_png.cpp; I bet that's where we should be looking...

edit: aha! If I just comment out this block:

// Convert big endian to little
if (bit_depth == 16) {
    png_set_swap(png_ptr);
}

it makes the tests pass (both of them). Obviously just unconditionally commenting it out isn't a true fix (it'll break LE arches); I guess we need to conditionalize the bit swap to only happen on LE arches. Now to figure out how to do that...

[QuLogic]

I see a png_set_swap, but it doesn't appear to check if it's a necessary thing. I think that function also doesn't do that check either. Not sure if we have an existing test for endianness.

[AdamWill]

snap ;)

[tacaswell]

Are there regressions from 1.5.x or have we always been broken on BE systems?

[AdamWill]

@tacaswell I don't know for sure myself - I only ran into this more or less by accident while doing drive-by Fedora package rebuilds, I didn't know thing one about matplotlib until two days ago. I don't even own any ppc64 systems, I'm testing this in a sandbox container Fedora releng gave me for the purpose.

Some of the font stuff that was broken was definitely new in 2.0, I did notice that, but I think some of the other stuff has been around longer.

[QuLogic]

@tacaswell I think the font problems are new in 2.0.0 as the commit that @AdamWill referenced in his first PR was not in 1.5.3, and it seems like there was a bit of a rewrite there.

Not sure about png, but it did have the CXX removal recently.

[AdamWill]

So, really not sure what the best way to go about implementing an endianness check would be. One approach I thought of: you can get the current system's endianness trivially from Python - sys.byteorder gives it. So you could presumably tweak the Python bits (setupext.py, I think) that configure the build process for the C++ bits to define a macro that we can use in _png.cpp to decide whether to do the conversion. But I don't know if that's a good way, really. It wouldn't work for cross-compiling, dunno if that's important.

Googling around for endianness checks gives you all kinds of suggestions and all kinds of reasons why they're all wrong and broken and terrible, so I'm a bit lost as to what the best choice would be...

[QuLogic]

It looks like we have NumPy available there, so maybe PyArray_EquivByteorders will work.

[AdamWill]

Aha, yeah, that sounds promising. On a quick look, numpy's check uses the BYTE_ORDER setting from endian.h if it's present (which it is on many, but not all platforms); if it's not, it has a hard coded list of target CPUs with known endianness. That seems to be about as sane an approach as any. I'll try writing a patch which uses PyArray_EquivByteorders, thanks.

[AdamWill]

So this is fixed now.