versatica / libmediasoupclient

mediasoup client side C++ library
https://mediasoup.org
ISC License
286 stars 177 forks source link

Upgrade to m112 #154

Closed jmillan closed 7 months ago

jmillan commented 1 year ago

While upgrading to m112 I'm facing the following link issue with the tests.

Screenshot 2023-03-28 at 16 39 19

Can anyone try it and provide some help?

copiltembel commented 1 year ago

Not necessarily related - as the error seems to be linked to the arm64 arch - but just to make sure I've tried it on a Linux box, and it DID build OK. Didn't try to run it with the mediasoup-broadcaster-demo yet though.

Just to make sure, I checked out refs/remotes/branch-heads/5615 for m112. I'll try on a Mac m1 hopefully later today and let you know.

copiltembel commented 1 year ago

Sorry, unable to test on a Mac m1 atm.

jmillan commented 1 year ago

Nice @copiltembel,

Just to make sure, I checked out refs/remotes/branch-heads/5615 for m112.

That's correct. Here the mapping https://chromiumdash.appspot.com/branches

I'll try on a Mac m1 hopefully later today and let you know.

Thanks, this are the commands I used to build it:

gn gen out/m112 --args='is_debug=false is_component_build=false is_clang=true rtc_include_tests=false rtc_use_h264=true use_rtti=true use_custom_libcxx=false'

ninja -C out/m112/

jmillan commented 1 year ago

I've tried it on a Linux box, and it DID build OK

If you can run the tests we can push it as the linking issue much probably a local error.

Test compilation is enabled with the flag: -DMEDIASOUPCLIENT_BUILD_TESTS:BOOLEAN=ON

jmillan commented 1 year ago

@copiltembel, did you use the same gn gen args as I did?

ibc commented 1 year ago

We must update website Installation section with those new values and args.

copiltembel commented 1 year ago

Right, I should have enabled the -DMEDIASOUPCLIENT_BUILD_TESTS:BOOLEAN=ON.

I've used these parameters when building libwebrtc m112:

`$ gn gen out/m112_debug --args='is_debug=true is_component_build=false is_clang=false rtc_include_tests=false rtc_use_h264=true use_rtti=true use_custom_libcxx=false treat_warnings_as_errors=false rtc_use_x11=false use_glib=false rtc_enable_protobuf=false rtc_exclude_audio_processing_module=true'

But I get linking errors related to libdl when building test_mediasoupclient:

[...]
[100%] Linking CXX executable test_mediasoupclient
/usr/bin/ld: /home/tdr/src/google/m112_webrtc/src/out/m112_debug/obj/libwebrtc.a(lib.o): in function `get_stack_size_internal':
/home/tdr/src/google/m112_webrtc/src/out/m112_debug/../../third_party/dav1d/libdav1d/src/lib.c:94: undefined reference to `dlsym'
/usr/bin/ld: /home/tdr/src/google/m112_webrtc/src/out/m112_debug/obj/libwebrtc.a(latebindingsymboltable_linux.o): in function `webrtc::adm_linux::GetDllError()':
/home/tdr/src/google/m112_webrtc/src/out/m112_debug/../../modules/audio_device/linux/latebindingsymboltable_linux.cc:25: undefined reference to `dlerror'
/usr/bin/ld: /home/tdr/src/google/m112_webrtc/src/out/m112_debug/obj/libwebrtc.a(latebindingsymboltable_linux.o): in function `webrtc::adm_linux::InternalLoadDll(absl::string_view)':
/home/tdr/src/google/m112_webrtc/src/out/m112_debug/../../modules/audio_device/linux/latebindingsymboltable_linux.cc:38: undefined reference to `dlopen'
/usr/bin/ld: /home/tdr/src/google/m112_webrtc/src/out/m112_debug/obj/libwebrtc.a(latebindingsymboltable_linux.o): in function `webrtc::adm_linux::InternalUnloadDll(void*)':
/home/tdr/src/google/m112_webrtc/src/out/m112_debug/../../modules/audio_device/linux/latebindingsymboltable_linux.cc:58: undefined reference to `dlclose'
/usr/bin/ld: /home/tdr/src/google/m112_webrtc/src/out/m112_debug/obj/libwebrtc.a(latebindingsymboltable_linux.o): in function `webrtc::adm_linux::LoadSymbol(void*, absl::string_view, void**)':
/home/tdr/src/google/m112_webrtc/src/out/m112_debug/../../modules/audio_device/linux/latebindingsymboltable_linux.cc:71: undefined reference to `dlsym'
/usr/bin/ld: /home/tdr/src/google/m112_webrtc/src/out/m112_debug/../../modules/audio_device/linux/latebindingsymboltable_linux.cc:72: undefined reference to `dlerror'
/usr/bin/ld: /home/tdr/src/google/m112_webrtc/src/out/m112_debug/obj/libwebrtc.a(latebindingsymboltable_linux.o): in function `webrtc::adm_linux::InternalLoadSymbols(void*, int, char const* const*, void**)':
/home/tdr/src/google/m112_webrtc/src/out/m112_debug/../../modules/audio_device/linux/latebindingsymboltable_linux.cc:95: undefined reference to `dlerror'
collect2: error: ld returned 1 exit status
make[2]: *** [test/CMakeFiles/test_mediasoupclient.dir/build.make:238: test/test_mediasoupclient] Error 1
make[2]: Leaving directory '/home/tdr/src/github/versatica/libmediasoupclient/build/debug'
make[1]: *** [CMakeFiles/Makefile2:207: test/CMakeFiles/test_mediasoupclient.dir/all] Error 2
make[1]: Leaving directory '/home/tdr/src/github/versatica/libmediasoupclient/build/debug'
make: *** [Makefile:130: all] Error 2
make: Leaving directory '/home/tdr/src/github/versatica/libmediasoupclient/build/debug'

Just for the sake of it, I disabled libwebrtc's module which were complaining about libdl, like this:

$ gn gen out/m112_debug --args='is_debug=true is_component_build=false is_clang=false rtc_include_tests=false rtc_use_h264=true use_rtti=true use_custom_libcxx=false treat_warnings_as_errors=false rtc_use_x11=false use_glib=false rtc_enable_protobuf=false rtc_exclude_audio_processing_module=true rtc_include_internal_audio_device=false rtc_include_dav1d_in_internal_decoder_factory=false'

Then I've built libmediasoupclient:

$ cmake -Bbuild/debug -DCMAKE_BUILD_TYPE=Debug -DLIBWEBRTC_INCLUDE_PATH:PATH=/home/tdr/src/google/m112_webrtc/src -DLIBWEBRTC_BINARY_PATH:PATH=/home/tdr/src/google/m112_webrtc/src/out/m112_debug/obj -DMEDIASOUPCLIENT_BUILD_TESTS:BOOLEAN=ON $ make -j -C build/debug/

which succeeded, but obviously test_mediasoupclient crashed when running.

I'll try to have a look tomorrow again at the libdl problem. I had it some time ago with a different application, but don't remember right now how I fixed it.

copiltembel commented 1 year ago

I've managed to build and run test_mediasoupclient by adding

set(CMAKE_EXE_LINKER_FLAGS -Wl,--no-as-needed) to test/CMakeLists.txt

But running it fails with this error:

$ ./build/debug/test/test_mediasoupclient 

#
# Fatal error in: ../../call/call.cc, line 722
# last system error: 0
# Check failed: (worker_thread_)->IsCurrent()
# 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_mediasoupclient is a Catch v2.13.7 host application.
Run with -? for options

-------------------------------------------------------------------------------
SendHandler
-------------------------------------------------------------------------------
/home/tdr/src/github/versatica/libmediasoupclient/test/src/Handler.test.cpp:37
...............................................................................

/home/tdr/src/github/versatica/libmediasoupclient/test/src/Handler.test.cpp:37: FAILED:
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases:  3 |  2 passed | 1 failed
assertions: 20 | 19 passed | 1 failed

Aborted (core dumped)

For reference, here's how I've built libwebrtc:

$ gn gen out/m112_debug --args='is_debug=true is_component_build=false is_clang=false rtc_include_tests=false rtc_use_h264=true use_rtti=true use_custom_libcxx=false treat_warnings_as_errors=false rtc_use_x11=false use_glib=false rtc_enable_protobuf=false'
jmillan commented 1 year ago

I'll try with an Intel Mac when possible.

The linking issues are related to:

After commenting calls to RTC_DCHECK_RUN_ON and making create[Audio|Video]Track() return just an null track there is no link error.

When running then the test binary I see this one:

Process 76390 launched: '/Users/jmillan/src/libmediasoupclient/build/test/test_mediasoupclient.app/Contents/MacOS/test_mediasoupclient' (arm64)
2023-03-29 11:26:22.004680+0200 test_mediasoupclient[76390:937365] [plugin] AddInstanceForFactory: No factory registered for id <CFUUID 0x60000021c260> F8BB1C28-BAE8-11D6-9C31-00039315CD46
2023-03-29 11:26:22.016236+0200 test_mediasoupclient[76390:937365]   HALC_ProxyObjectMap.cpp:153    HALC_ProxyObjectMap::_CopyObjectByObjectID: failed to create the local object
2023-03-29 11:26:22.016250+0200 test_mediasoupclient[76390:937365]      HALC_ShellDevice.cpp:2606   HALC_ShellDevice::RebuildControlList: couldn't find the control object
Poldraunic commented 1 year ago

I did build M112 and successfully linked it to lib/tests in both debug/release builds on M1 Mac. Tests do fail for me the same way in both builds:

./test/test_mediasoupclient.app/Contents/MacOS/test_mediasoupclient

#
# Fatal error in: ../../call/call.cc, line 722
# last system error: 0
# Check failed: (worker_thread_)->IsCurrent()
#
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_mediasoupclient is a Catch v2.13.7 host application.
Run with -? for options

-------------------------------------------------------------------------------
SendHandler
-------------------------------------------------------------------------------
/Users/egor/programming/libmediasoupclient_m112/test/src/Handler.test.cpp:37
...............................................................................

/Users/egor/programming/libmediasoupclient_m112/test/src/Handler.test.cpp:37: FAILED:
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases:  3 |  2 passed | 1 failed
assertions: 20 | 19 passed | 1 failed

zsh: abort      ./test/test_mediasoupclient.app/Contents/MacOS/test_mediasoupclient

Build commands:

gn gen out/m112_release_arm --args='is_debug=false is_clang=true rtc_include_tests=false rtc_use_h264=true use_rtti=true use_custom_libcxx=false treat_warnings_as_errors=false' --export-compile-commands
ninja -C out/m112_release_arm
cmake .. -DMEDIASOUPCLIENT_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Release -DLIBWEBRTC_INCLUDE_PATH=/Users/egor/programming/mediasoup/webrtc-checkout/src -DLIBWEBRTC_BINARY_PATH=/Users/egor/programming/mediasoup/webrtc-checkout/src/out/m112_release_arm/obj

Hope that helps! Looking forward to upgrading!


I did however got an excessive amount of warnings regarding symbol visibility. I am not knowledgable enough to know that exactly what it means, but in my app I solved it with patch to CMakeLists.txt

``` ld: warning: direct access in function 'webrtc::(anonymous namespace)::LibaomAv1Encoder::Encode(webrtc::VideoFrame const&, std::__1::vector > const*)' from file '/Users/egor/programming/mediasoup/webrtc-checkout/src/out/m112_release_arm/obj/libwebrtc.a(libaom_av1_encoder.o)' to global weak symbol 'void rtc::webrtc_checks_impl::LogStreamer<>::Call<>(char const*, int, char const*)::t' from file '../libmediasoupclient.a(PeerConnection.cpp.o)' means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings. ```

CMakeLists.txt patch, taken from previous libmediasoupclient version:

```patch diff --git a/CMakeLists.txt b/CMakeLists.txt index 8ce1b7d..99f9ef2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,6 +1,7 @@ cmake_minimum_required(VERSION 3.5) project(mediasoupclient LANGUAGES CXX) +cmake_policy(SET CMP0063 NEW) # Set version number. set(mediasoupclient_VERSION_MAJOR 3) @@ -16,6 +17,8 @@ configure_file ( # C++ standard requirements. set(CMAKE_CXX_STANDARD 14) set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CMAKE_CXX_VISIBILITY_PRESET hidden) +set(CMAKE_VISIBILITY_INLINES_HIDDEN ON) # Project options. option(MEDIASOUPCLIENT_BUILD_TESTS "Build unit tests" OFF) diff --git a/deps/libsdptransform/CMakeLists.txt b/deps/libsdptransform/CMakeLists.txt index b4f4648..e601ef7 100644 --- a/deps/libsdptransform/CMakeLists.txt +++ b/deps/libsdptransform/CMakeLists.txt @@ -1,4 +1,5 @@ -cmake_minimum_required(VERSION 3.0) +cmake_minimum_required(VERSION 3.3) +cmake_policy(SET CMP0063 NEW) project(sdptransform VERSION 1.2.9) ```
Poldraunic commented 1 year ago

I now think it might be related to xcode version. What version are you running? I am on 13.2.1 which I pulled from some libWebRTC config file where they mentioned you have to use this exact version. But that was on M94, not sure if it changed for M112. Nevermind striked text, I think my assumption is wrong.

copiltembel commented 1 year ago

I managed to get passed that test by commenting out some code that I believe is not needed in that failing test:

diff --git a/test/src/Handler.test.cpp b/test/src/Handler.test.cpp
index ccfbd61..4d71fab 100644
--- a/test/src/Handler.test.cpp
+++ b/test/src/Handler.test.cpp
@@ -48,8 +48,8 @@ TEST_CASE("SendHandler", "[Handler][SendHandler]")
          RtpParametersByKind,
          RtpParametersByKind);

-       static std::unique_ptr<mediasoupclient::PeerConnection> pc(
-         new mediasoupclient::PeerConnection(nullptr, &PeerConnectionOptions));
+       // static std::unique_ptr<mediasoupclient::PeerConnection> pc(
+       //   new mediasoupclient::PeerConnection(nullptr, &PeerConnectionOptions));

But I'm stuck at another test:

#
# Fatal error in: ../../pc/rtp_sender.cc, line 665
# last system error: 0
# Check failed: (signaling_thread_)->IsCurrent()
# 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_mediasoupclient is a Catch v2.13.7 host application.
Run with -? for options

-------------------------------------------------------------------------------
mediasoupclient
  transport.produce() succeeds
-------------------------------------------------------------------------------
/home/tdr/src/github/versatica/libmediasoupclient/test/src/mediasoupclient.test.cpp:160
...............................................................................

/home/tdr/src/github/versatica/libmediasoupclient/test/src/mediasoupclient.test.cpp:193: FAILED:
  {Unknown expression after the reported line}
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases:   8 |   7 passed | 1 failed
assertions: 146 | 145 passed | 1 failed

Aborted (core dumped)
jmillan commented 1 year ago

Thanks for the input, don't hesitate to keep progressing. We'll come back to this as soon as we can.

jmillan commented 1 year ago

Still stuck here. The tests totally depend on the webrtc internal test code which is something we may need to get rid of.

The same happens with mediasoup-broadcaster-demo..

mikepulaski commented 1 year ago

@jmillan I've been maintaining a fork^1 with some changes which helped us with our threading model, and we've done a few WebRTC upgrades (we're on M111 now, which is compatible with our M107 branch).

Long story short, you'll need to link against a debug version of WebRTC to run tests, or run tests in release mode.

IIRC, starting with M101, WebRTC's sequence checker (which checks for thread safety) has different implementations when running debug/release builds. The headers for the implementation use macros which expose different methods/member variables. When you run libmediasoupclient tests, there is an inconsistency between which methods the compiler thinks are available, and the symbols which actually are.

We use a custom build of WebRTC already, so we fixed this by patching the sequence checker to be disabled in our debug builds, which allows us to use release builds of WebRTC from our tests (and app during development).

Hope this helps! I'm happy to answer any other questions or help troubleshoot. I'm pretty familiar with both codebases :)

jmillan commented 1 year ago

Very much appreciated @mikepulaski! I'll give it a try and let you know!

jmillan commented 1 year ago

Linker issue is gone :-). Now I need to debug a RefCount crash.

Thanks @mikepulaski :-)

Screenshot 2023-07-20 at 12 07 24
jmillan commented 1 year ago

@mikepulaski,

This branch, as it is, does compile and link properly now for m112. Thanks for your suggestion!

The tests crashes as shown here. In order to debug such problem I'm compiling libwebrtc in debug mode ('is_debug=true'), and I find it failins with the following error:

error: 'CGDisplayStreamStop' is only available on macOS 13.0 or newer [-Werror,-Wunguarded-availability-new]

When compiling in non debug mode, the error is indeed a warning:

warning: 'CGDisplayStreamStop' is only available on macOS 13.0 or newer [-Wunguarded-availability-new]

As you see, compiling in debug mode adds the -Werror compilation flag.

Do you know how can I remove such flag?

Also, if you can spot the problem here https://github.com/versatica/libmediasoupclient/pull/154#issuecomment-1643644216, don't hesitate to comment or provide a fix.

Poldraunic commented 1 year ago

Do you set GN argument treat_warnings_as_errors=false?

jmillan commented 1 year ago

Do you set GN argument treat_warnings_as_errors=false?

Nop, I couldn't find it. I'll try it. Thanks!

jmillan commented 1 year ago

@mikepulaski, you have done in your branch many changes. I'll check them in case we would like to introduce any in mainstream. Is that OK?

We can comment further once I take a look at the changes. Would you please share the overall motivations for your changes?

EDIT: Does the public API change in any means? If so, could you share why and how?

mikepulaski commented 1 year ago

@jmillan Glad I can help!

As you see, compiling in debug mode adds the -Werror compilation flag. Do you know how can I remove such flag?

@Poldraunic's suggestion should do the trick!

FWIW, this API was incorrectly marked as only available for macOS13+ in the macOS SDK shipping with Xcode 14.3+, but has been fixed in Xcode 15. It's harmless :)

Also, if you can spot the problem here https://github.com/versatica/libmediasoupclient/pull/154#issuecomment-1643644216, don't hesitate to comment or provide a fix.

I'm not certain just by looking at it. Do you know which test is failing? It might help to have a bit more context.

From the looks of things, a ref counted object is freeing memory which has already been freed. The main difference I can see between this branch and the one that I forked is that I've replaced all instances of webrtc::RtpSenderInterface* and webrtc::MediaStreamTrackInterface* with rtc::scoped_refptr<> versions. In theory, it should be the same thing, but there are places where the reference counts are not being incremented/decremented (e.g., in Producer/Consumer).

I understand that it might be difficult to update public API signatures, but it might be worth doing it in your local copy to see if it fixes the problem. If it does, and you want to maintain API compatibility, you could manually call AddRef() and Release() where appropriate.

jmillan commented 1 year ago

Hi @mikepulaski,

I'm not certain just by looking at it. Do you know which test is failing? It might help to have a bit more context.

Yes, so having built this branch the following test fails when running the test binary:

Screenshot 2023-08-01 at 10 45 44

Running it under LLDB gives the following trace:

Screenshot 2023-08-01 at 10 46 55

libwebrtc is compiled in debug mode as suggested.

jmillan commented 10 months ago

@mikepulaski, do you think you can provide some help here? The problem is, as described, with the test aborting. As per the trace in the previous comment the medias source interface state is not kInitizalizing as it should, hence the abort.

Seems that the changes in the code are correct, but if we cannot test it then we are blocked to release it.

We very much appreciate if you or anyone finds the time to check this.

NOTE: We are talking about this exact branch, which can be cloned and compiled.

Poldraunic commented 10 months ago

I'd like to help, but I am unable to reproduce fail for this test. For me, everything is still as before:

@jmillan, are you sure you're on the correct WebRTC branch and it is compiled correctly?

jmillan commented 10 months ago

I'd like to help, but I am unable to reproduce fail for this test. For me, everything is still as before:

Do you mean this branch m112 is working for you without problems and the tests does not show that abort?

Poldraunic commented 10 months ago

@jmillan I haven't tried libmediasoupclient itself, but Device.test doesn't fail for me.

libmediasoupclient:

git log -1
commit 7a6dc1bd0295c0114f22b4b913ffde7e37cd74c4 (HEAD -> m112, origin/m112)
Author: José Luis Millán <jmillan@aliax.net>
Date:   Tue Aug 1 10:55:43 2023 +0200

    PeerConnection: modernize deprecated 'CreatePeerConnection' method

WebRTC:

git log -1
commit d75b9e9ff07ee42841b4e416629c9fbd4b058905 (HEAD -> m112, branch-heads/5615)
Author: Philipp Hancke <phancke@microsoft.com>
Date:   Mon Mar 27 10:19:52 2023 +0200

    [M112] Revert "Only serialize non-stopped RTP header extensions"

    This reverts commit be03c0971863c9e6807fcbdb4175754e8242a652.
    Causes regression in web projects that
    1/ add a stopped-by-default extension in SRD
    2/ call createAnswer
    3/ munge the stopped-by-default extension back in SLD
    4/ create a subsequent offer and expect the extension to be present

    BUG=chromium:1427442,chromium:1051821

    Change-Id: I2e48831e92384963a254d873398f54eaee96739a
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299143
    Reviewed-by: Harald Alvestrand <hta@webrtc.org>
    Commit-Queue: Philipp Hancke <phancke@microsoft.com>
    Reviewed-by: Henrik Boström <hbos@webrtc.org>
    Cr-Commit-Position: refs/branch-heads/5615@{#2}
    Cr-Branched-From: cdfeb4f7922f05007d88c8263842998ec79b6dd6-refs/heads/main@{#39376}
jmillan commented 10 months ago

Thanks, I'll try when I get some time 👍

ibc commented 7 months ago

Can we close this PR now that there is another one upgrading to M120?

jmillan commented 7 months ago

Closed in favour of https://github.com/versatica/libmediasoupclient/pull/173