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

upgrade our vendored libjxl to v0.7.2, 0.8.4 or 0.9.4? #499

Open
sezero opened this issue Jan 5, 2025 · 18 comments
Open

upgrade our vendored libjxl to v0.7.2, 0.8.4 or 0.9.4? #499

sezero opened this issue Jan 5, 2025 · 18 comments

Comments

@sezero
Copy link
Contributor

sezero commented Jan 5, 2025

Should we upgrade our vendored libjxl to v0.7.2, 0.8.4 or 0.9.4? (0.10.x
or 0.11.x would be too new - our tests are failing with those releases.)

The mainstream doesn't seem to be maintaining 0.6.x, the newer branches
received security fixes in late Nov. 2024.

For distros: Fedora EPEL (Extra Packages for Enterprise Linux) 8 and 9
are at 0.7.0, so is Debian stable.

If we do this though, Xcode (and possibly MSVC) projects will need some
surgery.

@madebr
Copy link
Contributor

madebr commented Jan 6, 2025

Do you track the state of all dependencies? I'd prefer to update all dependencies of all satellite libraries in one go every x months/years.

@slouken
Copy link
Collaborator

slouken commented Jan 6, 2025

We probably should, and we should probably track what's in distributions. It looks like Debian stable is at 0.7 and debian unstable is at 0.10. I'm concerned that 0.10+ has different image output, but realistically we should probably jump to that and update the tests because that's what the developers are working on and what will be in the next stable distributions.

@sezero
Copy link
Contributor Author

sezero commented Jan 6, 2025

Do you track the state of all dependencies? I'd prefer to update all dependencies of all satellite libraries in one go every x months/years.

Once in a while, yes, and if I'm aware of some important update in a 3rd party lib which is our dependency, then I update it. I don't usually upgrade though: E.g. for SDL2, I've been keeping libwebp at 1.0.3 (with updates / security fixes applied) and libtiff at 4.2.0 (the same way): no real need for an upgrade. For SDL3 though, I'd usually prefer latest maintained version if it were up to me.

We probably should, and we should probably track what's in distributions. It looks like Debian stable is at 0.7 and debian unstable is at 0.10. I'm concerned that 0.10+ has different image output, but realistically we should probably jump to that and update the tests because that's what the developers are working on and what will be in the next stable distributions.

For SDL3_image, upgrading to 0.11.x is reasonable I think. For SDL2_image, I don't know.

Either way, libjxl's own dependencies are monsters to deal with as they are. Won't be pretty I guess.

@slouken
Copy link
Collaborator

slouken commented Jan 6, 2025

Do you track the state of all dependencies? I'd prefer to update all dependencies of all satellite libraries in one go every x months/years.

Once in a while, yes, and if I'm aware of some important update in a 3rd party lib which is our dependency, then I update it. I don't usually upgrade though: E.g. for SDL2, I've been keeping libwebp at 1.0.3 (with updates / security fixes applied) and libtiff at 4.2.0 (the same way): no real need for an upgrade. For SDL3 though, I'd usually prefer latest maintained version if it were up to me.

Agreed, let's go ahead and do that.

We probably should, and we should probably track what's in distributions. It looks like Debian stable is at 0.7 and debian unstable is at 0.10. I'm concerned that 0.10+ has different image output, but realistically we should probably jump to that and update the tests because that's what the developers are working on and what will be in the next stable distributions.

For SDL3_image, upgrading to 0.11.x is reasonable I think. For SDL2_image, I don't know.

Either way, libjxl's own dependencies are monsters to deal with as they are. Won't be pretty I guess.

Let's leave it alone for SDL2_image and upgrade to latest in SDL3_image.

@sezero
Copy link
Contributor Author

sezero commented Jan 6, 2025

upgrade to latest in SDL3_image.

I created a v0.11.1-SDLbranch in our libjxl fork. The cmake'ry and rest would probably be handled better by @madebr.

Let's leave it alone for SDL2_image

Not 100% comfortable with staying at 0.6.1, but really OK.

@slouken
Copy link
Collaborator

slouken commented Jan 6, 2025

upgrade to latest in SDL3_image.

I created a v0.11.1-SDLbranch in our libjxl fork. The cmake'ry and rest would probably be handled better by @madebr.

Let's leave it alone for SDL2_image

Not 100% comfortable with staying at 0.6.1, but really OK.

Feel free to upgrade it to 0.7 if you want.

@sezero
Copy link
Contributor Author

sezero commented Jan 6, 2025

Feel free to upgrade it to 0.7 if you want.

Created a v0.7.2-SDL branch for it

@sezero
Copy link
Contributor Author

sezero commented Jan 6, 2025

Updated brotli and highway submodules in both v0.7.2-SDL and v0.11.1-SDL

@sezero
Copy link
Contributor Author

sezero commented Jan 6, 2025

The following commits of ours from the old v0.6.1-SDL branch might
be still needed, but they do not apply cleanly, and they obviously
need updating and / or changing for new v0.7.2-SDL and v0.11.1-SDL
branches: Leaving those to @madebr and possibly @slouken.

libsdl-org/libjxl@4addaa4
libsdl-org/libjxl@8858f35
libsdl-org/libjxl@3508c0c
libsdl-org/libjxl@d456eed
libsdl-org/libjxl@19cfa74

@sezero
Copy link
Contributor Author

sezero commented Jan 28, 2025

I upgraded both SDL2 and SDL3 (main) branches to libjxl-0.7.2, i.e.
they are now using v0.7.2-SDL branch: The CI builds seem successful.
Hopefully done everything correctly: @madebr, @slouken: Please review
and test. @1bsyl: Make sure Android.mk builds are successful.

  • Missing thing: The Xcode projects need updating: Job for @slouken
    for both SDL2 and SDL3 branches.

The SDL3 branch can still be upgraded to libjxl-0.11.1: v0.11.1-SDL
branch is there, but it is missing Android.mk support and it may have
build system / build flags changes, therefore I will not be doing that
at least for now. @madebr and/or @slouken are welcome to it.

@slouken
Copy link
Collaborator

slouken commented Jan 28, 2025

Thanks, I'll take a look at this when I'm done with SDL_ttf.

@1bsyl
Copy link
Contributor

1bsyl commented Jan 28, 2025

@sezero
doesn't seem to compile on android:
no $(LOCAL_PATH) on src files helps

Then, I got:

external/libjxl/third_party/highway/hwy/aligned_allocator.h:21:10: fatal error: 'algorithm' file not found
   21 | #include <algorithm>

but I never tried jxl before...

--- a/Android.mk
+++ b/Android.mk
@@ -10,21 +10,21 @@ LOCAL_C_INCLUDES := \
     $(LOCAL_PATH)/third_party/brotli/c/dec \
 
 BROTLI_SRC_FILES := \
-    $(LOCAL_PATH)/third_party/brotli/c/common/constants.c \
-    $(LOCAL_PATH)/third_party/brotli/c/common/context.c \
-    $(LOCAL_PATH)/third_party/brotli/c/common/dictionary.c \
-    $(LOCAL_PATH)/third_party/brotli/c/common/platform.c \
-    $(LOCAL_PATH)/third_party/brotli/c/common/shared_dictionary.c \
-    $(LOCAL_PATH)/third_party/brotli/c/common/transform.c \
-    $(LOCAL_PATH)/third_party/brotli/c/dec/bit_reader.c \
-    $(LOCAL_PATH)/third_party/brotli/c/dec/decode.c \
-    $(LOCAL_PATH)/third_party/brotli/c/dec/huffman.c \
-    $(LOCAL_PATH)/third_party/brotli/c/dec/state.c \
+    third_party/brotli/c/common/constants.c \
+    third_party/brotli/c/common/context.c \
+    third_party/brotli/c/common/dictionary.c \
+    third_party/brotli/c/common/platform.c \
+    third_party/brotli/c/common/shared_dictionary.c \
+    third_party/brotli/c/common/transform.c \
+    third_party/brotli/c/dec/bit_reader.c \
+    third_party/brotli/c/dec/decode.c \
+    third_party/brotli/c/dec/huffman.c \
+    third_party/brotli/c/dec/state.c \
 
 HIGHWAY_SRC_FILES := \
-    $(LOCAL_PATH)/third_party/highway/hwy/aligned_allocator.cc \
-    $(LOCAL_PATH)/third_party/highway/hwy/per_target.cc \
-    $(LOCAL_PATH)/third_party/highway/hwy/targets.cc \
+    third_party/highway/hwy/aligned_allocator.cc \
+    third_party/highway/hwy/per_target.cc \
+    third_party/highway/hwy/targets.cc \
 
 LOCAL_SRC_FILES :=  \
     $(BROTLI_SRC_FILES) \

@sezero
Copy link
Contributor Author

sezero commented Jan 28, 2025

@1bsyl : I quickly tried with my very old ndk. If I specified APP_STL on the ndk-build command line, then the include failure seems to go away. E.g., APP_STL=c++_static worked for me.

I'll push the Android.mk changes shortly.

@sezero
Copy link
Contributor Author

sezero commented Jan 28, 2025

I'll push the Android.mk changes shortly.

.. and done.

@1bsyl
Copy link
Contributor

1bsyl commented Jan 28, 2025

yes, that's help.
it misses also
jxl/version.h

@sezero
Copy link
Contributor Author

sezero commented Jan 28, 2025

yes, that's help. it misses also jxl/version.h

OK, updated to add jxl/version.h

@1bsyl
Copy link
Contributor

1bsyl commented Jan 28, 2025

ok, that's compile now !

@sezero
Copy link
Contributor Author

sezero commented Jan 28, 2025 via email

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

4 participants