From c0e3a62342bb718871ff5ab4efd699257e52f833 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Tue, 9 Apr 2024 15:16:35 +0200 Subject: [PATCH] Avoid abseil containers with SeqManager compare functions (#1369) --- .github/workflows/codeql.yaml | 1 + .github/workflows/mediasoup-node.yaml | 4 ++++ .github/workflows/mediasoup-rust.yaml | 6 +++++- .github/workflows/mediasoup-worker-fuzzer.yaml | 4 ++++ .github/workflows/mediasoup-worker-prebuild.yaml | 1 + .github/workflows/mediasoup-worker.yaml | 4 ++++ worker/include/RTC/NackGenerator.hpp | 8 +++----- worker/include/RTC/SenderBandwidthEstimator.hpp | 3 +-- worker/include/RTC/WebRtcServer.hpp | 2 ++ worker/src/RTC/AudioLevelObserver.cpp | 1 + worker/src/RTC/WebRtcServer.cpp | 11 ++++++++++- 11 files changed, 36 insertions(+), 9 deletions(-) diff --git a/.github/workflows/codeql.yaml b/.github/workflows/codeql.yaml index 16712fe8d3..331661b59c 100644 --- a/.github/workflows/codeql.yaml +++ b/.github/workflows/codeql.yaml @@ -42,6 +42,7 @@ jobs: env: MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true' MEDIASOUP_LOCAL_DEV: 'true' + MEDIASOUP_BUILDTYPE: 'Release' steps: - name: Checkout repository diff --git a/.github/workflows/mediasoup-node.yaml b/.github/workflows/mediasoup-node.yaml index bf643d7caf..a8436e5e0f 100644 --- a/.github/workflows/mediasoup-node.yaml +++ b/.github/workflows/mediasoup-node.yaml @@ -29,12 +29,16 @@ jobs: node: 20 - os: windows-2022 node: 20 + build-type: + - Release + - Debug runs-on: ${{ matrix.ci.os }} env: MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true' MEDIASOUP_LOCAL_DEV: 'true' + MEDIASOUP_BUILDTYPE: ${{ matrix.build-type }} steps: - name: Checkout diff --git a/.github/workflows/mediasoup-rust.yaml b/.github/workflows/mediasoup-rust.yaml index c2f449c010..a6605fa8d7 100644 --- a/.github/workflows/mediasoup-rust.yaml +++ b/.github/workflows/mediasoup-rust.yaml @@ -49,8 +49,12 @@ jobs: - name: cargo clippy run: cargo clippy --all-targets -- -D warnings + # NOTE: In Windows this will build and test libmediasoupworker in release + # mode twice since build.rs doesn't allow debug mode for Windows. - name: cargo test - run: cargo test --verbose + run: | + cargo test --verbose + cargo test --release --verbose - name: cargo doc run: cargo doc --locked --all --no-deps --lib diff --git a/.github/workflows/mediasoup-worker-fuzzer.yaml b/.github/workflows/mediasoup-worker-fuzzer.yaml index 6662e144ac..97b2b0cfb5 100644 --- a/.github/workflows/mediasoup-worker-fuzzer.yaml +++ b/.github/workflows/mediasoup-worker-fuzzer.yaml @@ -20,6 +20,9 @@ jobs: cc: clang cxx: clang++ arch: arm64 + build-type: + - Release + - Debug runs-on: ${{ matrix.build.os }} @@ -28,6 +31,7 @@ jobs: CXX: ${{ matrix.build.cxx }} MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true' MEDIASOUP_LOCAL_DEV: 'true' + MEDIASOUP_BUILDTYPE: ${{ matrix.build-type }} steps: - name: Checkout diff --git a/.github/workflows/mediasoup-worker-prebuild.yaml b/.github/workflows/mediasoup-worker-prebuild.yaml index e6616f033d..35cc8466cc 100644 --- a/.github/workflows/mediasoup-worker-prebuild.yaml +++ b/.github/workflows/mediasoup-worker-prebuild.yaml @@ -51,6 +51,7 @@ jobs: CXX: ${{ matrix.build.cxx }} MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true' MEDIASOUP_LOCAL_DEV: 'true' + MEDIASOUP_BUILDTYPE: 'Release' steps: - name: Checkout diff --git a/.github/workflows/mediasoup-worker.yaml b/.github/workflows/mediasoup-worker.yaml index 6851658a8f..0b05cd9dc1 100644 --- a/.github/workflows/mediasoup-worker.yaml +++ b/.github/workflows/mediasoup-worker.yaml @@ -53,6 +53,9 @@ jobs: # A single Node.js version should be fine for C++. node: - 20 + build-type: + - Release + - Debug runs-on: ${{ matrix.build.os }} @@ -61,6 +64,7 @@ jobs: CXX: ${{ matrix.build.cxx }} MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true' MEDIASOUP_LOCAL_DEV: 'true' + MEDIASOUP_BUILDTYPE: ${{ matrix.build-type }} steps: - name: Checkout diff --git a/worker/include/RTC/NackGenerator.hpp b/worker/include/RTC/NackGenerator.hpp index 3195267729..f46ab43fe7 100644 --- a/worker/include/RTC/NackGenerator.hpp +++ b/worker/include/RTC/NackGenerator.hpp @@ -5,8 +5,6 @@ #include "RTC/RtpPacket.hpp" #include "RTC/SeqManager.hpp" #include "handles/TimerHandle.hpp" -#include -#include #include #include #include @@ -80,9 +78,9 @@ namespace RTC // Allocated by this. TimerHandle* timer{ nullptr }; // Others. - absl::btree_map::SeqLowerThan> nackList; - absl::btree_set::SeqLowerThan> keyFrameList; - absl::btree_set::SeqLowerThan> recoveredList; + std::map::SeqLowerThan> nackList; + std::set::SeqLowerThan> keyFrameList; + std::set::SeqLowerThan> recoveredList; bool started{ false }; uint16_t lastSeq{ 0u }; // Seq number of last valid packet. uint32_t rtt{ 0u }; // Round trip time (ms). diff --git a/worker/include/RTC/SenderBandwidthEstimator.hpp b/worker/include/RTC/SenderBandwidthEstimator.hpp index 208b188f93..7b715deee9 100644 --- a/worker/include/RTC/SenderBandwidthEstimator.hpp +++ b/worker/include/RTC/SenderBandwidthEstimator.hpp @@ -6,7 +6,6 @@ #include "RTC/RateCalculator.hpp" #include "RTC/SeqManager.hpp" #include "RTC/TrendCalculator.hpp" -#include #include namespace RTC @@ -103,7 +102,7 @@ namespace RTC uint32_t initialAvailableBitrate{ 0u }; uint32_t availableBitrate{ 0u }; uint64_t lastAvailableBitrateEventAtMs{ 0u }; - absl::btree_map::SeqLowerThan> sentInfos; + std::map::SeqLowerThan> sentInfos; float rtt{ 0 }; // Round trip time in ms. CummulativeResult cummulativeResult; CummulativeResult probationCummulativeResult; diff --git a/worker/include/RTC/WebRtcServer.hpp b/worker/include/RTC/WebRtcServer.hpp index 96bde6e277..11f3901dfb 100644 --- a/worker/include/RTC/WebRtcServer.hpp +++ b/worker/include/RTC/WebRtcServer.hpp @@ -106,6 +106,8 @@ namespace RTC absl::flat_hash_map mapLocalIceUsernameFragmentWebRtcTransport; // Map of WebRtcTransports indexed by TransportTuple.hash. absl::flat_hash_map mapTupleWebRtcTransport; + // Whether the destructor has been called. + bool closing{ false }; }; } // namespace RTC diff --git a/worker/src/RTC/AudioLevelObserver.cpp b/worker/src/RTC/AudioLevelObserver.cpp index 0b880d88af..ef40fa26e1 100644 --- a/worker/src/RTC/AudioLevelObserver.cpp +++ b/worker/src/RTC/AudioLevelObserver.cpp @@ -5,6 +5,7 @@ #include "Logger.hpp" #include "MediaSoupErrors.hpp" #include "RTC/RtpDictionaries.hpp" +#include #include // std::lround() namespace RTC diff --git a/worker/src/RTC/WebRtcServer.cpp b/worker/src/RTC/WebRtcServer.cpp index 1cd27188b5..274391d922 100644 --- a/worker/src/RTC/WebRtcServer.cpp +++ b/worker/src/RTC/WebRtcServer.cpp @@ -232,6 +232,8 @@ namespace RTC { MS_TRACE(); + this->closing = true; + this->shared->channelMessageRegistrator->UnregisterHandler(this->id); for (auto& item : this->udpSocketOrTcpServers) @@ -497,7 +499,14 @@ namespace RTC this->webRtcTransports.find(webRtcTransport) != this->webRtcTransports.end(), "WebRtcTransport not handled"); - this->webRtcTransports.erase(webRtcTransport); + // NOTE: If WebRtcServer is closing then do not remove the transport from + // the set since it would modify the set while the WebRtcServer destructor + // is iterating it. + // See: https://github.com/versatica/mediasoup/pull/1369#issuecomment-2044672247 + if (!this->closing) + { + this->webRtcTransports.erase(webRtcTransport); + } } inline void WebRtcServer::OnWebRtcTransportLocalIceUsernameFragmentAdded(