search

ralfbiedert / openh264-rs

Idiomatic Rust wrappers around OpenH264.
69 stars 35 forks source link

Linking stage fails on linux x86_64 with nasm #26

Closed Mivik closed 1 year ago

Mivik commented 1 year ago

The library itself builds well on linux x86_64 with nasm installed. However, when I was trying to build a binary package, the linking stage failed.

..../libopenh264_sys2-455dbbb0b2ece582.rlib(error_concealment.o): in function `WelsDec::InitErrorCon(WelsDec::TagWelsDecoderContext*)':
          error_concealment.cpp:(.text._ZN7WelsDec12InitErrorConEPNS_21TagWelsDecoderContextE+0x63): undefined reference to `WelsCopy16x16_sse2'
          /usr/bin/ld: error_concealment.cpp:(.text._ZN7WelsDec12InitErrorConEPNS_21TagWelsDecoderContextE+0x8b): undefined reference to `WelsCopy8x8_mmx'

I tried to inspect these symbols, and found that in the object file (specifically libopenh264_common.a), these symbols are all prefixed with underscore and thus the linker failed to find them.

Through experiments I found that removing this line solves the problem:

https://github.com/ralfbiedert/openh264-rust/blob/0dd2282a690ba5b2e92a7a8013e275eae69614c9/openh264-sys2/build.rs#L60

, which indeed disables name prefixing.

Actually I'm not familiar with these linking stuff, so some of my understandings can be wrong. Given that, this fix can be dirty and not universally appliable so I'm asking here instead of creating a pull request.

ralfbiedert commented 1 year ago

Thanks for the report! Your guess is as good as mine w.r.t. linker flags. I happily accept a PR that removes the flag for the given config as long as the current CI tests keep passing.

DCNick3 commented 1 year ago

@ralfbiedert Are the nasm builds actually tested in CI though? As far as I can tell, the runners do not have nasm pre-installed...

With regards to prefixing: AFAIK, it's an MSVC toolchain thing, it wants to prefix user function names with and underscore, linux toolchains do not do this. Not sure about macOS, but it seems that the underscore prefixing was added in a commit fixing macOS build, so... Maybe it also does it??

ralfbiedert commented 1 year ago

Are the nasm builds actually tested in CI though? As far as I can tell, the runners do not have nasm pre-installed...

Agreed, nasm probably isn't installed (PR welcome).

DCNick3 commented 1 year ago

I am working on it

ralfbiedert commented 1 year ago

This got fixed by #28, right?

DCNick3 commented 1 year ago

Yes, it works for me now