Skip to content

Commit

Permalink
Avoid abseil containers with SeqManager compared functions
Browse files Browse the repository at this point in the history
Fixes #1366

### Details

- As demonstrated in temporal PR #1368, standard C++ STD containers do not throw (even in debug mode) when using `Compare` functions in maps/sets that do not honor transitivity, i.e. `comp(a,b) && comp(b,c) -> comp(a,c)`.
- So let's not use abseil containers in those cases.

### Bonus tracks

- Dupicate CI actions in debug mode.
- Make mediasoup Rust building honor `MEDIASOUP_BUILDTYPE` env variable if given.
- Fix an amazing bug in `AudioLevelObserver.cpp` which failed to compile because it uses a `absl::btree_multimap` without including the `absl/container/btree_map.h` header (it didn't fail before due to some absl header included by yet anothe included file, etc).
  • Loading branch information
ibc committed Apr 9, 2024
1 parent 1a507ec commit 479e7b1
Show file tree
Hide file tree
Showing 14 changed files with 310 additions and 14 deletions.
1 change: 1 addition & 0 deletions .github/workflows/codeql.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ jobs:
env:
MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true'
MEDIASOUP_LOCAL_DEV: 'true'
MEDIASOUP_BUILDTYPE: 'Release'

steps:
- name: Checkout repository
Expand Down
65 changes: 65 additions & 0 deletions .github/workflows/mediasoup-node-debug.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
name: mediasoup-node

on: [pull_request, workflow_dispatch]

concurrency:
# Cancel a currently running workflow from the same PR, branch or tag when a
# new workflow is triggered.
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

jobs:
ci:
strategy:
matrix:
ci:
- os: ubuntu-20.04
node: 18
- os: ubuntu-22.04
node: 20
- os: ubuntu-20.04
node: 18
arch: arm64
- os: ubuntu-22.04
node: 20
arch: arm64
- os: macos-12
node: 18
- os: macos-14
node: 20
- os: windows-2022
node: 20

runs-on: ${{ matrix.ci.os }}

env:
MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true'
MEDIASOUP_LOCAL_DEV: 'true'
MEDIASOUP_BUILDTYPE: 'Debug'

steps:
- name: Checkout
uses: actions/checkout@v4

- name: Node.js
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.ci.node }}

- name: Configure cache
uses: actions/cache@v3
with:
path: |
~/.npm
key: ${{ matrix.ci.os }}-node-${{ hashFiles('**/package.json') }}
restore-keys: |
${{ matrix.ci.os }}-node-
- name: npm ci
run: npm ci --foreground-scripts

- name: npm run lint:node
run: npm run lint:node

- name: npm run test:node
run: npm run test:node
1 change: 1 addition & 0 deletions .github/workflows/mediasoup-node.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ jobs:
env:
MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true'
MEDIASOUP_LOCAL_DEV: 'true'
MEDIASOUP_BUILDTYPE: 'Release'

steps:
- name: Checkout
Expand Down
60 changes: 60 additions & 0 deletions .github/workflows/mediasoup-rust-debug.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
name: mediasoup-rust

on: [pull_request, workflow_dispatch]

concurrency:
# Cancel a currently running workflow from the same PR, branch or tag when a
# new workflow is triggered.
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

env:
CARGO_TERM_COLOR: always

jobs:
ci:
strategy:
matrix:
ci:
- os: ubuntu-20.04
- os: ubuntu-22.04
- os: ubuntu-20.04
arch: arm64
- os: ubuntu-22.04
arch: arm64
- os: macos-12
- os: macos-14
- os: windows-2022

runs-on: ${{ matrix.ci.os }}

env:
KEEP_BUILD_ARTIFACTS: '1'
MEDIASOUP_BUILDTYPE: 'Debug'

steps:
- name: Checkout
uses: actions/checkout@v4

- name: Configure cache
uses: actions/cache@v3
with:
path: |
~/.cargo/registry
~/.cargo/git
key: ${{ matrix.ci.os }}-cargo-${{ hashFiles('**/Cargo.toml') }}

- name: cargo fmt
run: cargo fmt --all -- --check

- name: cargo clippy
run: cargo clippy --all-targets -- -D warnings

- name: cargo test
run: cargo test --verbose

- name: cargo doc
run: cargo doc --locked --all --no-deps --lib
env:
DOCS_RS: '1'
RUSTDOCFLAGS: '-D rustdoc::broken-intra-doc-links -D rustdoc::private_intra_doc_links'
1 change: 1 addition & 0 deletions .github/workflows/mediasoup-rust.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ jobs:

env:
KEEP_BUILD_ARTIFACTS: '1'
MEDIASOUP_BUILDTYPE: 'Release'

steps:
- name: Checkout
Expand Down
111 changes: 111 additions & 0 deletions .github/workflows/mediasoup-worker-debug.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
name: mediasoup-worker

on: [pull_request, workflow_dispatch]

concurrency:
# Cancel a currently running workflow from the same PR, branch or tag when a
# new workflow is triggered.
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

jobs:
ci:
strategy:
matrix:
build:
- os: ubuntu-20.04
cc: gcc
cxx: g++
- os: ubuntu-20.04
cc: clang
cxx: clang++
- os: ubuntu-22.04
cc: gcc
cxx: g++
- os: ubuntu-22.04
cc: clang
cxx: clang++
- os: ubuntu-20.04
cc: gcc
cxx: g++
arch: arm64
- os: ubuntu-20.04
cc: clang
cxx: clang++
arch: arm64
- os: ubuntu-22.04
cc: gcc
cxx: g++
arch: arm64
- os: ubuntu-22.04
cc: clang
cxx: clang++
arch: arm64
- os: macos-12
cc: gcc
cxx: g++
- os: macos-14
cc: clang
cxx: clang++
- os: windows-2022
cc: cl
cxx: cl
# A single Node.js version should be fine for C++.
node:
- 20

runs-on: ${{ matrix.build.os }}

env:
CC: ${{ matrix.build.cc }}
CXX: ${{ matrix.build.cxx }}
MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true'
MEDIASOUP_LOCAL_DEV: 'true'
MEDIASOUP_BUILDTYPE: 'Debug'

steps:
- name: Checkout
uses: actions/checkout@v4

- name: Node.js
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node }}

- name: Configure cache
uses: actions/cache@v3
with:
path: |
~/.npm
key: ${{ matrix.build.os }}-node-${{matrix.build.cc}}-${{ hashFiles('**/package.json') }}
restore-keys: |
${{ matrix.build.os }}-node-${{matrix.build.cc}}-
# We need to install pip invoke manually.
- name: pip3 install invoke
run: pip3 install invoke
if: runner.os != 'macOS'

# In macOS we need to specify this option.
- name: pip3 install --break-system-packages invoke
run: pip3 install --break-system-packages invoke
if: runner.os == 'macOS'

# We need to install npm deps of worker/scripts/package.json.
- name: npm ci --prefix worker/scripts
run: npm ci --prefix worker/scripts --foreground-scripts
# TODO: Maybe fix this one day.
if: runner.os != 'Windows'

- name: invoke -r worker lint
run: invoke -r worker lint
# TODO: Maybe fix this one day.
if: runner.os != 'Windows'

- name: invoke -r worker mediasoup-worker
run: invoke -r worker mediasoup-worker

- name: invoke -r worker test
run: invoke -r worker test
# TODO: Maybe fix this one day.
if: runner.os != 'Windows'
53 changes: 53 additions & 0 deletions .github/workflows/mediasoup-worker-fuzzer-debug.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
name: mediasoup-worker-fuzzer

on: [pull_request, workflow_dispatch]

concurrency:
# Cancel a currently running workflow from the same PR, branch or tag when a
# new workflow is triggered.
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

jobs:
ci:
strategy:
matrix:
build:
- os: ubuntu-22.04
cc: clang
cxx: clang++
- os: ubuntu-22.04
cc: clang
cxx: clang++
arch: arm64

runs-on: ${{ matrix.build.os }}

env:
CC: ${{ matrix.build.cc }}
CXX: ${{ matrix.build.cxx }}
MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true'
MEDIASOUP_LOCAL_DEV: 'true'
MEDIASOUP_BUILDTYPE: 'Debug'

steps:
- name: Checkout
uses: actions/checkout@v4

# We need to install pip invoke manually.
- name: pip3 install invoke
run: pip3 install invoke
if: runner.os != 'macOS'

# In macOS we need to specify this option.
- name: pip3 install --break-system-packages invoke
run: pip3 install --break-system-packages invoke
if: runner.os == 'macOS'

# Build the mediasoup-worker-fuzzer binary (which uses libFuzzer).
- name: invoke -r worker fuzzer
run: invoke -r worker fuzzer

# Run mediasoup-worker-fuzzer for 5 minutes.
- name: run-fuzzer.sh 300
run: cd worker && ./scripts/run-fuzzer.sh 300
1 change: 1 addition & 0 deletions .github/workflows/mediasoup-worker-fuzzer.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ jobs:
CXX: ${{ matrix.build.cxx }}
MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true'
MEDIASOUP_LOCAL_DEV: 'true'
MEDIASOUP_BUILDTYPE: 'Release'

steps:
- name: Checkout
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/mediasoup-worker-prebuild.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/mediasoup-worker.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ jobs:
CXX: ${{ matrix.build.cxx }}
MEDIASOUP_SKIP_WORKER_PREBUILT_DOWNLOAD: 'true'
MEDIASOUP_LOCAL_DEV: 'true'
MEDIASOUP_BUILDTYPE: 'Release'

steps:
- name: Checkout
Expand Down
17 changes: 10 additions & 7 deletions worker/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ use std::process::Command;
use std::{env, fs};

fn main() {
// On Windows Rust always links against release version of MSVC runtime, thus requires
// Release build here
let build_type = if cfg!(all(debug_assertions, not(windows))) {
"Debug"
} else {
"Release"
};
// Honor MEDIASOUP_BUILDTYPE environment variable if given
// On Windows Rust always links against release version of MSVC runtime,
// thus requires Release build
let build_type = env::var("MEDIASOUP_BUILDTYPE").unwrap_or_else(|_| {
if cfg!(all(debug_assertions, not(windows))) {
"Debug".to_string()
} else {
"Release".to_string()
}
});

let out_dir = env::var("OUT_DIR").unwrap();

Expand Down
8 changes: 3 additions & 5 deletions worker/include/RTC/NackGenerator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
#include "RTC/RtpPacket.hpp"
#include "RTC/SeqManager.hpp"
#include "handles/TimerHandle.hpp"
#include <absl/container/btree_map.h>
#include <absl/container/btree_set.h>
#include <map>
#include <set>
#include <vector>
Expand Down Expand Up @@ -80,9 +78,9 @@ namespace RTC
// Allocated by this.
TimerHandle* timer{ nullptr };
// Others.
absl::btree_map<uint16_t, NackInfo, RTC::SeqManager<uint16_t>::SeqLowerThan> nackList;
absl::btree_set<uint16_t, RTC::SeqManager<uint16_t>::SeqLowerThan> keyFrameList;
absl::btree_set<uint16_t, RTC::SeqManager<uint16_t>::SeqLowerThan> recoveredList;
std::map<uint16_t, NackInfo, RTC::SeqManager<uint16_t>::SeqLowerThan> nackList;
std::set<uint16_t, RTC::SeqManager<uint16_t>::SeqLowerThan> keyFrameList;
std::set<uint16_t, RTC::SeqManager<uint16_t>::SeqLowerThan> recoveredList;
bool started{ false };
uint16_t lastSeq{ 0u }; // Seq number of last valid packet.
uint32_t rtt{ 0u }; // Round trip time (ms).
Expand Down
Loading

0 comments on commit 479e7b1

Please sign in to comment.