drowe67 / codec2

Open source speech codec designed for communications quality speech between 700 and 3200 bit/s. The main application is low bandwidth HF/VHF digital radio.
GNU Lesser General Public License v2.1
201 stars 32 forks source link

lpc_to_lsp function name clashes with libspeex when building for static ffmpeg binary with musl (alpine linux) linker #59

Closed AlexanderSchuetz97 closed 3 months ago

AlexanderSchuetz97 commented 4 months ago

both libspeex and libcodec2 define a function for export called lpc_to_lsp. When statically linking ffmpeg with both libraries this blows up. Would it be possible to give this function (and the other functions in lsp.h) a more unique name that doesnt clash? or perhaps make the implementation static and be just a header so that the linker doesn't even export this function? I don't think this function gets used a lot in this codebase, so either renaming it or making it static should not be a big task?

I am aware that I could just aswell report this to libspeex but this repository is more active so I decided to ask here.

drowe67 commented 3 months ago

Actually on a historical note I think they came from the same code.

@AlexanderSchuetz97 could you pls tell me a little about your project, are you a ffmpeg maintainer? As per the codec2 README we don't usually make changes to support one off or experimental projects - they are best done outside of libcodec2 for example a patch or linker option in your project's build system.

Also I'm curious why this hasn't come up before. I have ffmpeg as part of my distro and it seems to have both libraries. I imagine this is a very common problem (functions with the same name in different libraries), and can't help thinking there must be a common solution.

AlexanderSchuetz97 commented 3 months ago

I am not a ffmpeg maintainer. I work for a small business that sells image/video/audio management software for a variety of different customer fields in europe. (medical, automotive/manufacturing, police)

I need to compile a static version of ffmpeg thats using lgpl, works on any distro (past present and future) and ideally can read as many formats as possible as I have absolutely no idea what the end user will feed into ffmpeg. (I have seen some crazy formats come by)

The reason why I want a static build is so that the binary will work on all Linux distros. You cannot do this reliably with glibc, so you have to use musl-libc for this.

The maintainers of alpine do dynamic linking for their version of ffmpeg.

Other static builds ive found do not seem to enable both libcodec2 and libspeex. Most(all?) are gpl anyways so I cannot use them anyways...

Some static builds do it wrong and use glibc. That will cause funny things to happen once you try to use ffmpeg to open a network stream.

I think I might be the first person that tries to do this with libc-musl. Because Ive spent hours debugging various crashes that occured due to subtle differences between glibc and musl libc today in various ffmpeg modules/dependencies.

If you know a magic linker option I can pass to ffmpegs ./configure script that will just "ignore" this duplication of symbols then I am all for it and would add it to my buildscript. However my knowledge of static linking (which is not on the black magic wizzard level) tells me that this will not be easy, if at all possible. Ive went for the easy quick win and just renamed it here as I had various other more difficult issues to still deal with.

If you do not want to accept my pr then that is fine by me. Ill just keep using my fork as my changes are straight forward enough to maintain with reasonable effort. If you have linker magic that works for me then that would of course be appreciated.

If you want my buildscript, then tell me and I will attach it to this issue tomorrow when I am back at work. (or rather upload it somehwere since the script together with all the random dependencies artefacts needed are about 100mb)

In terms of userbase, this will ship to a few R&D teams of BMW, Volkswagen, ZF, various industrial parts suppliers, about 30-40% of hospitals in Germany/Switzerland and the parts of the Swiss and Saxon (Germany) police in the next 1-3 years.

If you conclude that as per your readme what I am trying to do falls into the "wont be fixed here" category then close this issue and the pr please, because that way I will know that it wont be merged and need to do things to ensure that my/the fork will be maintained going forward on my end.

drowe67 commented 3 months ago

Thanks for your response @AlexanderSchuetz97 - I'll do some thinking and get back to you. IIRC we've handled similar namespace clashes in the past.

drowe67 commented 3 months ago

OK this is how we handled a similar problem. The LPCNet project uses some of the same functions as codec2, and we had a circular build problem. So this solution uses #defines and some compiler flags to rename the functions.

Not sure which solution is best.

AlexanderSchuetz97 commented 3 months ago

As long as the compiler flags are documented in the readme and easy to find then theres nothing wrong with this approch.

In my humble opinion tho, the "easy" low effort solution is to just rename those 2 functions. Adding compiler options for this will just increase the maintainability for little to no gain. I dont think those 2 functions are part of what you consider public api of this project. If someone used them then that was probably by coincidence and then said person should really just copy the .c and .h into his project.

tmiw commented 3 months ago

I'd actually lean towards just merging the open PR for this (which renames the functions), assuming it's not actually intended to be part of the public API. An extra #define seems like it'd add additional maintainability effort to me. Of course, it's ultimately up to @drowe67.

drowe67 commented 3 months ago

@AlexanderSchuetz97 - I don't have any major objection to your solution. However how about:

cd ~/codec2
mkdir build_linux
cd build_linux
cmake -DBUILD_SHARED_LIBS=OFF -DCMAKE_C_FLAGS="-Dlsp_to_lpc=codec2_lsp_to_lpc -Dlpc_to_lsp=codec2_lpc_to_lsp" ..
make
nm src/libcodec2.a | grep -e lpc_to_lsp -elsp_to_lpc
<snip>
                 U codec2_lsp_to_lpc
0000000000000130 T codec2_lpc_to_lsp
0000000000000530 T codec2_lsp_to_lpc
                 U codec2_lpc_to_lsp

I'm guessing at some point you're building codec2 from source then and building ffmpeg which links with your local libcodec2.a, so hopefully this wouldn't require any ffmpeg ./configure options.

AlexanderSchuetz97 commented 3 months ago

This would probably do it. Ill try that tomorrow. I did not consider editing the elf binary. Thats some out of the box thinking haha

AlexanderSchuetz97 commented 3 months ago

This works. Thank you very much!

There is one last thing, what do we do about the merge request? I assume you don't want to merge it? Can you tell me what you intend to do with it? If you don't want to merge it then I will remove my fork of this repository. After you have told me what you intend to do I will close this issue.

If you dont want to merge this, then I would recommend adding a hint to the projects README.

drowe67 commented 3 months ago

Good to see it works for you. Yes we can close the PR without merging.