diff --git a/.clang-tidy b/.clang-tidy index 44918701..aa215469 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -9,6 +9,7 @@ Checks: > -bugprone-easily-swappable-parameters, -modernize-use-trailing-return-type, -modernize-avoid-c-arrays, + -modernize-type-traits, -modernize-use-auto, -modernize-use-nodiscard, -modernize-return-braced-init-list, @@ -29,7 +30,8 @@ WarningsAsErrors: > bugprone-string-literal-with-embedded-nul, bugprone-suspicious-memset-usage, -HeaderFilterRegex: '(include/livekit|src|examples)' +HeaderFilterRegex: '.*/(include/livekit|src)/.*\.(h|hpp)$' +ExcludeHeaderFilterRegex: '(.*/src/tests/.*)|(.*/_deps/.*)|(.*/build-[^/]*/.*)' FormatStyle: file diff --git a/.github/workflows/builds.yml b/.github/workflows/builds.yml index 94829ac0..784687d2 100644 --- a/.github/workflows/builds.yml +++ b/.github/workflows/builds.yml @@ -553,7 +553,6 @@ jobs: continue-on-error: false permissions: contents: read - pull-requests: write steps: - name: Checkout (with submodules) @@ -568,8 +567,28 @@ jobs: sudo apt-get update sudo apt-get install -y \ build-essential cmake ninja-build pkg-config \ - llvm-dev libclang-dev clang clang-tidy \ - libssl-dev + llvm-dev libclang-dev clang \ + libssl-dev wget ca-certificates gnupg + + - name: Install clang-tidy 19 (for ExcludeHeaderFilterRegex support) + run: | + set -eux + # Ubuntu 24.04 apt ships clang-tidy 18, which doesn't understand + # ExcludeHeaderFilterRegex (added in 19). Pull clang-tidy 19 from + # the upstream LLVM apt repository and pin the unversioned names. + sudo install -m 0755 -d /etc/apt/keyrings + wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key \ + | sudo tee /etc/apt/keyrings/llvm.asc >/dev/null + sudo chmod a+r /etc/apt/keyrings/llvm.asc + codename=$(lsb_release -cs) + echo "deb [signed-by=/etc/apt/keyrings/llvm.asc] http://apt.llvm.org/${codename}/ llvm-toolchain-${codename}-19 main" \ + | sudo tee /etc/apt/sources.list.d/llvm-19.list >/dev/null + sudo apt-get update + sudo apt-get install -y clang-tidy-19 clang-tools-19 + sudo ln -sf /usr/bin/clang-tidy-19 /usr/local/bin/clang-tidy + sudo ln -sf /usr/bin/run-clang-tidy-19 /usr/local/bin/run-clang-tidy + clang-tidy --version + run-clang-tidy --help | head -1 || true - name: Install Rust (stable) uses: dtolnay/rust-toolchain@3c5f7ea28cd621ae0bf5283f0e981fb97b8a7af9 @@ -590,33 +609,9 @@ jobs: run: cmake --build build-release --target livekit_proto - name: Run clang-tidy - uses: cpp-linter/cpp-linter-action@77c390c5ba9c947ebc185a3e49cc754f1558abb5 # v2.18.0 - id: linter env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - with: - style: '' - tidy-checks: '' - database: build-release - files-changed-only: false - lines-changed-only: true - extensions: 'c,cpp,cc,cxx' - ignore: 'build-*|cpp-example-collection|client-sdk-rust|vcpkg_installed|src/tests|bridge|src/room_event_converter.cpp' - file-annotations: true - thread-comments: false - step-summary: true - tidy-review: false - no-lgtm: true - jobs: 0 # 0 == use all available CPU cores - - - name: Check warning thresholds - env: - TIDY_FINDINGS: ${{ steps.linter.outputs.clang-tidy-checks-failed }} - MAX_TIDY_FINDINGS: '0' - run: | - echo "clang-tidy findings: ${TIDY_FINDINGS}" - if [ "${TIDY_FINDINGS}" -gt "${MAX_TIDY_FINDINGS}" ]; then - echo "::warning::clang-tidy found ${TIDY_FINDINGS} issue(s), threshold is ${MAX_TIDY_FINDINGS}" - exit 1 - fi - echo "clang-tidy findings within threshold" + TIDY_BLOB_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + # This script is intended to be run locally and in CI. It will auto-detect + # the GHA environment and add PR annotations and a run summary. + # As of writing all warnings are treateded as errors to avoid tech debt build up + run: ./scripts/clang-tidy.sh --fail-on-warning diff --git a/.gitignore b/.gitignore index dc29c3fe..2a54c6ab 100644 --- a/.gitignore +++ b/.gitignore @@ -30,7 +30,7 @@ lib/ *.dylib *.dll *.exe -livekit.log +*.log web/ *trace.json compile_commands.json diff --git a/AGENTS.md b/AGENTS.md index 7656e301..f6a0d883 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,6 +79,7 @@ Be sure to update the directory layout in this file if the directory layout chan | `client-sdk-rust/livekit-ffi/protocol/*.proto` | FFI contract (protobuf definitions, read-only reference) | | `cmake/` | Build helpers (`protobuf.cmake`, `spdlog.cmake`, `LiveKitConfig.cmake.in`) | | `docker/` | Dockerfile for CI and SDK distribution images | +| `scripts/` | Developer / CI helper scripts (e.g. `clang-tidy.sh`) | | `.github/workflows/` | GitHub Actions CI workflows | ### Key Types diff --git a/README.md b/README.md index 25558314..6136542b 100644 --- a/README.md +++ b/README.md @@ -522,13 +522,28 @@ sudo apt-get install clang-format clang-tidy clang-tools To run: -1. Run a build script command, such that `compile_command.json` is present at the root of the repository -2. Run: +1. Generate `compile_commands.json` and the protobuf headers via a release build: ```bash -clang-tidy -p . src/*.cpp +./build.sh release ``` +1. Run the clang-tidy wrapper, which uses the same file set, regex filters, and `.clang-tidy` config as CI: + +```bash +./scripts/clang-tidy.sh +``` + +The wrapper forwards extra arguments to `run-clang-tidy`, examples below: + +```bash +./scripts/clang-tidy.sh -j 4 # Change number of cores used +./scripts/clang-tidy.sh -checks='-*,misc-const-correctness' # Only run certain checks +./scripts/clang-tidy.sh -fix # Apply fixes automatically +``` + +Output is captured to `clang-tidy.log` at the repo root. This is done as a convenience feature, as often times the terminal buffer is not large enough for all the output. + ### Memory Checks Run valgrind on various examples or tests to check for memory leaks and other issues. diff --git a/include/livekit/audio_stream.h b/include/livekit/audio_stream.h index 334d7615..d6dce270 100644 --- a/include/livekit/audio_stream.h +++ b/include/livekit/audio_stream.h @@ -41,9 +41,13 @@ class FfiEvent; * This struct wraps an AudioFrame and is used as the output type when * reading from an AudioStream. */ +// NOLINTBEGIN(bugprone-exception-escape) +// AudioFrame can throw in various places monitored by bugprone-exception-escape +// Suppressing for now, would require significant refactor to fix struct AudioFrameEvent { AudioFrame frame; ///< The decoded PCM audio frame. }; +// NOLINTEND(bugprone-exception-escape) /** * Represents a pull-based stream of decoded PCM audio frames coming from diff --git a/include/livekit/local_track_publication.h b/include/livekit/local_track_publication.h index d00d863f..f2ccd346 100644 --- a/include/livekit/local_track_publication.h +++ b/include/livekit/local_track_publication.h @@ -24,16 +24,11 @@ namespace proto { class OwnedTrackPublication; } -class Track; - class LocalTrackPublication : public TrackPublication { public: /// Note, this LocalTrackPublication is constructed internally only; /// safe to accept proto::OwnedTrackPublication. explicit LocalTrackPublication(const proto::OwnedTrackPublication &owned); - - /// Typed accessor for the attached LocalTrack (if any). - std::shared_ptr track() const noexcept; }; } // namespace livekit diff --git a/include/livekit/remote_track_publication.h b/include/livekit/remote_track_publication.h index aa394081..212eabd3 100644 --- a/include/livekit/remote_track_publication.h +++ b/include/livekit/remote_track_publication.h @@ -24,17 +24,12 @@ namespace proto { class OwnedTrackPublication; } -class Track; - class RemoteTrackPublication : public TrackPublication { public: /// Note, this RemoteTrackPublication is constructed internally only; /// safe to accept proto::OwnedTrackPublication. explicit RemoteTrackPublication(const proto::OwnedTrackPublication &owned); - - /// Typed accessor for the attached RemoteTrack (if any). - std::shared_ptr track() const noexcept; - + bool subscribed() const noexcept { return subscribed_; } void setSubscribed(bool subscribed); diff --git a/include/livekit/result.h b/include/livekit/result.h index bc7f1338..dce0e874 100644 --- a/include/livekit/result.h +++ b/include/livekit/result.h @@ -64,49 +64,62 @@ template class [[nodiscard]] Result { /// Allows `if (result)` style success checks. explicit operator bool() const noexcept { return ok(); } + // TODO (AEG): clang-tidy flagged these accessors because the signatures are marked + // noexcept, but std::get can throw a std::bad_variant_access exception on + // std::variant specifically. Investigate if this is actually a concern or if the types + // are safe within this class (unit test ideal). + /// Access the success value. Requires `ok() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) T &value() & noexcept { assert(ok()); return std::get<0>(storage_); } /// Access the success value. Requires `ok() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) const T &value() const & noexcept { assert(ok()); return std::get<0>(storage_); } /// Move the success value out. Requires `ok() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) T &&value() && noexcept { assert(ok()); return std::get<0>(std::move(storage_)); } /// Move the success value out. Requires `ok() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) const T &&value() const && noexcept { assert(ok()); return std::get<0>(std::move(storage_)); } /// Access the error value. Requires `has_error() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) E &error() & noexcept { assert(has_error()); return std::get<1>(storage_); } /// Access the error value. Requires `has_error() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) const E &error() const & noexcept { assert(has_error()); return std::get<1>(storage_); } /// Move the error value out. Requires `has_error() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) E &&error() && noexcept { assert(has_error()); return std::get<1>(std::move(storage_)); } /// Move the error value out. Requires `has_error() == true`. + // NOLINTNEXTLINE(bugprone-exception-escape) const E &&error() const && noexcept { assert(has_error()); return std::get<1>(std::move(storage_)); diff --git a/include/livekit/subscription_thread_dispatcher.h b/include/livekit/subscription_thread_dispatcher.h index c647c565..8e5fa65c 100644 --- a/include/livekit/subscription_thread_dispatcher.h +++ b/include/livekit/subscription_thread_dispatcher.h @@ -471,7 +471,7 @@ class SubscriptionThreadDispatcher { active_readers_; /// Next auto-increment ID for data frame callbacks. - DataFrameCallbackId next_data_callback_id_; + DataFrameCallbackId next_data_callback_id_{0}; /// Registered data frame callbacks keyed by opaque callback ID. std::unordered_map diff --git a/include/livekit/track.h b/include/livekit/track.h index eeff571a..6d1e98c4 100644 --- a/include/livekit/track.h +++ b/include/livekit/track.h @@ -85,6 +85,8 @@ class Track { std::optional simulcasted() const noexcept { return simulcasted_; } std::optional width() const noexcept { return width_; } std::optional height() const noexcept { return height_; } + // std::string can actually throw, suppressing for now to maintain API compatibility + // NOLINTNEXTLINE(bugprone-exception-escape) std::optional mime_type() const noexcept { return mime_type_; } // Handle access diff --git a/scripts/clang-tidy.sh b/scripts/clang-tidy.sh new file mode 100755 index 00000000..edcca5b9 --- /dev/null +++ b/scripts/clang-tidy.sh @@ -0,0 +1,523 @@ +#!/usr/bin/env bash +# +# Copyright 2026 LiveKit +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# clang-tidy.sh -- Run clang-tidy locally and in CI using the same file set +# and config. Picks up checks from the repo-root .clang-tidy automatically. +# +# Usage (from anywhere; the script self-anchors to the repo root): +# ./scripts/clang-tidy.sh # run on full src/ tree +# ./scripts/clang-tidy.sh -j 4 # override parallelism +# ./scripts/clang-tidy.sh --github-actions # force GitHub Actions annotation mode +# ./scripts/clang-tidy.sh --fail-on-warning # exit non-zero on any finding (warning OR error) +# ./scripts/clang-tidy.sh -fix # forwarded to run-clang-tidy +# +# Every run prints a concise stdout summary at the end with the warning and +# error counts (and a per-check breakdown when there are findings). +# +# In GitHub Actions (auto-detected via $GITHUB_ACTIONS=true, or forced with +# --github-actions), this script additionally: +# - Emits ::warning/::error workflow commands so findings appear as PR file +# annotations (yellow for warnings, red for errors). Severity comes from +# clang-tidy itself -- errors are findings promoted by WarningsAsErrors +# in .clang-tidy. +# - Writes a markdown summary to $GITHUB_STEP_SUMMARY. +# +# Exit code: +# - Non-zero when run-clang-tidy itself reports errors (findings promoted +# to errors via WarningsAsErrors in .clang-tidy). +# - Also non-zero when --fail-on-warning is set and at least one warning +# was emitted. This is what CI uses to gate merges so warning-level +# tech debt cannot accumulate silently. +# - Zero otherwise. +# +# Requires CMake to have generated build-release/compile_commands.json. +# Run once: ./build.sh release (configures, builds, generates protobuf headers) + +set -euo pipefail + +# Anchor every relative path (BUILD_DIR, clang-tidy.log, etc.) to the repo root +# regardless of how/where this script is invoked. Without this, calling the +# script from a subdirectory or via an absolute path would break the +# compile_commands.json / generated/ checks below. +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +cd "${script_dir}/.." + +# Usage banner. Printed by --help and referenced by the EXIT trap below so +# users who hit a pre-flight error or a bad argument get pointed at this. +usage() { + cat <<'EOF' +Usage: ./scripts/clang-tidy.sh [OPTIONS] [run-clang-tidy args...] + +Run clang-tidy locally or in CI using the same file set and config that the +repo's CI workflow uses. Picks up checks from the repo-root .clang-tidy. + +Options: + -h, --help, -? + Show this help and exit. + --github-actions, --gh + Force GitHub Actions annotation + step-summary mode. + Auto-detected when GITHUB_ACTIONS=true. + --fail-on-warning, --strict + Exit non-zero when any warning is emitted (errors already exit + non-zero on their own). Off by default for local edit/run cycles; + CI opts in via the workflow file. + +Any other arguments are forwarded verbatim to run-clang-tidy. Common ones: + -j N worker count (defaults to nproc / sysctl hw.ncpu) + -fix apply fixes in place + -checks=... override the .clang-tidy check list for this run + +Examples: + ./scripts/clang-tidy.sh # full src/ tree + ./scripts/clang-tidy.sh -j 4 # override parallelism + ./scripts/clang-tidy.sh --github-actions # force CI annotation mode + ./scripts/clang-tidy.sh --fail-on-warning # exit non-zero on any finding + ./scripts/clang-tidy.sh -fix # forwarded to run-clang-tidy + +Pre-requisites: + CMake must have generated build-release/compile_commands.json AND the + protobuf headers under build-release/generated/. Run once: + ./build.sh release +EOF +} + +# Print a one-line "see --help" hint on any non-zero exit during pre-flight +# (argument parsing, missing build artifacts, missing run-clang-tidy, etc.). +# It is suppressed once we hand off to run-clang-tidy itself, so legitimate +# clang-tidy findings don't trigger the hint. +__tidy_hint_active=1 +__tidy_print_hint() { + local rc=$? + if (( rc != 0 )) && (( __tidy_hint_active )); then + echo >&2 + echo "Run './scripts/clang-tidy.sh --help' for usage." >&2 + fi +} +trap __tidy_print_hint EXIT + +BUILD_DIR="build-release" +# Positive match for top-level src/*.{c,cpp,cc,cxx}; negative lookahead excludes +# dep paths (_deps/, build-*/, -src/src/) and every other top-level dir. Python +# regex (PCRE-ish) supports lookahead; this regex is evaluated by run-clang-tidy. +FILE_REGEX='^(?!.*/(_deps|build-[^/]*|bridge|examples|client-sdk-rust|cpp-example-collection|vcpkg_installed|docker|docs|data)/).*/src/(?!tests/).*\.(c|cpp|cc|cxx)$' + +CI_MODE=0 +# Automatically detect CI mode if in GitHub actions environment +if [[ "${GITHUB_ACTIONS:-}" == "true" ]]; then + CI_MODE=1 +fi + +# When set, the script exits non-zero on any finding (warning or error), +# not just on errors. Off by default so local edit/run cycles aren't blocked +# by in-progress code; CI opts in via the workflow file. +FAIL_ON_WARNING=0 + +forward_args=() +while (($#)); do + case "$1" in + -h|--help|-\?) + usage + __tidy_hint_active=0 + exit 0 + ;; + --github-actions|--gh) + CI_MODE=1 + shift + ;; + --fail-on-warning|--strict) + FAIL_ON_WARNING=1 + shift + ;; + --) + # Explicit forwarding separator: everything after `--` goes straight + # to run-clang-tidy without further interpretation. + shift + forward_args+=("$@") + break + ;; + --*) + # Long options we don't recognize are user typos far more often than + # genuine run-clang-tidy long options (run-clang-tidy uses single-dash + # long options like -fix, -j, -checks=...). Reject and surface usage. + echo "ERROR: unknown option: $1" >&2 + usage >&2 + exit 2 + ;; + *) + forward_args+=("$1") + shift + ;; + esac +done + +if [[ ! -f "${BUILD_DIR}/compile_commands.json" ]]; then + echo "ERROR: ${BUILD_DIR}/compile_commands.json not found." >&2 + echo "Run: ./build.sh release (configures + builds, generates protobuf headers)" >&2 + exit 1 +fi + +# Protobuf sanity check. `cmake --preset` only configures -- it doesn't invoke +# the Rust/protoc chain that writes livekit's generated headers into +# build-release/generated/. If those headers are missing or stale, every TU +# that #includes "room.pb.h" / "ffi.pb.h" / etc. fails with dozens of +# clang-diagnostic-error diagnostics (e.g. "no member named 'FrameMetadata' in +# namespace 'livekit::proto'") that drown out the real findings. Detect the +# "configured but never built" state early and point the user at ./build.sh. +proto_dir="${BUILD_DIR}/generated" +if [[ ! -d "${proto_dir}" ]] || ! compgen -G "${proto_dir}/*.pb.h" >/dev/null; then + echo "ERROR: no generated protobuf headers found in ${proto_dir}/." >&2 + echo "clang-tidy needs .pb.h files that are produced during the build step," >&2 + echo "not by 'cmake --preset' alone. To generate them, run:" >&2 + echo "" >&2 + echo " ./build.sh release # or release-tests / release-all, as needed" >&2 + echo "" >&2 + echo "If you previously bumped client-sdk-rust, also run 'clean-all' first:" >&2 + echo "" >&2 + echo " ./build.sh clean-all && ./build.sh release" >&2 + exit 1 +fi + +if ! command -v run-clang-tidy >/dev/null 2>&1; then + echo "ERROR: run-clang-tidy not found in PATH." >&2 + echo "Install LLVM: brew install llvm (macOS)" >&2 + echo " apt install clang-tools-NN (Linux)" >&2 + exit 1 +fi + +# On macOS, the C++ standard library headers (, , ...) live +# inside the active Xcode / Command Line Tools SDK rather than on the default +# include path. Homebrew's clang-tidy doesn't know where that SDK is, so +# without -isysroot it fails every TU with "'cstdint' file not found" before +# any real check runs. `xcrun --show-sdk-path` resolves the currently selected +# SDK and we forward it to every clang-tidy invocation via --extra-arg. Linux +# CI doesn't need this -- the system clang-tidy already finds libstdc++/libc++ +# through its built-in resource dir. +extra_args=() +if [[ "$(uname)" == "Darwin" ]]; then + sdk_path="$(xcrun --show-sdk-path 2>/dev/null || true)" + if [[ -n "${sdk_path}" ]]; then + extra_args+=(-extra-arg="-isysroot${sdk_path}") + fi +fi + +# run-clang-tidy parallelizes across TUs via -j. Default to one worker per +# logical CPU so local runs aren't artificially slow. `nproc` is the Linux +# coreutils tool; macOS doesn't ship it, so fall back to `sysctl hw.ncpu`, +# and finally to a conservative 4 if neither is available (e.g. a minimal +# container). +if command -v nproc >/dev/null 2>&1; then + jobs=$(nproc) +else + jobs=$(sysctl -n hw.ncpu 2>/dev/null || echo 4) +fi + +# -------- Begin GitHub Actions annotations -------- + +# Emit GitHub Actions workflow commands for each clang-tidy diagnostic line +# in the given log. Notes (`path:L:C: note: ...`) are deliberately skipped -- +# they belong to the preceding warning/error and would produce noisy extra +# annotations. Severity (::warning vs ::error) mirrors clang-tidy's prefix. +emit_annotations() { + local log="$1" + local workspace="${GITHUB_WORKSPACE:-${PWD}}" + local line path lineno col severity message check rel_path + + while IFS= read -r line; do + [[ "${line}" =~ ^(.+):([0-9]+):([0-9]+):[[:space:]]+(warning|error):[[:space:]]+(.+)[[:space:]]\[([^]]+)\][[:space:]]*$ ]] || continue + path="${BASH_REMATCH[1]}" + lineno="${BASH_REMATCH[2]}" + col="${BASH_REMATCH[3]}" + severity="${BASH_REMATCH[4]}" + message="${BASH_REMATCH[5]}" + check="${BASH_REMATCH[6]}" + # clang-tidy appends ",-warnings-as-errors" to the bracket suffix when a + # diagnostic is promoted from warning to error via WarningsAsErrors. Strip + # it so the title, docs link, and Top-checks bucket are identical for the + # warning and error code paths -- severity is already conveyed by the + # ::warning / ::error workflow command prefix below. + check="${check%,-warnings-as-errors}" + + rel_path="${path#${workspace}/}" + + message="${message//$'%'/%25}" + message="${message//$'\r'/%0D}" + message="${message//$'\n'/%0A}" + + printf '::%s file=%s,line=%s,col=%s,title=clang-tidy (%s)::%s\n' \ + "${severity}" "${rel_path}" "${lineno}" "${col}" "${check}" "${message}" + done < "${log}" +} + +# Append a markdown summary (counts + top checks + full finding list) to +# $GITHUB_STEP_SUMMARY so the GitHub job page surfaces every finding without +# needing to scan the raw log. Counts require a [check-name] suffix so +# clang-tidy's own config-parse errors can't inflate the totals. +write_step_summary() { + local log="$1" + local summary_file="${GITHUB_STEP_SUMMARY:-}" + [[ -n "${summary_file}" ]] || return 0 + + local workspace="${GITHUB_WORKSPACE:-${PWD}}" + local findings_tsv + findings_tsv="$(mktemp -t tidy-findings.XXXXXX)" + + # Extract every real finding (severity must be followed by [check-name]) + # as tab-separated severity\tpath\tline\tcol\tcheck\tmessage. Use bash's + # regex engine (same as emit_annotations) for portability -- BSD awk and + # mawk don't support gawk's 3-argument match(). + local sline spath slineno scol sseverity smessage scheck + while IFS= read -r sline; do + [[ "${sline}" =~ ^(.+):([0-9]+):([0-9]+):[[:space:]]+(warning|error):[[:space:]]+(.+)[[:space:]]\[([^]]+)\][[:space:]]*$ ]] || continue + spath="${BASH_REMATCH[1]#${workspace}/}" + slineno="${BASH_REMATCH[2]}" + scol="${BASH_REMATCH[3]}" + sseverity="${BASH_REMATCH[4]}" + smessage="${BASH_REMATCH[5]}" + scheck="${BASH_REMATCH[6]}" + # Strip the ",-warnings-as-errors" promotion marker so error rows bucket + # under the same check name as their warning counterparts (see + # emit_annotations for the rationale). + scheck="${scheck%,-warnings-as-errors}" + printf '%s\t%s\t%s\t%s\t%s\t%s\n' \ + "${sseverity}" "${spath}" "${slineno}" "${scol}" "${scheck}" "${smessage}" \ + >> "${findings_tsv}" + done < "${log}" + + local warnings errors total + warnings=$(awk -F'\t' '$1=="warning"{c++} END{print c+0}' "${findings_tsv}") + errors=$(awk -F'\t' '$1=="error"{c++} END{print c+0}' "${findings_tsv}") + total=$((warnings + errors)) + + # Resolve a blob URL prefix so findings in the table below become clickable + # links to github.com. Prefer TIDY_BLOB_SHA (set by the workflow to the PR + # head SHA) over GITHUB_SHA -- on pull_request events GITHUB_SHA points at + # the ephemeral refs/pull/N/merge commit, whose blob URLs stop resolving + # once the PR closes. On push / workflow_dispatch / schedule runs + # TIDY_BLOB_SHA is unset and we fall through to GITHUB_SHA, which is the + # pushed / selected commit respectively. When neither is set (local runs), + # repo_url stays empty and the file column renders as plain code. + local repo_url="" + if [[ -n "${GITHUB_SERVER_URL:-}" && -n "${GITHUB_REPOSITORY:-}" ]]; then + local blob_sha="${TIDY_BLOB_SHA:-${GITHUB_SHA:-}}" + if [[ -n "${blob_sha}" ]]; then + repo_url="${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/blob/${blob_sha}" + fi + fi + + # Render a check name as a markdown link to its official clang-tidy docs page + # (mirrors what cpp-linter-action used to do). The canonical URL layout is + # https://clang.llvm.org/extra/clang-tidy/checks//.html + # where is everything up to the first '-'. Two categories don't + # follow that layout: + # * clang-diagnostic-* -- compiler diagnostics, no per-check doc page + # * clang-analyzer-* -- static analyzer, documented on a single page + # Those fall back to plain code formatting / the analyzer index page. + check_link() { + local name="$1" + local module="${name%%-*}" + local rest="${name#*-}" + case "${name}" in + clang-diagnostic-*) + printf '`%s`' "${name}" + ;; + clang-analyzer-*) + printf '[`%s`](https://clang.llvm.org/docs/analyzer/checkers.html)' "${name}" + ;; + *) + printf '[`%s`](https://clang.llvm.org/extra/clang-tidy/checks/%s/%s.html)' \ + "${name}" "${module}" "${rest}" + ;; + esac + } + + { + echo "## Analysis results" + echo + echo "| Severity | Count |" + echo "|----------|-------|" + # Same GFM shortcodes used in the detailed findings table below, so the + # count summary and per-finding severity column read consistently. + echo "| :x: Error | ${errors} |" + echo "| :warning: Warning | ${warnings} |" + echo + + if (( total > 0 )); then + echo "### Top checks" + echo + echo '| Check | Count |' + echo '|-------|-------|' + awk -F'\t' '{print $5}' "${findings_tsv}" \ + | sort | uniq -c | sort -rn | head -5 \ + | while read -r count name; do + printf '| %s | %d |\n' "$(check_link "${name}")" "${count}" + done + echo + + echo "
All ${total} findings" + echo + echo '| Severity | File | Check | Message |' + echo '|----------|------|-------|---------|' + # Stream errors first, then warnings, by running two awk passes over the + # TSV. This preserves log-discovery order within each severity bucket + # without relying on `sort -s` (whose stable-sort semantics differ + # subtly between GNU and BSD coreutils). + { + awk -F'\t' '$1=="error"' "${findings_tsv}" + awk -F'\t' '$1=="warning"' "${findings_tsv}" + } | while IFS=$'\t' read -r sev path lineno col check msg; do + msg="${msg//|/\\|}" + local icon label file_cell + # GFM emoji shortcodes render in step summaries; :x: (red X) and + # :warning: (yellow triangle) are the closest visual analogs to + # GitHub's native annotation pills shown in the PR review UI. The + # title-cased label matches the count table at the top of the summary. + if [[ "${sev}" == "error" ]]; then + icon=':x:' + label='Error' + else + icon=':warning:' + label='Warning' + fi + # Link to github.com when we have a blob URL and a repo-relative path. + # Absolute paths (leading '/') are system headers that leaked past the + # note filter -- rendering them as a github.com link would 404, so + # fall back to plain code formatting. GitHub blob anchors support + # #L but not columns, so the column is kept in the label only. + if [[ -n "${repo_url}" && "${path}" != /* ]]; then + file_cell="[\`${path}:${lineno}\`](${repo_url}/${path}#L${lineno})" + else + file_cell="\`${path}:${lineno}\`" + fi + printf '| %s %s | %s | %s | %s |\n' \ + "${icon}" "${label}" "${file_cell}" "$(check_link "${check}")" "${msg}" + done + echo + echo "
" + echo + fi + } >> "${summary_file}" + + rm -f "${findings_tsv}" +} + +# Print a one-line summary plus a small per-check breakdown to stdout. Always +# runs (regardless of CI_MODE) so local invocations get the same headline view +# the GitHub step summary provides. Sets two globals consumed by the exit-code +# logic below: __TIDY_WARNINGS and __TIDY_ERRORS. The parsing regex matches +# the one used by emit_annotations / write_step_summary so the three views +# can't drift apart. +print_stdout_summary() { + local log="$1" + local workspace="${GITHUB_WORKSPACE:-${PWD}}" + local checks_tsv + checks_tsv="$(mktemp -t tidy-stdout.XXXXXX)" + + local line severity check + while IFS= read -r line; do + [[ "${line}" =~ ^(.+):([0-9]+):([0-9]+):[[:space:]]+(warning|error):[[:space:]]+(.+)[[:space:]]\[([^]]+)\][[:space:]]*$ ]] || continue + severity="${BASH_REMATCH[4]}" + check="${BASH_REMATCH[6]}" + # Match the annotation/summary normalization: strip the + # ",-warnings-as-errors" promotion suffix so a check buckets under one + # name regardless of severity. + check="${check%,-warnings-as-errors}" + printf '%s\t%s\n' "${severity}" "${check}" >> "${checks_tsv}" + done < "${log}" + + local warnings errors + warnings=$(awk -F'\t' '$1=="warning"{c++} END{print c+0}' "${checks_tsv}") + errors=$(awk -F'\t' '$1=="error"{c++} END{print c+0}' "${checks_tsv}") + + echo "------------------------------------------------------------" + if (( warnings == 0 && errors == 0 )); then + echo "clang-tidy summary: clean (0 warnings, 0 errors)" + else + printf 'clang-tidy summary: %d warning(s), %d error(s)\n' \ + "${warnings}" "${errors}" + echo " by check:" + awk -F'\t' '{print $2}' "${checks_tsv}" \ + | sort | uniq -c | sort -rn \ + | while read -r count name; do + printf ' %-50s %d\n' "${name}" "${count}" + done + fi + echo "------------------------------------------------------------" + + rm -f "${checks_tsv}" + __TIDY_WARNINGS="${warnings}" + __TIDY_ERRORS="${errors}" +} + +# --------- End GitHub Actions annotations --------- + +# Capture clang-tidy's combined stdout+stderr to a stable, repo-local path so +# it can be re-parsed after the run (for annotations and the step summary) and +# re-read by the user afterwards (e.g. `grep misc-const clang-tidy.log`). +# `*.log` is gitignored so this file never gets committed. Each run overwrites +# the previous log via `tee` (no -a), keeping the path predictable. +log="clang-tidy.log" + +# Past pre-flight: any non-zero exit from here on is a clang-tidy result +# (findings, internal error, etc.), not a user-facing argument/usage error, +# so suppress the "see --help" hint installed via the EXIT trap above. +__tidy_hint_active=0 + +set +e +# run-clang-tidy is a Python script that doesn't flush stdout in its per-file +# loop; it only flushes once at exit. When its stdout is a pipe (the `| tee` +# below), Python's stdio defaults to block-buffered mode, so diagnostics +# accumulate in an ~8 KB buffer and the user sees nothing until the run is +# essentially over. PYTHONUNBUFFERED=1 forces line/unbuffered writes so each +# file's findings appear as soon as that file's clang-tidy finishes. +# Empty-array expansion under `set -u` is treated as "unbound" on bash <4.4 +# (notably macOS's system /bin/bash 3.2 and minimal Linux containers). The +# `${arr[@]+"${arr[@]}"}` idiom expands to nothing when the array is empty +# and to the quoted elements when non-empty, sidestepping the issue +# portably back to bash 3.2. +PYTHONUNBUFFERED=1 run-clang-tidy \ + -p "${BUILD_DIR}" \ + -quiet \ + -j "${jobs}" \ + ${extra_args[@]+"${extra_args[@]}"} \ + ${forward_args[@]+"${forward_args[@]}"} \ + "${FILE_REGEX}" \ + 2>&1 | tee "${log}" +rc="${PIPESTATUS[0]}" +set -e + +if [[ "${CI_MODE}" == "1" ]]; then + emit_annotations "${log}" + write_step_summary "${log}" +fi + +echo "Results written to: $(cd "$(dirname "${log}")" && pwd)/$(basename "${log}")" + +# Always emit the concise headline summary to stdout. Sets __TIDY_WARNINGS / +# __TIDY_ERRORS globals which the strict-mode escalation below consumes. +__TIDY_WARNINGS=0 +__TIDY_ERRORS=0 +print_stdout_summary "${log}" + +# Strict mode: any warning escalates to a non-zero exit. Errors already make +# run-clang-tidy itself exit non-zero (rc != 0), so this only changes the +# warnings-only case. Used by CI to gate merges on a clean clang-tidy result. +if [[ "${FAIL_ON_WARNING}" == "1" && "${rc}" == "0" && "${__TIDY_WARNINGS}" -gt 0 ]]; then + echo "clang-tidy: --fail-on-warning is set and ${__TIDY_WARNINGS} warning(s) were emitted; exiting non-zero." >&2 + rc=1 +fi + +exit "${rc}" diff --git a/src/audio_stream.cpp b/src/audio_stream.cpp index 1f2a5798..07b1e693 100644 --- a/src/audio_stream.cpp +++ b/src/audio_stream.cpp @@ -55,7 +55,7 @@ AudioStream::fromParticipant(Participant &participant, TrackSource track_source, AudioStream::~AudioStream() { close(); } AudioStream::AudioStream(AudioStream &&other) noexcept { - std::lock_guard lock(other.mutex_); + const std::scoped_lock lock(other.mutex_); queue_ = std::move(other.queue_); capacity_ = other.capacity_; eof_ = other.eof_; @@ -76,8 +76,8 @@ AudioStream &AudioStream::operator=(AudioStream &&other) noexcept { close(); { - std::lock_guard lock_this(mutex_); - std::lock_guard lock_other(other.mutex_); + const std::scoped_lock lock_this(mutex_); + const std::scoped_lock lock_other(other.mutex_); queue_ = std::move(other.queue_); capacity_ = other.capacity_; @@ -110,7 +110,7 @@ bool AudioStream::read(AudioFrameEvent &out_event) { void AudioStream::close() { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_) { return; } @@ -221,7 +221,7 @@ void AudioStream::onFfiEvent(const FfiEvent &event) { void AudioStream::pushFrame(AudioFrameEvent &&ev) { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_ || eof_) { return; @@ -239,7 +239,7 @@ void AudioStream::pushFrame(AudioFrameEvent &&ev) { void AudioStream::pushEos() { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (eof_) { return; } diff --git a/src/data_stream.cpp b/src/data_stream.cpp index 8b1a6d73..9f846a27 100644 --- a/src/data_stream.cpp +++ b/src/data_stream.cpp @@ -38,7 +38,7 @@ std::string generateRandomId(std::size_t bytes = 16) { out.reserve(bytes * 2); const char *hex = "0123456789abcdef"; for (std::size_t i = 0; i < bytes; ++i) { - int v = dist(rng); + const int v = dist(rng); out.push_back(hex[(v >> 4) & 0xF]); out.push_back(hex[v & 0xF]); } @@ -109,7 +109,7 @@ TextStreamReader::TextStreamReader(TextStreamInfo info) void TextStreamReader::onChunkUpdate(const std::string &text) { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_) return; queue_.push_back(text); @@ -120,7 +120,7 @@ void TextStreamReader::onChunkUpdate(const std::string &text) { void TextStreamReader::onStreamClose( const std::map &trailer_attrs) { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); for (const auto &kv : trailer_attrs) { info_.attributes[kv.first] = kv.second; } @@ -154,7 +154,7 @@ ByteStreamReader::ByteStreamReader(ByteStreamInfo info) void ByteStreamReader::onChunkUpdate(const std::vector &bytes) { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_) return; queue_.push_back(bytes); @@ -165,7 +165,7 @@ void ByteStreamReader::onChunkUpdate(const std::vector &bytes) { void ByteStreamReader::onStreamClose( const std::map &trailer_attrs) { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); for (const auto &kv : trailer_attrs) { info_.attributes[kv.first] = kv.second; } @@ -308,13 +308,13 @@ TextStreamWriter::TextStreamWriter( } void TextStreamWriter::write(const std::string &text) { - std::lock_guard lock(write_mutex_); + const std::scoped_lock lock(write_mutex_); if (closed_) throw std::runtime_error("Cannot write to closed TextStreamWriter"); for (const auto &chunk_str : splitUtf8(text, kStreamChunkSize)) { const auto *p = reinterpret_cast(chunk_str.data()); - std::vector bytes(p, p + chunk_str.size()); + const std::vector bytes(p, p + chunk_str.size()); LK_LOG_DEBUG("sending chunk"); sendChunk(bytes); } @@ -339,7 +339,7 @@ ByteStreamWriter::ByteStreamWriter( } void ByteStreamWriter::write(const std::vector &data) { - std::lock_guard lock(write_mutex_); + const std::scoped_lock lock(write_mutex_); if (closed_) throw std::runtime_error("Cannot write to closed ByteStreamWriter"); @@ -348,7 +348,7 @@ void ByteStreamWriter::write(const std::vector &data) { const std::size_t n = std::min(kStreamChunkSize, data.size() - offset); auto it = data.begin() + static_cast(offset); - std::vector chunk(it, it + static_cast(n)); + const std::vector chunk(it, it + static_cast(n)); sendChunk(chunk); offset += n; } diff --git a/src/data_track_stream.cpp b/src/data_track_stream.cpp index 94188284..30a355c8 100644 --- a/src/data_track_stream.cpp +++ b/src/data_track_stream.cpp @@ -38,7 +38,7 @@ void DataTrackStream::init(FfiHandle subscription_handle) { bool DataTrackStream::read(DataTrackFrame &out) { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_ || eof_) { return false; } @@ -70,7 +70,7 @@ bool DataTrackStream::read(DataTrackFrame &out) { void DataTrackStream::close() { std::int32_t listener_id = -1; { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_) { return; } @@ -94,7 +94,7 @@ void DataTrackStream::onFfiEvent(const FfiEvent &event) { const auto &dts = event.data_track_stream_event(); { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_ || dts.stream_handle() != static_cast(subscription_handle_.get())) { return; @@ -111,7 +111,7 @@ void DataTrackStream::onFfiEvent(const FfiEvent &event) { } void DataTrackStream::pushFrame(DataTrackFrame &&frame) { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_ || eof_) { return; @@ -128,7 +128,7 @@ void DataTrackStream::pushFrame(DataTrackFrame &&frame) { void DataTrackStream::pushEos() { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (eof_) { return; } diff --git a/src/ffi_client.cpp b/src/ffi_client.cpp index c2d1829a..1f08d824 100644 --- a/src/ffi_client.cpp +++ b/src/ffi_client.cpp @@ -172,14 +172,14 @@ bool FfiClient::isInitialized() const noexcept { FfiClient::ListenerId FfiClient::AddListener(const FfiClient::Listener &listener) { - std::lock_guard guard(lock_); - FfiClient::ListenerId id = next_listener_id++; + const std::scoped_lock guard(lock_); + const FfiClient::ListenerId id = next_listener_id++; listeners_[id] = listener; return id; } void FfiClient::RemoveListener(ListenerId id) { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); listeners_.erase(id); } @@ -191,7 +191,7 @@ FfiClient::sendRequest(const proto::FfiRequest &request) const { } const uint8_t *resp_ptr = nullptr; size_t resp_len = 0; - FfiHandleId handle = + const FfiHandleId handle = livekit_ffi_request(reinterpret_cast(bytes.data()), bytes.size(), &resp_ptr, &resp_len); if (handle == INVALID_HANDLE) { @@ -216,7 +216,7 @@ void FfiClient::PushEvent(const proto::FfiEvent &event) const { std::unique_ptr to_complete; std::vector listeners_copy; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); // Complete pending future if this event is a callback with async_id if (auto async_id = ExtractAsyncId(event)) { @@ -259,7 +259,7 @@ FfiClient::AsyncId FfiClient::generateAsyncId() { bool FfiClient::cancelPendingByAsyncId(AsyncId async_id) { std::unique_ptr to_cancel; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); auto it = pending_by_id_.find(async_id); if (it != pending_by_id_.end()) { to_cancel = std::move(it->second); @@ -283,7 +283,7 @@ std::future FfiClient::registerAsync( pending->match = std::move(match); pending->handler = std::move(handler); { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); pending_by_id_.emplace(async_id, std::move(pending)); } return fut; @@ -455,7 +455,7 @@ FfiClient::getTrackStatsAsync(uintptr_t track_handle) { get_stats_req->set_request_async_id(async_id); try { - proto::FfiResponse resp = sendRequest(req); + const proto::FfiResponse resp = sendRequest(req); if (!resp.has_get_stats()) { logAndThrow("FfiResponse missing get_stats"); } @@ -502,7 +502,7 @@ FfiClient::publishTrackAsync(std::uint64_t local_participant_handle, } const proto::OwnedTrackPublication &pub = cb.publication(); - pr.set_value(std::move(pub)); + pr.set_value(pub); }); // Build and send the request @@ -701,7 +701,7 @@ FfiClient::subscribeDataTrack(std::uint64_t track_handle, } try { - proto::FfiResponse resp = sendRequest(req); + const proto::FfiResponse resp = sendRequest(req); if (!resp.has_subscribe_data_track()) { return Result::failure(SubscribeDataTrackError{ diff --git a/src/ffi_client.h b/src/ffi_client.h index bc5fd3e7..182dc143 100644 --- a/src/ffi_client.h +++ b/src/ffi_client.h @@ -32,6 +32,7 @@ #include "livekit/data_track_error.h" #include "livekit/result.h" #include "livekit/stats.h" +#include "lk_log.h" #include "room.pb.h" namespace livekit { @@ -188,8 +189,11 @@ class FfiClient { try { promise.set_exception(std::make_exception_ptr( std::runtime_error("Async operation cancelled"))); - } catch (const std::future_error &) { - // already satisfied + } catch (const std::future_error &e) { + // Unlikely to throw here as the promise should be satisfied before cancel() + // Logging a debug message to avoid clang empty catch warning + LK_LOG_DEBUG("FfiClient::cancel: promise already satisfied: {}", + e.what()); } } }; diff --git a/src/local_participant.cpp b/src/local_participant.cpp index 8fe31129..d33b7f2a 100644 --- a/src/local_participant.cpp +++ b/src/local_participant.cpp @@ -99,7 +99,7 @@ void LocalParticipant::publishDtmf(int code, const std::string &digit) { } // TODO, should we take destination as inputs? - std::vector destination_identities; + const std::vector destination_identities; auto fut = FfiClient::instance().publishSipDtmfAsync( static_cast(handle_id), static_cast(code), digit, destination_identities); @@ -212,7 +212,7 @@ void LocalParticipant::publishTrack(const std::shared_ptr &track, static_cast(track_handle), options); // Will throw if the async op fails (error in callback). - proto::OwnedTrackPublication owned_pub = fut.get(); + const proto::OwnedTrackPublication owned_pub = fut.get(); // Construct a LocalTrackPublication from the proto publication. auto publication = std::make_shared(owned_pub); @@ -416,7 +416,7 @@ void LocalParticipant::handleRpcMethodInvocation( // Track this invocation and check if we're shutting down { - std::lock_guard lock(state->mutex); + const std::scoped_lock lock(state->mutex); if (state->shutting_down) { // Already shutting down, don't process new invocations return; @@ -430,18 +430,18 @@ void LocalParticipant::handleRpcMethodInvocation( struct InvocationGuard { std::shared_ptr state; ~InvocationGuard() { - std::lock_guard lock(state->mutex); + const std::scoped_lock lock(state->mutex); state->active_invocations--; if (state->active_invocations == 0) { state->cv.notify_all(); } } - } guard{state}; + } const guard{state}; std::optional response_error; std::optional response_payload; - RpcInvocationData params{request_id, caller_identity, payload, - response_timeout_sec}; + const RpcInvocationData params{request_id, caller_identity, payload, + response_timeout_sec}; auto it = rpc_handlers_.find(method); if (it == rpc_handlers_.end()) { // No handler registered → built-in UNSUPPORTED_METHOD @@ -465,7 +465,7 @@ void LocalParticipant::handleRpcMethodInvocation( // Check again if shutdown started during handler execution { - std::lock_guard lock(state->mutex); + const std::scoped_lock lock(state->mutex); if (state->shutting_down) { // Shutdown started, don't send response - handle may be invalid return; diff --git a/src/local_track_publication.cpp b/src/local_track_publication.cpp index 24fc0de9..b98ead82 100644 --- a/src/local_track_publication.cpp +++ b/src/local_track_publication.cpp @@ -16,7 +16,6 @@ #include "livekit/local_track_publication.h" -#include "livekit/track.h" #include "track_proto_converter.h" namespace livekit { @@ -32,9 +31,4 @@ LocalTrackPublication::LocalTrackPublication( static_cast(owned.info().encryption_type()), convertAudioFeatures(owned.info().audio_features())) {} -std::shared_ptr LocalTrackPublication::track() const noexcept { - auto base = TrackPublication::track(); - return std::static_pointer_cast(base); -} - } // namespace livekit diff --git a/src/logging.cpp b/src/logging.cpp index e11af570..f8001e7b 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -91,7 +91,7 @@ std::shared_ptr createDefaultLogger() { namespace detail { std::shared_ptr getLogger() { - std::lock_guard lock(loggerMutex()); + const std::scoped_lock lock(loggerMutex()); auto &logger = loggerStorage(); if (!logger) { logger = createDefaultLogger(); @@ -101,7 +101,7 @@ std::shared_ptr getLogger() { } void shutdownLogger() { - std::lock_guard lock(loggerMutex()); + const std::scoped_lock lock(loggerMutex()); auto &logger = loggerStorage(); if (logger) { spdlog::drop(kLoggerName); @@ -118,7 +118,7 @@ void setLogLevel(LogLevel level) { LogLevel getLogLevel() { return fromSpdlogLevel(detail::getLogger()->level()); } void setLogCallback(LogCallback callback) { - std::lock_guard lock(loggerMutex()); + const std::scoped_lock lock(loggerMutex()); auto &logger = loggerStorage(); auto current_level = logger ? logger->level() : spdlog::level::info; diff --git a/src/remote_track_publication.cpp b/src/remote_track_publication.cpp index 10cf9623..c57b5586 100644 --- a/src/remote_track_publication.cpp +++ b/src/remote_track_publication.cpp @@ -18,7 +18,6 @@ #include "ffi.pb.h" #include "ffi_client.h" -#include "livekit/track.h" #include "track_proto_converter.h" namespace livekit { @@ -34,11 +33,6 @@ RemoteTrackPublication::RemoteTrackPublication( static_cast(owned.info().encryption_type()), convertAudioFeatures(owned.info().audio_features())) {} -std::shared_ptr RemoteTrackPublication::track() const noexcept { - auto base = TrackPublication::track(); - return std::static_pointer_cast(base); -} - void RemoteTrackPublication::setSubscribed(bool subscribed) { if (ffiHandleId() == 0) { throw std::runtime_error( diff --git a/src/room.cpp b/src/room.cpp index ae1af703..9eae941d 100644 --- a/src/room.cpp +++ b/src/room.cpp @@ -80,7 +80,7 @@ Room::~Room() { int listener_to_remove = 0; std::unique_ptr local_participant_to_cleanup; { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); listener_to_remove = listener_id_; listener_id_ = 0; // Move local participant out for cleanup outside the lock @@ -102,7 +102,7 @@ Room::~Room() { } void Room::setDelegate(RoomDelegate *delegate) { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); delegate_ = delegate; } @@ -111,7 +111,7 @@ bool Room::Connect(const std::string &url, const std::string &token, TRACE_EVENT0("livekit", "Room::Connect"); { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); if (connection_state_ != ConnectionState::Disconnected) { throw std::runtime_error("already connected"); } @@ -154,7 +154,7 @@ bool Room::Connect(const std::string &url, const std::string &token, new_remote_participants; { const auto &participants = connectCb.result().participants(); - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); for (const auto &pt : participants) { const auto &owned = pt.participant(); auto rp = createRemoteParticipant(owned); @@ -180,7 +180,7 @@ bool Room::Connect(const std::string &url, const std::string &token, // Publish all state atomically under lock { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); room_handle_ = std::move(new_room_handle); room_info_ = std::move(new_room_info); local_participant_ = std::move(new_local_participant); @@ -193,7 +193,7 @@ bool Room::Connect(const std::string &url, const std::string &token, auto listenerId = FfiClient::instance().AddListener( [this](const proto::FfiEvent &e) { OnEvent(e); }); { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); listener_id_ = listenerId; } @@ -207,24 +207,24 @@ bool Room::Connect(const std::string &url, const std::string &token, } RoomInfoData Room::room_info() const { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); return room_info_; } LocalParticipant *Room::localParticipant() const { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); return local_participant_.get(); } RemoteParticipant *Room::remoteParticipant(const std::string &identity) const { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); auto it = remote_participants_.find(identity); return it == remote_participants_.end() ? nullptr : it->second.get(); } std::vector> Room::remoteParticipants() const { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); std::vector> out; out.reserve(remote_participants_.size()); for (const auto &kv : remote_participants_) { @@ -234,13 +234,13 @@ Room::remoteParticipants() const { } E2EEManager *Room::e2eeManager() const { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); return e2ee_manager_.get(); } void Room::registerTextStreamHandler(const std::string &topic, TextStreamHandler handler) { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); auto [it, inserted] = text_stream_handlers_.emplace(topic, std::move(handler)); if (!inserted) { @@ -250,13 +250,13 @@ void Room::registerTextStreamHandler(const std::string &topic, } void Room::unregisterTextStreamHandler(const std::string &topic) { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); text_stream_handlers_.erase(topic); } void Room::registerByteStreamHandler(const std::string &topic, ByteStreamHandler handler) { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); auto [it, inserted] = byte_stream_handlers_.emplace(topic, std::move(handler)); if (!inserted) { @@ -266,7 +266,7 @@ void Room::registerByteStreamHandler(const std::string &topic, } void Room::unregisterByteStreamHandler(const std::string &topic) { - std::lock_guard g(lock_); + const std::scoped_lock g(lock_); byte_stream_handlers_.erase(topic); } @@ -378,7 +378,7 @@ void Room::OnEvent(const FfiEvent &event) { // lock. RoomDelegate *delegate_snapshot = nullptr; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); delegate_snapshot = delegate_; } @@ -388,7 +388,7 @@ void Room::OnEvent(const FfiEvent &event) { LocalParticipant *lp = nullptr; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); if (!local_participant_) { return; } @@ -417,7 +417,7 @@ void Room::OnEvent(const FfiEvent &event) { // Check if this event is for our room handle { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); if (!room_handle_ || re.room_handle() != static_cast(room_handle_->get())) { return; @@ -428,7 +428,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kParticipantConnected: { std::shared_ptr new_participant; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &owned = re.participant_connected().info(); // createRemoteParticipant takes proto::OwnedParticipant new_participant = createRemoteParticipant(owned); @@ -446,7 +446,7 @@ void Room::OnEvent(const FfiEvent &event) { std::shared_ptr removed; DisconnectReason reason = DisconnectReason::Unknown; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &pd = re.participant_disconnected(); const std::string &identity = pd.participant_identity(); reason = toDisconnectReason(pd.disconnect_reason()); @@ -476,7 +476,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kLocalTrackPublished: { LocalTrackPublishedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); if (!local_participant_) { LK_LOG_ERROR("kLocalTrackPublished: local_participant_ is nullptr"); break; @@ -500,7 +500,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kLocalTrackUnpublished: { LocalTrackUnpublishedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); if (!local_participant_) { LK_LOG_ERROR("kLocalTrackUnpublished: local_participant_ is nullptr"); break; @@ -524,7 +524,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kLocalTrackSubscribed: { LocalTrackSubscribedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); if (!local_participant_) { break; } @@ -548,7 +548,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kTrackPublished: { TrackPublishedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &tp = re.track_published(); const std::string &identity = tp.participant_identity(); auto it = remote_participants_.find(identity); @@ -577,7 +577,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kTrackUnpublished: { TrackUnpublishedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &tu = re.track_unpublished(); const std::string &identity = tu.participant_identity(); const std::string &pub_sid = tu.publication_sid(); @@ -615,7 +615,7 @@ void Room::OnEvent(const FfiEvent &event) { RemoteParticipant *rparticipant = nullptr; std::shared_ptr remote_track; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); // Find participant auto pit = remote_participants_.find(identity); if (pit == remote_participants_.end()) { @@ -670,7 +670,7 @@ void Room::OnEvent(const FfiEvent &event) { TrackSource unsub_source = TrackSource::SOURCE_UNKNOWN; std::string unsub_identity; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &tu = re.track_unsubscribed(); unsub_identity = tu.participant_identity(); const std::string &track_sid = tu.track_sid(); @@ -714,7 +714,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kTrackSubscriptionFailed: { TrackSubscriptionFailedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &tsf = re.track_subscription_failed(); const std::string &identity = tsf.participant_identity(); auto pit = remote_participants_.find(identity); @@ -766,7 +766,7 @@ void Room::OnEvent(const FfiEvent &event) { TrackMutedEvent ev; bool success = false; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &tm = re.track_muted(); const std::string &identity = tm.participant_identity(); const std::string &sid = tm.track_sid(); @@ -805,7 +805,7 @@ void Room::OnEvent(const FfiEvent &event) { TrackUnmutedEvent ev; bool success = false; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &tu = re.track_unmuted(); const std::string &identity = tu.participant_identity(); const std::string &sid = tu.track_sid(); @@ -848,9 +848,11 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kActiveSpeakersChanged: { ActiveSpeakersChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &asc = re.active_speakers_changed(); for (const auto &identity : asc.participant_identities()) { + // Appears to be clang-tidy false positive + // NOLINTNEXTLINE(misc-const-correctness) Participant *participant = nullptr; if (local_participant_ && local_participant_->identity() == identity) { @@ -874,7 +876,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kRoomMetadataChanged: { RoomMetadataChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto old_metadata = room_info_.metadata; room_info_.metadata = re.room_metadata_changed().metadata(); ev.old_metadata = old_metadata; @@ -888,7 +890,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kRoomSidChanged: { RoomSidChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); room_info_.sid = re.room_sid_changed().sid(); ev.sid = room_info_.sid.value_or(std::string{}); } @@ -900,7 +902,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kParticipantMetadataChanged: { ParticipantMetadataChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &pm = re.participant_metadata_changed(); const std::string &identity = pm.participant_identity(); Participant *participant = nullptr; @@ -918,7 +920,7 @@ void Room::OnEvent(const FfiEvent &event) { identity); break; } - std::string old_metadata = participant->metadata(); + const std::string old_metadata = participant->metadata(); participant->set_metadata(pm.metadata()); ev.participant = participant; ev.old_metadata = old_metadata; @@ -933,7 +935,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kParticipantNameChanged: { ParticipantNameChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &pn = re.participant_name_changed(); const std::string &identity = pn.participant_identity(); Participant *participant = nullptr; @@ -950,7 +952,7 @@ void Room::OnEvent(const FfiEvent &event) { identity); break; } - std::string old_name = participant->name(); + const std::string old_name = participant->name(); participant->set_name(pn.name()); ev.participant = participant; ev.old_name = old_name; @@ -964,7 +966,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kParticipantAttributesChanged: { ParticipantAttributesChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &pa = re.participant_attributes_changed(); const std::string &identity = pa.participant_identity(); Participant *participant = nullptr; @@ -1003,7 +1005,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kParticipantEncryptionStatusChanged: { ParticipantEncryptionStatusChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &pe = re.participant_encryption_status_changed(); const std::string &identity = pe.participant_identity(); Participant *participant = nullptr; @@ -1033,7 +1035,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kConnectionQualityChanged: { ConnectionQualityChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &cq = re.connection_quality_changed(); const std::string &identity = cq.participant_identity(); Participant *participant = nullptr; @@ -1067,7 +1069,7 @@ void Room::OnEvent(const FfiEvent &event) { const auto &dp = re.data_packet_received(); RemoteParticipant *rp = nullptr; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); auto it = remote_participants_.find(dp.participant_identity()); if (it != remote_participants_.end()) { rp = it->second.get(); @@ -1092,7 +1094,7 @@ void Room::OnEvent(const FfiEvent &event) { E2eeStateChangedEvent ev; { LK_LOG_DEBUG("e2ee_state_changed for participant"); - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &es = re.e2ee_state_changed(); const std::string &identity = es.participant_identity(); Participant *participant = nullptr; @@ -1126,7 +1128,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kConnectionStateChanged: { ConnectionStateChangedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &cs = re.connection_state_changed(); // TODO, maybe we should update our |connection_state_| // correspoindingly, but the this kConnectionStateChanged event is never @@ -1183,7 +1185,7 @@ void Room::OnEvent(const FfiEvent &event) { old_byte_readers; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); listener_to_remove = listener_id_; listener_id_ = 0; @@ -1228,7 +1230,7 @@ void Room::OnEvent(const FfiEvent &event) { std::shared_ptr text_reader; std::shared_ptr byte_reader; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); // Determine stream type from oneof in protobuf // Adjust these names if your generated C++ uses different ones @@ -1275,7 +1277,7 @@ void Room::OnEvent(const FfiEvent &event) { std::shared_ptr text_reader; std::shared_ptr byte_reader; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); auto itT = text_stream_readers_.find(chunk.stream_id()); if (itT != text_stream_readers_.end()) { text_reader = itT->second; @@ -1292,7 +1294,7 @@ void Room::OnEvent(const FfiEvent &event) { } else if (byte_reader) { // Convert string bytes -> vector const std::string &s = chunk.content(); - std::vector bytes(s.begin(), s.end()); + const std::vector bytes(s.begin(), s.end()); byte_reader->onChunkUpdate(bytes); } break; @@ -1307,7 +1309,7 @@ void Room::OnEvent(const FfiEvent &event) { trailer_attrs.emplace(kv.first, kv.second); } { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); auto itT = text_stream_readers_.find(trailer.stream_id()); if (itT != text_stream_readers_.end()) { text_reader = itT->second; @@ -1366,7 +1368,7 @@ void Room::OnEvent(const FfiEvent &event) { case proto::RoomEvent::kParticipantsUpdated: { ParticipantsUpdatedEvent ev; { - std::lock_guard guard(lock_); + const std::scoped_lock guard(lock_); const auto &pu = re.participants_updated(); for (const auto &info : pu.participants()) { const std::string &identity = info.identity(); diff --git a/src/subscription_thread_dispatcher.cpp b/src/subscription_thread_dispatcher.cpp index 0c5fe32e..91877877 100644 --- a/src/subscription_thread_dispatcher.cpp +++ b/src/subscription_thread_dispatcher.cpp @@ -43,19 +43,22 @@ const char *trackKindName(TrackKind kind) { } // namespace -SubscriptionThreadDispatcher::SubscriptionThreadDispatcher() - : next_data_callback_id_(1) {} +SubscriptionThreadDispatcher::SubscriptionThreadDispatcher() = default; +// NOLINTBEGIN(bugprone-exception-escape) +// Exceptions can be thrown by stopAll() in this desctuctor, and clang flags as +// an exception escape suppressing for now SubscriptionThreadDispatcher::~SubscriptionThreadDispatcher() { LK_LOG_DEBUG("Destroying SubscriptionThreadDispatcher"); stopAll(); } +// NOLINTEND(bugprone-exception-escape) void SubscriptionThreadDispatcher::setOnAudioFrameCallback( const std::string &participant_identity, TrackSource source, AudioFrameCallback callback, const AudioStream::Options &opts) { const CallbackKey key{participant_identity, source, ""}; - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); const bool replacing = audio_callbacks_.find(key) != audio_callbacks_.end(); audio_callbacks_[key] = RegisteredAudioCallback{std::move(callback), opts}; LK_LOG_DEBUG("Registered audio frame callback for participant={} source={} " @@ -69,7 +72,7 @@ void SubscriptionThreadDispatcher::setOnAudioFrameCallback( AudioFrameCallback callback, const AudioStream::Options &opts) { const CallbackKey key{participant_identity, TrackSource::SOURCE_UNKNOWN, track_name}; - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); const bool replacing = audio_callbacks_.find(key) != audio_callbacks_.end(); audio_callbacks_[key] = RegisteredAudioCallback{std::move(callback), opts}; LK_LOG_DEBUG( @@ -82,7 +85,7 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback( const std::string &participant_identity, TrackSource source, VideoFrameCallback callback, const VideoStream::Options &opts) { const CallbackKey key{participant_identity, source, ""}; - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); const bool replacing = video_callbacks_.find(key) != video_callbacks_.end(); video_callbacks_[key] = RegisteredVideoCallback{ std::move(callback), @@ -100,7 +103,7 @@ void SubscriptionThreadDispatcher::setOnVideoFrameEventCallback( VideoFrameEventCallback callback, const VideoStream::Options &opts) { const CallbackKey key{participant_identity, TrackSource::SOURCE_UNKNOWN, track_name}; - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); const bool replacing = video_callbacks_.find(key) != video_callbacks_.end(); video_callbacks_[key] = RegisteredVideoCallback{ VideoFrameCallback{}, @@ -118,7 +121,7 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback( VideoFrameCallback callback, const VideoStream::Options &opts) { const CallbackKey key{participant_identity, TrackSource::SOURCE_UNKNOWN, track_name}; - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); const bool replacing = video_callbacks_.find(key) != video_callbacks_.end(); video_callbacks_[key] = RegisteredVideoCallback{ std::move(callback), @@ -137,7 +140,7 @@ void SubscriptionThreadDispatcher::clearOnAudioFrameCallback( std::thread old_thread; bool removed_callback = false; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); removed_callback = audio_callbacks_.erase(key) > 0; old_thread = extractReaderThreadLocked(key); LK_LOG_DEBUG( @@ -158,7 +161,7 @@ void SubscriptionThreadDispatcher::clearOnAudioFrameCallback( std::thread old_thread; bool removed_callback = false; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); removed_callback = audio_callbacks_.erase(key) > 0; old_thread = extractReaderThreadLocked(key); LK_LOG_DEBUG( @@ -178,7 +181,7 @@ void SubscriptionThreadDispatcher::clearOnVideoFrameCallback( std::thread old_thread; bool removed_callback = false; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); removed_callback = video_callbacks_.erase(key) > 0; old_thread = extractReaderThreadLocked(key); LK_LOG_DEBUG( @@ -199,7 +202,7 @@ void SubscriptionThreadDispatcher::clearOnVideoFrameCallback( std::thread old_thread; bool removed_callback = false; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); removed_callback = video_callbacks_.erase(key) > 0; old_thread = extractReaderThreadLocked(key); LK_LOG_DEBUG( @@ -233,7 +236,7 @@ void SubscriptionThreadDispatcher::handleTrackSubscribed( const CallbackKey fallback_key{participant_identity, source, ""}; std::thread old_thread; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); if ((track->kind() == TrackKind::KIND_AUDIO && audio_callbacks_.find(key) == audio_callbacks_.end()) || (track->kind() == TrackKind::KIND_VIDEO && @@ -256,7 +259,7 @@ void SubscriptionThreadDispatcher::handleTrackUnsubscribed( std::thread old_thread; std::thread fallback_old_thread; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); old_thread = extractReaderThreadLocked(key); fallback_old_thread = extractReaderThreadLocked(fallback_key); LK_LOG_DEBUG("Handling unsubscribed track for participant={} source={} " @@ -282,7 +285,7 @@ DataFrameCallbackId SubscriptionThreadDispatcher::addOnDataFrameCallback( std::thread old_thread; DataFrameCallbackId id; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); id = next_data_callback_id_++; const DataCallbackKey key{participant_identity, track_name}; data_callbacks_[id] = RegisteredDataCallback{key, std::move(callback)}; @@ -303,7 +306,7 @@ void SubscriptionThreadDispatcher::removeOnDataFrameCallback( DataFrameCallbackId id) { std::thread old_thread; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); data_callbacks_.erase(id); old_thread = extractDataReaderThreadLocked(id); } @@ -325,7 +328,7 @@ void SubscriptionThreadDispatcher::handleDataTrackPublished( std::vector old_threads; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); const DataCallbackKey key{track->publisherIdentity(), track->info().name}; remote_data_tracks_[key] = track; @@ -349,13 +352,13 @@ void SubscriptionThreadDispatcher::handleDataTrackUnpublished( std::vector old_threads; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); for (auto it = active_data_readers_.begin(); it != active_data_readers_.end();) { auto &reader = it->second; if (reader->remote_track && reader->remote_track->info().sid == sid) { { - const std::lock_guard sub_guard(reader->sub_mutex); + const std::scoped_lock sub_guard(reader->sub_mutex); if (reader->stream) { reader->stream->close(); } @@ -384,7 +387,7 @@ void SubscriptionThreadDispatcher::handleDataTrackUnpublished( void SubscriptionThreadDispatcher::stopAll() { std::vector threads; { - const std::lock_guard lock(lock_); + const std::scoped_lock lock(lock_); LK_LOG_DEBUG("Stopping all subscription readers active_readers={} " "active_data_readers={} audio_callbacks={} " "video_callbacks={} data_callbacks={}", @@ -409,7 +412,7 @@ void SubscriptionThreadDispatcher::stopAll() { for (auto &[id, reader] : active_data_readers_) { { - const std::lock_guard sub_guard(reader->sub_mutex); + const std::scoped_lock sub_guard(reader->sub_mutex); if (reader->stream) { reader->stream->close(); } @@ -520,22 +523,34 @@ std::thread SubscriptionThreadDispatcher::startAudioReaderLocked( reader.audio_stream = stream; const std::string participant_identity = key.participant_identity; const TrackSource source = key.source; - // NOLINTBEGIN(bugprone-lambda-function-name) + // NOLINTBEGIN(bugprone-lambda-function-name,bugprone-exception-escape) + // Outer try/catch contains anything escaping the per-frame try/catch + // (stream->read, LK_LOG formatting, etc.) so an exception in this reader + // thread cannot std::terminate the process. clang-tidy still flags a + // residual escape path through spdlog's own formatter; that's a logger + // fault, not application logic -- suppressed at the lambda level. reader.thread = std::thread([stream, cb, participant_identity, source]() { - LK_LOG_DEBUG("Audio reader thread started for participant={} source={}", - participant_identity, static_cast(source)); - AudioFrameEvent ev; - while (stream->read(ev)) { - try { - cb(ev.frame); - } catch (const std::exception &e) { - LK_LOG_ERROR("Audio frame callback exception: {}", e.what()); + try { + LK_LOG_DEBUG("Audio reader thread started for participant={} source={}", + participant_identity, static_cast(source)); + AudioFrameEvent ev; + while (stream->read(ev)) { + try { + cb(ev.frame); + } catch (const std::exception &e) { + LK_LOG_ERROR("Audio frame callback exception: {}", e.what()); + } } + LK_LOG_DEBUG("Audio reader thread exiting for participant={} source={}", + participant_identity, static_cast(source)); + } catch (const std::exception &e) { + LK_LOG_ERROR("Audio reader thread terminating due to exception: {}", + e.what()); + } catch (...) { + LK_LOG_ERROR("Audio reader thread terminating due to unknown exception"); } - LK_LOG_DEBUG("Audio reader thread exiting for participant={} source={}", - participant_identity, static_cast(source)); }); - // NOLINTEND(bugprone-lambda-function-name) + // NOLINTEND(bugprone-lambda-function-name,bugprone-exception-escape) active_readers_[key] = std::move(reader); LK_LOG_DEBUG("Started audio reader for participant={} source={} " "active_readers={}", @@ -573,27 +588,37 @@ std::thread SubscriptionThreadDispatcher::startVideoReaderLocked( auto event_cb = callback.event_callback; const std::string participant_identity = key.participant_identity; const TrackSource source = key.source; - // NOLINTBEGIN(bugprone-lambda-function-name) + // NOLINTBEGIN(bugprone-lambda-function-name,bugprone-exception-escape) + // Mirrors the audio reader: outer try/catch contains escapes from + // stream->read, LK_LOG, etc. Residual diagnostic from spdlog's own + // formatter is an unrelated logger-fault path and is suppressed. reader.thread = std::thread([stream = std::move(stream), legacy_cb, event_cb, participant_identity, source]() { - LK_LOG_DEBUG("Video reader thread started for participant={} source={}", - participant_identity, static_cast(source)); - VideoFrameEvent ev; - while (stream->read(ev)) { - try { - if (event_cb) { - event_cb(ev); - } else if (legacy_cb) { - legacy_cb(ev.frame, ev.timestamp_us); + try { + LK_LOG_DEBUG("Video reader thread started for participant={} source={}", + participant_identity, static_cast(source)); + VideoFrameEvent ev; + while (stream->read(ev)) { + try { + if (event_cb) { + event_cb(ev); + } else if (legacy_cb) { + legacy_cb(ev.frame, ev.timestamp_us); + } + } catch (const std::exception &e) { + LK_LOG_ERROR("Video frame callback exception: {}", e.what()); } - } catch (const std::exception &e) { - LK_LOG_ERROR("Video frame callback exception: {}", e.what()); } + LK_LOG_DEBUG("Video reader thread exiting for participant={} source={}", + participant_identity, static_cast(source)); + } catch (const std::exception &e) { + LK_LOG_ERROR("Video reader thread terminating due to exception: {}", + e.what()); + } catch (...) { + LK_LOG_ERROR("Video reader thread terminating due to unknown exception"); } - LK_LOG_DEBUG("Video reader thread exiting for participant={} source={}", - participant_identity, static_cast(source)); }); - // NOLINTEND(bugprone-lambda-function-name) + // NOLINTEND(bugprone-lambda-function-name,bugprone-exception-escape) active_readers_[key] = std::move(reader); LK_LOG_DEBUG("Started video reader for participant={} source={} " "active_readers={}", @@ -615,7 +640,7 @@ std::thread SubscriptionThreadDispatcher::extractDataReaderThreadLocked( auto reader = std::move(it->second); active_data_readers_.erase(it); { - const std::lock_guard guard(reader->sub_mutex); + const std::scoped_lock guard(reader->sub_mutex); if (reader->stream) { reader->stream->close(); } @@ -634,7 +659,7 @@ std::thread SubscriptionThreadDispatcher::extractDataReaderThreadLocked( auto reader = std::move(it->second); active_data_readers_.erase(it); { - const std::lock_guard guard(reader->sub_mutex); + const std::scoped_lock guard(reader->sub_mutex); if (reader->stream) { reader->stream->close(); } @@ -651,8 +676,8 @@ std::thread SubscriptionThreadDispatcher::startDataReaderLocked( const DataFrameCallback &cb) { auto old_thread = extractDataReaderThreadLocked(id); - int total_active = static_cast(active_readers_.size()) + - static_cast(active_data_readers_.size()); + const int total_active = static_cast(active_readers_.size()) + + static_cast(active_data_readers_.size()); if (total_active >= kMaxActiveReaders) { LK_LOG_ERROR("Cannot start data reader for {} track={}: active reader " "limit ({}) reached", @@ -687,7 +712,7 @@ std::thread SubscriptionThreadDispatcher::startDataReaderLocked( identity, track_name); { - const std::lock_guard guard(reader->sub_mutex); + const std::scoped_lock guard(reader->sub_mutex); reader->stream = stream; } diff --git a/src/tests/unit/test_subscription_thread_dispatcher.cpp b/src/tests/unit/test_subscription_thread_dispatcher.cpp index bb3bdfd1..a3864c64 100644 --- a/src/tests/unit/test_subscription_thread_dispatcher.cpp +++ b/src/tests/unit/test_subscription_thread_dispatcher.cpp @@ -562,8 +562,15 @@ TEST_F(SubscriptionThreadDispatcherTest, "alice", "my-track", [](const std::vector &, std::optional) {}); - EXPECT_NE(id, 0u); + EXPECT_EQ(id, 0u); EXPECT_EQ(dataCallbacks(dispatcher).size(), 1u); + + // Add a second one to confirm size and IDs are correct + auto id2 = dispatcher.addOnDataFrameCallback( + "alice", "my-track", + [](const std::vector &, std::optional) {}); + EXPECT_EQ(id2, 1u); + EXPECT_EQ(dataCallbacks(dispatcher).size(), 2u); } TEST_F(SubscriptionThreadDispatcherTest, diff --git a/src/tests/unit/test_track_publication.cpp b/src/tests/unit/test_track_publication.cpp new file mode 100644 index 00000000..a0e74bf3 --- /dev/null +++ b/src/tests/unit/test_track_publication.cpp @@ -0,0 +1,98 @@ +/* + * Copyright 2026 LiveKit + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include + +#include "livekit/local_track_publication.h" +#include "livekit/remote_track_publication.h" +#include "livekit/track.h" +#include "livekit/track_publication.h" +#include "track.pb.h" + +namespace livekit::test { +namespace { + +// Concrete `Track` for testing. The base ctor is `protected`, so a thin +// subclass is needed to construct one. The handle is intentionally zero so +// no FFI drop is invoked on destruction. +class FakeTrack : public Track { +public: + FakeTrack() + : Track(FfiHandle(), "fake-sid", "fake-name", TrackKind::KIND_AUDIO, + StreamState::STATE_ACTIVE, + /*muted=*/false, /*remote=*/false) {} +}; + +// Default-construct the proto: the publication ctor only reads via getters, +// which return well-defined defaults for unset fields. Using a zero handle +// keeps the `FfiHandle` a no-op on destruction. +proto::OwnedTrackPublication makeOwnedPub() { + return proto::OwnedTrackPublication{}; +} + +template +class TrackPublicationTest : public ::testing::Test {}; + +using PublicationTypes = + ::testing::Types; +TYPED_TEST_SUITE(TrackPublicationTest, PublicationTypes); + +} // namespace + +TYPED_TEST(TrackPublicationTest, TrackIsNullByDefault) { + TypeParam pub(makeOwnedPub()); + EXPECT_EQ(pub.track(), nullptr); +} + +TYPED_TEST(TrackPublicationTest, SetTrackRoundTrips) { + TypeParam pub(makeOwnedPub()); + + auto track = std::make_shared(); + pub.setTrack(track); + + EXPECT_EQ(pub.track().get(), track.get()); + // Publication holds a strong reference alongside the local one. + EXPECT_GE(track.use_count(), 2); +} + +TYPED_TEST(TrackPublicationTest, SetTrackNullClears) { + TypeParam pub(makeOwnedPub()); + + pub.setTrack(std::make_shared()); + ASSERT_NE(pub.track(), nullptr); + + pub.setTrack(nullptr); + EXPECT_EQ(pub.track(), nullptr); +} + +TYPED_TEST(TrackPublicationTest, ReplacingTrackReleasesPrevious) { + TypeParam pub(makeOwnedPub()); + + auto first = std::make_shared(); + pub.setTrack(first); + ASSERT_EQ(pub.track().get(), first.get()); + + auto second = std::make_shared(); + pub.setTrack(second); + + EXPECT_EQ(pub.track().get(), second.get()); + // The publication no longer references `first`; only the local handle does. + EXPECT_EQ(first.use_count(), 1); +} + +} // namespace livekit::test diff --git a/src/trace/event_tracer.cpp b/src/trace/event_tracer.cpp index cfcc267c..ec57de33 100644 --- a/src/trace/event_tracer.cpp +++ b/src/trace/event_tracer.cpp @@ -92,7 +92,7 @@ struct TraceEventData { // Escape a string for JSON std::string JsonEscape(const std::string &s) { std::ostringstream oss; - for (char c : s) { + for (const char c : s) { switch (c) { case '"': oss << "\\\""; @@ -211,6 +211,8 @@ std::unordered_set g_enabled_categories; std::atomic g_tracing_enabled{false}; std::atomic g_shutdown_requested{false}; std::thread g_writer_thread; +// This is internal test code, not worth changing to avoid throw risk +// NOLINTNEXTLINE(bugprone-throwing-static-initialization) std::ofstream g_trace_file; uint64_t g_start_time = 0; bool g_first_event = true; @@ -257,7 +259,7 @@ void WriterThreadFunc() { // Exit if shutdown requested and queue is empty if (g_shutdown_requested.load()) { - std::lock_guard lock(g_mutex); + const std::scoped_lock lock(g_mutex); if (g_event_queue.empty()) { break; } @@ -269,7 +271,7 @@ void WriterThreadFunc() { void SetupEventTracer(GetCategoryEnabledPtr get_category_enabled_ptr, AddTraceEventPtr add_trace_event_ptr) { - std::lock_guard lock(g_mutex); + const std::scoped_lock lock(g_mutex); g_custom_get_category_enabled = get_category_enabled_ptr; g_custom_add_trace_event = add_trace_event_ptr; } @@ -286,7 +288,7 @@ const unsigned char *EventTracer::GetCategoryEnabled(const char *name) { } // Check if category is enabled - std::lock_guard lock(g_mutex); + const std::scoped_lock lock(g_mutex); // Empty enabled set means all categories are enabled if (g_enabled_categories.empty()) { @@ -294,7 +296,7 @@ const unsigned char *EventTracer::GetCategoryEnabled(const char *name) { } // Check if this specific category is enabled - std::string category_name(name); + const std::string category_name(name); if (g_enabled_categories.count(category_name) > 0) { return &g_enabled_byte; } @@ -302,7 +304,7 @@ const unsigned char *EventTracer::GetCategoryEnabled(const char *name) { // Check for wildcard matches (e.g., "livekit.*" matches "livekit.connect") for (const auto &pattern : g_enabled_categories) { if (pattern.back() == '*') { - std::string prefix = pattern.substr(0, pattern.size() - 1); + const std::string prefix = pattern.substr(0, pattern.size() - 1); if (category_name.compare(0, prefix.size(), prefix) == 0) { return &g_enabled_byte; } @@ -366,7 +368,7 @@ void EventTracer::AddTraceEvent(char phase, // Queue the event for the writer thread { - std::lock_guard lock(g_mutex); + const std::scoped_lock lock(g_mutex); g_event_queue.push(std::move(event)); } g_cv.notify_one(); @@ -376,7 +378,7 @@ namespace internal { bool StartTracing(const std::string &file_path, const std::vector &categories) { - std::lock_guard lock(g_mutex); + const std::scoped_lock lock(g_mutex); // Don't start if already running if (g_tracing_enabled.load()) { @@ -426,7 +428,7 @@ void StopTracing() { } // Close the file with JSON footer - std::lock_guard lock(g_mutex); + const std::scoped_lock lock(g_mutex); if (g_trace_file.is_open()) { g_trace_file << R"(],"displayTimeUnit":"ms"})"; g_trace_file.close(); diff --git a/src/trace/event_tracer.h b/src/trace/event_tracer.h index 0b7aae28..181abf22 100644 --- a/src/trace/event_tracer.h +++ b/src/trace/event_tracer.h @@ -18,6 +18,9 @@ * Use of this source code is governed by a BSD-style license. */ +// NOLINTBEGIN +// External code: this header is derived from WebRTC's event_tracer.h + // This file defines the interface for event tracing in LiveKit SDK. // // Event log handlers are set through SetupEventTracer(). User of this API will @@ -89,4 +92,6 @@ namespace webrtc { using namespace livekit::trace; } // namespace webrtc +// NOLINTEND + #endif // LIVEKIT_TRACE_EVENT_TRACER_H_ diff --git a/src/trace/event_tracer_internal.h b/src/trace/event_tracer_internal.h index 01884b85..e01d76b3 100644 --- a/src/trace/event_tracer_internal.h +++ b/src/trace/event_tracer_internal.h @@ -21,9 +21,7 @@ #include #include -namespace livekit { -namespace trace { -namespace internal { +namespace livekit::trace::internal { /** * Start tracing and write events to the specified file. @@ -54,8 +52,6 @@ void StopTracing(); */ bool IsTracingEnabled(); -} // namespace internal -} // namespace trace -} // namespace livekit +} // namespace livekit::trace::internal #endif // LIVEKIT_TRACE_EVENT_TRACER_INTERNAL_H_ diff --git a/src/trace/trace_event.h b/src/trace/trace_event.h index 9efda258..6944ba16 100644 --- a/src/trace/trace_event.h +++ b/src/trace/trace_event.h @@ -18,6 +18,9 @@ * Use of this source code is governed by a BSD-style license. */ +// NOLINTBEGIN +// External code: this header is derived from Chromium's trace_event.h + #ifndef LIVEKIT_TRACE_TRACE_EVENT_H_ #define LIVEKIT_TRACE_TRACE_EVENT_H_ @@ -859,4 +862,7 @@ class TraceEndOnScopeClose { }; } // namespace trace_event_internal } // namespace webrtc + +// NOLINTEND + #endif // LIVEKIT_TRACE_TRACE_EVENT_H_ diff --git a/src/video_stream.cpp b/src/video_stream.cpp index ecfb3796..a3bce022 100644 --- a/src/video_stream.cpp +++ b/src/video_stream.cpp @@ -50,7 +50,7 @@ VideoStream::fromParticipant(Participant &participant, TrackSource track_source, VideoStream::~VideoStream() { close(); } VideoStream::VideoStream(VideoStream &&other) noexcept { - std::lock_guard lock(other.mutex_); + const std::scoped_lock lock(other.mutex_); queue_ = std::move(other.queue_); capacity_ = other.capacity_; eof_ = other.eof_; @@ -69,8 +69,8 @@ VideoStream &VideoStream::operator=(VideoStream &&other) noexcept { close(); { - std::lock_guard lock_this(mutex_); - std::lock_guard lock_other(other.mutex_); + const std::scoped_lock lock_this(mutex_); + const std::scoped_lock lock_other(other.mutex_); queue_ = std::move(other.queue_); capacity_ = other.capacity_; @@ -104,7 +104,7 @@ bool VideoStream::read(VideoFrameEvent &out) { void VideoStream::close() { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_) { return; } @@ -213,7 +213,7 @@ void VideoStream::onFfiEvent(const proto::FfiEvent &event) { void VideoStream::pushFrame(VideoFrameEvent &&ev) { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (closed_ || eof_) { return; @@ -231,7 +231,7 @@ void VideoStream::pushFrame(VideoFrameEvent &&ev) { void VideoStream::pushEos() { { - std::lock_guard lock(mutex_); + const std::scoped_lock lock(mutex_); if (eof_) { return; }