| Summary: | SDL_image fails to compile with the latest NDK (r15) | ||
|---|---|---|---|
| Product: | SDL_image | Reporter: | Olli Kallioinen <olli.kallioinen> |
| Component: | misc | Assignee: | Sylvain <sylvain.becker> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | major | ||
| Priority: | P2 | CC: | sylvain.becker |
| Version: | unspecified | ||
| Hardware: | ARM | ||
| OS: | Android (All) | ||
| Attachments: |
Patch
Patch to remove the assembly version |
||
|
Description
Olli Kallioinen
2017-07-05 22:45:05 UTC
this is a duplicated of bug 3108 https://hg.libsdl.org/SDL_image/rev/8402819e7bb2 *** This bug has been marked as a duplicate of bug 3108 *** Are you sure this is a duplicate? I pulled the latest changes and I'm still getting the same compiler error (now just on armeabi-v7a). sorry, you're right, this is not a duplicated and something need to be done ! I quickly tried: to revert: https://hg.libsdl.org/SDL_image/rev/8402819e7bb2 (because otherwise, arch = x86 would try to compile the arm code) and remove the include <machine/cpu-features.h> from "external/jpeg-9/jidctfst.S" And that compiled and seemed to work :/ in fact it may work, but not really as expected. It won't have the correct #if directive like "#if __ARM_HAVE_HALFWORD_MULTIPLY" for instance. I'd say, just remove the source file "jidctfst.S" and force the use of "jidctfst.c" Reopening... Sylvain, can you look at creating the proper fix? Thanks! Created attachment 2933 [details] Patch Ok. So this is not the patch I prefer, but since cpu-features.h is accessible or not regarding the ndk version, let's just copy-paste it inside the assembler code. I also reverted the previous Android.mk to match what is in external/jpeg-9/Android.mk. It compiles for all arch, with ndk r13/15/16. Moreover: Instead of duplicating the Android.mk, it could just refer the other : see bug 2051. (same was applied for SDL_ttf and freetype). Maybe, later, we should update the jpeg-9 to latest version ? is this https://github.com/android/platform_external_jpeg ? Created attachment 2947 [details]
Patch to remove the assembly version
Some update as I looked more closely on this.
The file jidctfst.S contains an assembly version of the fast integer inverse-DCT.
the file jidctfst.c contains a C version of this "fast" fonction.
It also exists C version "slow", which is more accurate.
SDL_images use by default the slow version. JPEG default DCT method is also to use the slow function.
To use the fast IDCT (C or Assembly), one needs to enable the compile flag in SDL IMG_jpeg.c FAST_JPEG which set dct_method to "JDCT_FASTEST".
I enabled the FAST_JPEG and used the assembly implementation. And it just did not work: images are not decoded correctly and totally screwed up.
Tried with ABI arm and armv7a, with clang and gcc. same result.
I also tried a small benchmark loading tens of images on a device. When the slow version performs in 430 ms, the C-fast version is doing that 400ms, and the assembly-fast version is doing 390. which is not that much better.
So I think, we could just allow the C fast version (which is working), for people who would enable FAST_JPEG in SDL2_image.
Here's a patch that modify Android.mk (and also the one in external/jpeg-9).
I accepted the patch to remove the assembly version, since it's broken: https://hg.libsdl.org/SDL_image/rev/7ad06019831d Thanks! Re-opening, since jpeg is updated to jpeg-9b, we have the issue again that with newer ndk, "machine/cpu-features.h" has been removed.
So let's disable assembly:
(and also sources were included twice)
diff -r e747a2cd2d83 external/jpeg-9b/Android.mk
--- a/external/jpeg-9b/Android.mk Sun Oct 22 19:13:34 2017 -0700
+++ b/external/jpeg-9b/Android.mk Mon Oct 23 12:02:01 2017 +0200
@@ -10,7 +10,7 @@
jdapistd.c jdarith.c jdatadst.c jdatasrc.c jdcoefct.c jdcolor.c \
jddctmgr.c jdhuff.c jdinput.c jdmainct.c jdmarker.c jdmaster.c \
jdmerge.c jdpostct.c jdsample.c jdtrans.c jerror.c jfdctflt.c \
- jfdctfst.c jfdctint.c jidctflt.c jidctfst.c jidctint.c jquant1.c \
+ jfdctfst.c jfdctint.c jidctflt.c jquant1.c \
jquant2.c jutils.c jmemmgr.c \
jmem-android.c
@@ -20,7 +20,7 @@
endif
# temp fix until we understand why this broke cnn.com
-#ANDROID_JPEG_NO_ASSEMBLER := true
+ANDROID_JPEG_NO_ASSEMBLER := true
ifeq ($(strip $(ANDROID_JPEG_NO_ASSEMBLER)),true)
LOCAL_SRC_FILES += jidctint.c jidctfst.c
Fixed, thanks! https://hg.libsdl.org/SDL_image/rev/4009a3c9612b |