We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3694

Summary: SDL_image fails to compile with the latest NDK (r15)
Product: SDL_image Reporter: Olli Kallioinen <olli.kallioinen>
Component: miscAssignee: 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
Compiling SDL_image fails with the latest NDK(r15) with an error:

  SDL_image/external/jpeg-9/jidctfst.S:17:10:
  fatal error:'machine/cpu-features.h' file not found


I was thinking that maybe it was an issue with the new NDK, but the NDK developers confirmed that the header was removed on purpose:
https://github.com/android-ndk/ndk/issues/443
Comment 1 Sylvain 2017-09-11 18:52:40 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 ***
Comment 2 Olli Kallioinen 2017-09-13 15:20:59 UTC
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).
Comment 3 Sylvain 2017-09-13 16:26:57 UTC
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 :/
Comment 4 Sylvain 2017-09-13 16:33:44 UTC
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"
Comment 5 Sam Lantinga 2017-09-13 16:50:00 UTC
Reopening...
Comment 6 Sam Lantinga 2017-09-13 16:50:33 UTC
Sylvain, can you look at creating the proper fix?

Thanks!
Comment 7 Sylvain 2017-09-13 19:57:21 UTC
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 ?
Comment 8 Sylvain 2017-09-18 15:10:22 UTC
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).
Comment 9 Sam Lantinga 2017-09-18 23:11:12 UTC
I accepted the patch to remove the assembly version, since it's broken:
https://hg.libsdl.org/SDL_image/rev/7ad06019831d

Thanks!
Comment 10 Sylvain 2017-10-23 10:16:46 UTC
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
Comment 11 Sam Lantinga 2017-10-23 14:41:35 UTC
Fixed, thanks!
https://hg.libsdl.org/SDL_image/rev/4009a3c9612b