Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(main/openal-soft): disable dlopen() of opensles for luanti #23149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertkirkman
Copy link
Contributor

@robertkirkman robertkirkman commented Feb 10, 2025

  • One of two potential solutions to [Bug]: after openal-soft bump to 1.24.2, sound stopped working in x11/luanti #23148

  • compared to fix(x11/luanti): force-link to libOpenSLES.so #23185

    • Potential advantages: would not require editing any other reverse dependency of openal-soft that could in the future be found to be affected by the same issue
    • Potential disadvantages: requires editing the openal-soft package
  • mpv --ao=openal and mplayer -ao openal continue to work with or without this reversion, so it seems ok to me. however I do not understand why this is only required for luanti, and I have not found any other reverse dependency of openal-soft that was negatively affected by the bump to openal-soft 1.24.2. MRE created and submitted upstream

@Biswa96
Copy link
Member

Biswa96 commented Feb 10, 2025

Instead of reverting the patches, would it be possible to disable the HAVE_DYNLOAD constant in alc/backends/opensl.cpp file?

@robertkirkman robertkirkman changed the title fix(main/openal-soft): revert opensl loading change for luanti fix(main/openal-soft): disable dlopen() of opensles for luanti Feb 11, 2025
@robertkirkman
Copy link
Contributor Author

robertkirkman commented Feb 11, 2025

Thank you, I did not really notice that earlier, but yes, adding -DHAVE_DLFCN_H=OFF to TERMUX_PKG_EXTRA_CONFIGURE_ARGS appears to have an identical effect, and it also solves the problem, while being much fewer lines, so I have changed it to that.

@Biswa96
Copy link
Member

Biswa96 commented Feb 11, 2025

I did not mean to add that option globally. There are many source files which depend on that option. So, I am not sure if -DHAVE_DLFCN_H=OFF breaks any existing stuff.

@robertkirkman
Copy link
Contributor Author

robertkirkman commented Feb 11, 2025

Well, I did not think that other stuff will be affected because it doesn't seem used in the Termux build of this package,

but I do not fully understand the problem, since I have no idea why only one app is affected by the problem and not other apps,

and yes you are correct, so I will write this exactly how you suggested. Also, the exact way you suggested does also work to fix the problem.

@robertkirkman robertkirkman marked this pull request as draft February 11, 2025 10:11
@robertkirkman
Copy link
Contributor Author

robertkirkman commented Feb 11, 2025

@Biswa96 something still seemed confusing to me, even more so than this problem already is, so I carefully double checked everything, and unfortunately, I have to report the following:

  • my original bisection and whole commit reversion was correct, and -DHAVE_DLFCN_H=OFF or -DALSOFT_DLOPEN=OFF by themselves were both also able to work
  • but, very unfortunately, I am regretfully saddened to have to report that when you sent me your suggestion about HAVE_DYNLOAD, I made a mistake while testing it and I did not test it correctly, so I did not realize that it didn't work as written. When I double check the exact method you suggested, unfortunately it results in this error:
CANNOT LINK EXECUTABLE "mplayer": cannot locate symbol "SL_IID_ENGINE" referenced by "/data/data/com.termux/files/usr/lib/libopenal.so"...

I have now thoroughly tested this, and it appears that at the very least, in order for the HAVE_DYNLOAD reversion to work, this single line in the CMakeLists.txt must also be reverted. I have just pushed a version that is currently able to fix the problem for me.

@robertkirkman robertkirkman marked this pull request as ready for review February 11, 2025 10:44
@robertkirkman
Copy link
Contributor Author

After continuing to debug this problem for a while, I have managed to create:

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants