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 to m112 #154

Closed
wants to merge 6 commits into from
Closed

Upgrade to m112 #154

wants to merge 6 commits into from

Conversation

jmillan
Copy link
Member

@jmillan jmillan commented Mar 28, 2023

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
Copy link
Contributor

copiltembel commented Mar 28, 2023

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
Copy link
Contributor

copiltembel commented Mar 28, 2023

Sorry, unable to test on a Mac m1 atm.

@jmillan
Copy link
Member Author

jmillan commented Mar 28, 2023

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
Copy link
Member Author

jmillan commented Mar 28, 2023

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
Copy link
Member Author

jmillan commented Mar 28, 2023

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

@ibc
Copy link
Member

ibc commented Mar 28, 2023

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

@copiltembel
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

jmillan commented Mar 29, 2023

I'll try with an Intel Mac when possible.

The linking issues are related to:

  • RTC_DCHECK_RUN_ON calls on fake_audio_capture_module.cpp
  • And to the audio and video tracks generated in MediaStreamTrackFactory.cpp

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
Copy link

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<webrtc::VideoFrameType, std::__1::allocator<webrtc::VideoFrameType> > 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:

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
Copy link

Poldraunic commented Apr 2, 2023

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
Copy link
Contributor

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
Copy link
Member Author

jmillan commented Apr 5, 2023

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

@jmillan
Copy link
Member Author

jmillan commented Jun 20, 2023

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
Copy link

mikepulaski commented Jul 20, 2023

@jmillan I've been maintaining a fork1 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 :)

Footnotes

  1. https://github.com/tupleapp/libmediasoupclient/tree/3.4.0/m107

@jmillan
Copy link
Member Author

jmillan commented Jul 20, 2023

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

@jmillan
Copy link
Member Author

jmillan commented Jul 20, 2023

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

Thanks @mikepulaski :-)

Screenshot 2023-07-20 at 12 07 24

.., due to a limitation in libwebrtc

#154 (comment)
@jmillan
Copy link
Member Author

jmillan commented Jul 20, 2023

@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 #154 (comment), don't hesitate to comment or provide a fix.

@Poldraunic
Copy link

Do you set GN argument treat_warnings_as_errors=false?

@jmillan
Copy link
Member Author

jmillan commented Jul 20, 2023

Do you set GN argument treat_warnings_as_errors=false?

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

@jmillan
Copy link
Member Author

jmillan commented Jul 21, 2023

@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
Copy link

@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 #154 (comment), 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
Copy link
Member Author

jmillan commented Aug 1, 2023

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
Copy link
Member Author

jmillan commented Nov 3, 2023

@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
Copy link

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
Copy link
Member Author

jmillan commented Nov 6, 2023

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
Copy link

@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
Copy link
Member Author

jmillan commented Nov 7, 2023

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

@ibc
Copy link
Member

ibc commented Feb 16, 2024

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

@jmillan
Copy link
Member Author

jmillan commented Feb 20, 2024

Closed in favour of #173

@jmillan jmillan closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants