That's an endianness problem fixed quite a while ago:

https://bitbucket.org/pygame/pygame/issues/39

But the fix seems to be in pygame 1.9.2 only, afaics.

*** Bug 1392887 has been marked as a duplicate of this bug. ***

(In reply to Michael J Gruber from comment #1)
> That's an endianness problem fixed quite a while ago:
> 
> https://bitbucket.org/pygame/pygame/issues/39
> 
> But the fix seems to be in pygame 1.9.2 only, afaics.

That is about a big-endian issue, where as here we've a failure on ppc64le, so little-endian and the test works fine on x86 so it is not just an endianness issue.

Maybe it's always treating ppc64 as big endian ...

(In reply to Dan Horák from comment #4)
> Maybe it's always treating ppc64 as big endian ...

I've checked pygame, SDL, SDL_image and libpng for something like this (using case insensitive grep for PPC / POWERPC) and I could not find anything like this.

This bug appears to have been reported against 'rawhide' during the Fedora 26 development cycle.
Changing version to '26'.

Updated to 1.9.3, and this still happens.

It appears that pygame folks could need a helping hand for testing on big endian to confirm that the proposed fix works:

https://bitbucket.org/pygame/pygame/issues/211/red-and-green-channels-swapped-when-saving

(It turned out that the old fix from issue 39 was not quite correct.)

The issue #211 talks about incomplete big endian fix, which is now long time committed. Our problem is not on big endians (ppc64, s390x), it's all OK there, but with the little endian flavour of ppc (aka ppc64le).

Created attachment 1307110 [details]
vector test code

I looked to the failed test on ppc64le and I traced a strange behaviour of vector used in SDL.
SavePNG() function of pygame somewhere reach ConvertAltivec32to32_prefetch() function of SDL.

ConvertAltivec32to32_prefetch() of SDL-1.2.15/src/video/SDL_blit_N.c
uses altivec vectors thru this code:
            src += 4;
            width -= 4;
            vbits = vec_perm(vbits, voverflow, valigner);  /* src is ready. */
            vbits = vec_perm(vbits, vzero, vpermute);  /* swizzle it. */
            vec_st(vbits, 0, dst);  /* store it back out. */
            dst += 4;
            vbits = voverflow;


I am puzzled by the behaviour of vec_perm where it don't return the result I expect.

I highlighted the suspect behaviour of vec_perm in ppc64le by writting a test code: test.c (see attachment)

I compiled it with the following options (same as those used to build SDL).
gcc -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mcpu=power8 -mtune=power8 -I./include -D_GNU_SOURCE=1 -fvisibility=hidden -maltivec -pthread -I/usr/include/kde/artsc -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -D_REENTRANT -DXTHREADS -D_REENTRANT -DHAVE_LINUX_VERSION_H -Wall test.c  -fPIC -DPIC -o test

When I ran ./test, I got:
 vperm[0] 0
 vperm[0] 3
 vperm[0] 2
 vperm[0] 1
 vbits[0] 0x112233
 resulting vbits[0] 0x22110033
 vbits[0] expected 0x00332211

Do you agree vec_perm does not return the correct result in this case ?

packages version:
pygame-1.9.3-1
SDL-1.2.15-24
gcc-7.1.1-6
glibc-2.25.90-29

Bill Schmidt commented this behaviour as follow:
The LE transformation for vec_perm has an implicit assumption that the permutation is being used to reorder vector elements (in this case 4-byte integer word elements), not to reorder bytes within those elements.  Although this is legal behavior, it is not anticipated by the transformation performed by the compilers.  (CCing reps from XLC and LLVM compilers for awareness.)

Note that if vec1 and vzero are vector char constants instead, the expected behavior is maintained (see attached test2.c rewritten from Guy's test.c).

To understand the difference, the vector int version of vec1 looks like this in a hardware register:

    BE:  00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff    
    LE:  cc dd ee ff 88 99 aa bb 44 55 66 77 00 11 22 33

But the vector char version looks like this:

    BE:  00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff
    LE:  ff ee dd cc bb aa 99 88 77 66 55 44 33 22 11 00

In the latter case, the compiler can generate correct LE code by a simple modification of the BE permute control vector ("vperm" in Guy's test.c) and a reordering of the two input vectors.  That is what all of the compilers do today.  It is not so simple to deal with an arbitrary reordering of bytes within vector elements.

I infer that the goal in test.c is to swap the second and fourth channels of a pixel representation, but instead we are swapping the first and third channels.  It is not so clear to me what the goal of "swizzle it" in ConvertAltivec32to32_prefetch() is without being able to see the "valigner" and "vpermute" vectors, though I would guess it is doing the same thing.  If that is so, then I would recommend making the following source change to the SDL library function:

#if __LITTLE_ENDIAN__
  vpermute = { 2, 1, 0, 3, 2, 1, 0, 3, 2, 1, 0, 3, 2, 1, 0, 3 };
#else
  vpermute = { 0, 3, 2, 1, 0, 3, 2, 1, 0, 3, 2, 1, 0, 3, 2, 1 };
#endif

As shown in the attached test3.c, this will produce the correct result for LE.  (Reason: Numbering from the left for BE, and numbering from the right for LE, produces a different interpretation of what the odd and even lanes are.)

Created attachment 1322598 [details]
correct result with vector char

Created attachment 1322599 [details]
reorder bytes

Created attachment 1322601 [details]
proposed patch

I get the test ok when building pygame-1.9.3-5 with this patch applied on SDL-1.2.15-28.

Hi , I don't know is related but I think it worth to mention [1] , on opencl-heards we also got and issue with  vector , on ppc64le we should use __verctor instead vector ... 


[1] 
https://bugzilla.redhat.com/show_bug.cgi?id=1487174

(fixing typos sorry) 
Hi , I don't know if it is related but I think it worth to mention [1] , on opencl-heards we also got an issue with vector, on ppc64le we should use __verctor instead vector ...

I still don't understand why this is specific to PowerPC. To me it seems like a bug in PPC64LE GCC's implementation. See this LLVM commit <http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140602/107034.html> that reorders last argument so that the vec_perm() retains it's platform-specific endianity while it is implemented on top of AlitVec instructions that has big-endian semantics even on PPC64LE.

Shouldn't this be fixed in GCC instead?

All the POWER compilers (GCC, LLVM, XLC) have the same implementation of vec_perm for little endian.  There is an implicit assumption that when you specify vec_perm of vector ints, that you want to reorder the vector ints, not reorder the bytes within a vector int.  The SDL code is using each int in a vector int to represent four individual bytes of data, which confuses the compiler's algorithm for converting the permute vector to perform an LE transformation.  This was not anticipated in the design of the vec_perm transformation agreed to by all the compilers, and we will have to do some re-design work in the future to catch these cases.  As this is unlikely to happen quickly, I've recommended a source change to handle this particular unexpected usage.

And by the way, to be clear:  I am not fully convinced yet that there even is a transformation that can anticipate this usage in all cases.  When we can determine the permute control vector is a constant, analyzing it is possible.  If it is constructed in another way, there is no way to detect this kind of nonstandard behavior.

Bill , but seems that use __vector instead vector fix the problem [1], I hadn't test in  SDL-1.2  because code have more than 600 vector declarations .


[1]
https://anonscm.debian.org/cgit/pkg-opencl/khronos-opencl-headers.git/tree/debian/patches/use__vector.patch

https://github.com/openembedded/meta-openembedded/commit/97dfbbbf6bddbd360638cde98cb53790ccc83059 

https://github.com/KhronosGroup/OpenCL-Headers/pull/20

(In reply to Sergio Monteiro Basto from comment #20)

Replacing vector with __vector cannot fix the problem with usage of vec_perm.

Furthermore, replacing vector with __vector should not be necessary.  This is papering over a bug in the version of Clang that you are using.  You should work with the Clang/LLVM PowerPC community to figure out why that appears to be needed.  This certainly didn't used to be required.

(In reply to Bill Schmidt from comment #18)
> There is an implicit assumption that when you
> specify vec_perm of vector ints, that you want to reorder the vector ints,
> not reorder the bytes within a vector int.  The SDL code is using each int
> in a vector int to represent four individual bytes of data, which confuses
> the compiler's algorithm for converting the permute vector to perform an LE
> transformation.

Then shouldn't the SDL code be changed to use vector unsigned char instead?

Nevertheless thank you for the patch. I will apply it to the Fedora's SDL. In worst case it will break PPC64LE only that already seems to be broken. We also cannot count with upstream's help because upstream abandoned SDL in favor of SDL2, he does not welcome any bug reports in their Bugzilla instance and they closed mailing list with recommendation to use Discourse web instead.

SDL-1.2.15-29.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-1c27e533b2

SDL-1.2.15-25.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-f94a70fb4e

(In reply to Bill Schmidt from comment #21)
> (In reply to Sergio Monteiro Basto from comment #20)
> 
> Replacing vector with __vector cannot fix the problem with usage of vec_perm.
> 
> Furthermore, replacing vector with __vector should not be necessary.  This
> is papering over a bug in the version of Clang that you are using.  You
> should work with the Clang/LLVM PowerPC community to figure out why that
> appears to be needed.  This certainly didn't used to be required.

Sorry my mistake/confusion, OK but this is an issue with gcc and I can't let notice another strange error around vector, in openCL when compilation of opencv in ppc64le

/usr/include/CL/cl_platform.h:390:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector unsigned char     __cl_uchar16;

what we are missing here , if you can help ? 

[1] 
https://copr-be.cloud.fedoraproject.org/results/sergiomb/opencv/fedora-27-ppc64le/00604161-opencv/build.log.gz (attention all lines are duplicated)

https://copr.fedorainfracloud.org/coprs/sergiomb/opencv/build/604161/

What does "/usr/bin/c++ -v" show?  I don't know what version of the compiler you're using.

I've double-checked that "typedef vector unsigned char __cl_uchar16;" works fine on recent GCC.  One guess is that the CL code may have done a "#undef vector" at some point; vector is a predefined macro that can be removed in this fashion, in which case only __vector is available.  In this case your workaround would be needed.

This is not uncommon in C++ code where "vector bool" collides with the use of "bool" as a keyword in the language.  Some headers just #undef vector, bool, and pixel to get rid of all the AltiVec predefines.

So all in all I would suggest you go ahead with your workaround.

Sorry for the confusion about Clang; I misunderstood what you were building and how.

Doesn't typedef vector unsigned char __cl_uchar16; only work for -std=gnu++* modes?  For -std=c++* or -ansi it either isn't defined at all, or when altivec.h is included and #undef vector isn't done, then it is totally incompatible with libstdc++ headers.

      if (!flag_iso)
        {
          builtin_define ("vector=vector");
          builtin_define ("pixel=pixel");
          builtin_define ("bool=bool");
          builtin_define ("_Bool=_Bool");
          init_vector_keywords ();

          /* Enable context-sensitive macros.  */
          cpp_get_callbacks (pfile)->macro_to_expand = rs6000_macro_to_expand;
        }

(In reply to Jakub Jelinek from comment #28)

Oh, yes.  I had forgotten about this.  If you are building with -ansi, etc., then you will miss these predefines.  However, I don't see a standards directive on the compile command, and I thought -std=gnu++<something> was the default.  I might be wrong about that.  If you add -std=gnu++11 to the command line, does it succeed?

(In reply to Bill Schmidt from comment #26)
> What does "/usr/bin/c++ -v" show?  I don't know what version of the compiler
> you're using.

from build.log.gz :) 
-- The CXX compiler identification is GNU 7.1.1 
-- The C compiler identification is GNU 7.1.1

c++ -v
Using built-in specs.
COLLECT_GCC=/usr/bin/c++
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/7/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --enable-libmpx --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 7.1.1 20170802 (Red Hat 7.1.1-7) (GCC) 


(In reply to Bill Schmidt from comment #27)
> I've double-checked that "typedef vector unsigned char __cl_uchar16;" works
> fine on recent GCC.  One guess is that the CL code may have done a "#undef
> vector" at some point; vector is a predefined macro that can be removed in
> this fashion, in which case only __vector is available.  In this case your
> workaround would be needed.
> 
> This is not uncommon in C++ code where "vector bool" collides with the use
> of "bool" as a keyword in the language.  Some headers just #undef vector,
> bool, and pixel to get rid of all the AltiVec predefines.

Hum , maybe I will investigate , I don't have much time now ... 

> So all in all I would suggest you go ahead with your workaround.
> 
> Sorry for the confusion about Clang; I misunderstood what you were building
> and how.

no problem at all , many thanks for your reply .
Cheers. 
(In reply to Bill Schmidt from comment #29)
> (In reply to Jakub Jelinek from comment #28)
> 
> Oh, yes.  I had forgotten about this.  If you are building with -ansi, etc.,
> then you will miss these predefines.  However, I don't see a standards
> directive on the compile command, and I thought -std=gnu++<something> was
> the default.  I might be wrong about that.  If you add -std=gnu++11 to the
> command line, does it succeed?

I will try now

SDL-1.2.15-29.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-1c27e533b2

SDL-1.2.15-25.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-f94a70fb4e

(In reply to Bill Schmidt from comment #29)
> (In reply to Jakub Jelinek from comment #28)
> 
> Oh, yes.  I had forgotten about this.  If you are building with -ansi, etc.,
> then you will miss these predefines.  However, I don't see a standards
> directive on the compile command, and I thought -std=gnu++<something> was
> the default.  I might be wrong about that.  If you add -std=gnu++11 to the
> command line, does it succeed?

same thing [1]

/usr/include/CL/cl_platform.h:390:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector unsigned char     __cl_uchar16;

/usr/include/CL/cl_platform.h:391:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector signed char       __cl_char16;

/usr/include/CL/cl_platform.h:392:12: error: 'vector' does not name a type; did you mean 'vec_or'?
    typedef vector unsigned short    __cl_ushort8;

etc 

[1]
https://copr-be.cloud.fedoraproject.org/results/sergiomb/opencv/fedora-26-ppc64le/00606906-opencv/build.log.gz

https://copr-be.cloud.fedoraproject.org/results/sergiomb/opencv/fedora-26-ppc64le/00606906-opencv/

https://copr.fedorainfracloud.org/coprs/sergiomb/opencv/build/606906/

SDL-1.2.15-25.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

SDL-1.2.15-29.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.

Created attachment 1332750 [details]
0001-Re-enable-tests-on-ppc64le.patch

Built successfully after re-enable tests on ppc64le. 

https://koji.fedoraproject.org/koji/taskinfo?taskID=22166379

tests fail on ppc64le because vec_perm() is fixed closing the bug