From 66102569a58caa6185eb257005863a791b626a73 Mon Sep 17 00:00:00 2001
From: Johannes Kron <kron@chromium.org>
Date: Tue, 02 Jun 2026 07:50:55 -0700
Subject: [PATCH] Improve thread safety in StatsCollectingDecoder and Encoder

This change adds locking to protect shared state within the
StatsCollectingDecoder and StatsCollectingEncoder classes to address
potential data races during concurrent callbacks.

While these classes typically operate on a single sequence, concurrent
calls to Decoded() or OnEncodedImage() can occur in specific WebRTC
scenarios. Hardware decoders and encoders may fire callbacks on
sequences different from the primary thread. Furthermore, in
multi-encoder simulcast or during transitions between hardware and
software implementations, delayed callbacks can arrive simultaneously
from different threads.

To ensure thread safety, internal state—including the StatsCollector
instance, frame counters, and timing metadata—is now guarded by a mutex.
The Release() method is also updated to use this lock when performing
final statistics calculations.

Fixed: 501679156
Change-Id: I3afb8ffc9c6401a7cb6219d4ec0f8b75e94b5275
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7762202
Reviewed-by: Harald Alvestrand <hta@chromium.org>
Commit-Queue: Johannes Kron <kron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1640163}
---

diff --git a/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.cc b/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.cc
index eccb48b..442ca9a7 100644
--- a/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.cc
+++ b/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.cc
@@ -62,12 +62,12 @@
                                        bool missing_frames,
                                        int64_t render_time_ms) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(decoding_sequence_checker_);
-  if (!first_frame_decoded_) {
-    first_frame_decoded_ = true;
-    ++(*GetDecoderCounter());
-  }
   {
     base::AutoLock auto_lock(lock_);
+    if (!first_frame_decoded_) {
+      first_frame_decoded_ = true;
+      ++(*GetDecoderCounter());
+    }
     number_of_new_keyframes_ += input_image.IsKey();
   }
   return decoder_->Decode(input_image, missing_frames, render_time_ms);
@@ -86,21 +86,27 @@
   DVLOG(3) << __func__;
   int32_t ret = decoder_->Release();
 
-  // There will be no new calls to Decoded() after the call to
-  // decoder_->Release(). Any outstanding calls to Decoded() will also finish
-  // before decoder_->Release() returns. It's therefore safe to access member
-  // variables here.
-  if (stats_collector_.is_active() &&
-      stats_collector_.samples_collected() >=
-          StatsCollector::kMinSamplesThreshold) {
-    ReportStats(stats_collector_.ComputeVideoStats());
-  }
+  std::optional<StatsCollector::Stats> stats_to_report;
+  {
+    base::AutoLock auto_lock(lock_);
+    // There shouldn't be any new calls to Decoded() after the call to
+    // decoder_->Release(). Any outstanding calls to Decoded() will typically
+    // finish before decoder_->Release() returns. Use lock to be safe since this
+    // is not guaranteed.
+    if (stats_collector_.is_active() &&
+        stats_collector_.samples_collected() >=
+            StatsCollector::kMinSamplesThreshold) {
+      stats_to_report = stats_collector_.ComputeVideoStats();
+    }
 
-  if (first_frame_decoded_) {
-    --(*GetDecoderCounter());
-    first_frame_decoded_ = false;
+    if (first_frame_decoded_) {
+      --(*GetDecoderCounter());
+      first_frame_decoded_ = false;
+    }
   }
-
+  if (stats_to_report) {
+    ReportStats(*stats_to_report);
+  }
   return ret;
 }
 
@@ -118,58 +124,56 @@
 void StatsCollectingDecoder::Decoded(webrtc::VideoFrame& decodedImage,
                                      std::optional<int32_t> decode_time_ms,
                                      std::optional<uint8_t> qp) {
-  // Decoded may be called on either the decoding sequence (SW decoding) or
-  // media sequence (HW decoding). However, these calls are not happening at the
-  // same time. If there's a fallback from SW decoding to HW decoding, a call to
-  // HW decoder->Release() ensures that any potential callbacks on the media
-  // sequence are finished before the decoding continues on the decoding
-  // sequence.
+  // Decoded() may be called on either the decoding sequence (SW decoding) or
+  // media sequence (HW decoding). While these calls typically do not happen at
+  // the same time, hardware decoders can be unpredictable. If there is a
+  // fallback from HW decoding to SW decoding, a call to HW decoder->Release()
+  // is expected to ensure that any potential callbacks on the media sequence
+  // are finished. However, in rare cases, a delayed "straggler" callback could
+  // fire on the media sequence concurrently with the active decoding sequence.
   DCHECK(decoded_callback_);
   decoded_callback_->Decoded(decodedImage, decode_time_ms, qp);
-  if (stats_collector_.has_finished()) {
-    // Return early if we've already finished the stats collection.
-    return;
-  }
 
-  base::TimeTicks now = base::TimeTicks::Now();
-  // Verify that there's only a single decoder when data collection is taking
-  // place.
-  if ((now - last_check_for_simultaneous_decoders_) >
-      kCheckSimultaneousDecodersInterval) {
-    last_check_for_simultaneous_decoders_ = now;
-    DVLOG(3) << "Simultaneous decoders: " << *GetDecoderCounter();
-    if (stats_collector_.is_active()) {
-      if (*GetDecoderCounter() > kMaximumDecodersToCollectStats) {
-        // Too many decoders, cancel stats collection.
-        stats_collector_.Clear();
-      }
-    } else if (*GetDecoderCounter() <= kMaximumDecodersToCollectStats) {
-      // Start up stats collection since there's only a single decoder active.
-      stats_collector_.Start();
-    }
-  }
-
-  // Read out number of new processed keyframes since last Decoded() callback.
-  size_t number_of_new_keyframes = 0;
+  std::optional<StatsCollector::Stats> stats_to_report;
   {
     base::AutoLock auto_lock(lock_);
-    number_of_new_keyframes += number_of_new_keyframes_;
-    number_of_new_keyframes_ = 0;
-  }
-
-  if (stats_collector_.is_active() && decodedImage.processing_time()) {
-    int pixel_size = static_cast<int>(decodedImage.size());
-    bool is_hardware_accelerated =
-        decoder_->GetDecoderInfo().is_hardware_accelerated;
-    float processing_time_ms = decodedImage.processing_time()->Elapsed().ms();
-
-    std::optional<StatsCollector::Stats> stats_to_report =
-        stats_collector_.AddProcessingTimeAndGetStats(
-            pixel_size, is_hardware_accelerated, processing_time_ms,
-            number_of_new_keyframes, now);
-    if (stats_to_report) {
-      ReportStats(*stats_to_report);
+    if (stats_collector_.has_finished()) {
+      // Return early if stats collection is already finished.
+      return;
     }
+
+    base::TimeTicks now = base::TimeTicks::Now();
+    // Verify that there's only a single decoder when data collection is taking
+    // place.
+    if ((now - last_check_for_simultaneous_decoders_) >
+        kCheckSimultaneousDecodersInterval) {
+      last_check_for_simultaneous_decoders_ = now;
+      DVLOG(3) << "Simultaneous decoders: " << *GetDecoderCounter();
+      if (stats_collector_.is_active()) {
+        if (*GetDecoderCounter() > kMaximumDecodersToCollectStats) {
+          // Too many decoders, cancel stats collection.
+          stats_collector_.Clear();
+        }
+      } else if (*GetDecoderCounter() <= kMaximumDecodersToCollectStats) {
+        // Start up stats collection since there's only a single decoder active.
+        stats_collector_.Start();
+      }
+    }
+
+    if (stats_collector_.is_active() && decodedImage.processing_time()) {
+      int pixel_size = static_cast<int>(decodedImage.size());
+      bool is_hardware_accelerated =
+          decoder_->GetDecoderInfo().is_hardware_accelerated;
+      float processing_time_ms = decodedImage.processing_time()->Elapsed().ms();
+
+      stats_to_report = stats_collector_.AddProcessingTimeAndGetStats(
+          pixel_size, is_hardware_accelerated, processing_time_ms,
+          number_of_new_keyframes_, now);
+      number_of_new_keyframes_ = 0;
+    }
+  }
+  if (stats_to_report) {
+    ReportStats(*stats_to_report);
   }
 }
 
diff --git a/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.h b/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.h
index 4ef87a6b..ca7e9d0 100644
--- a/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.h
+++ b/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder.h
@@ -29,10 +29,10 @@

diff --git a/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder_test.cc b/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder_test.cc
index 7ac1a594..ddb995e 100644
--- a/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder_test.cc
+++ b/third_party/blink/renderer/platform/peerconnection/stats_collecting_decoder_test.cc
@@ -6,10 +6,15 @@
 #include <optional>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/functional/callback_helpers.h"
 #include "base/memory/raw_ptr.h"
 #include "base/notreached.h"
+#include "base/run_loop.h"
+#include "base/task/thread_pool.h"
+#include "base/test/bind.h"
 #include "base/test/task_environment.h"
+#include "base/thread_annotations.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/webrtc/api/make_ref_counted.h"
 #include "third_party/webrtc/api/video/i420_buffer.h"
@@ -88,6 +93,8 @@
 
   int32_t Release() override { return WEBRTC_VIDEO_CODEC_OK; }
 
+  webrtc::DecodedImageCallback* GetCallback() const { return callback_; }
+
   DecoderInfo GetDecoderInfo() const override {
     DecoderInfo info;
     info.is_hardware_accelerated = *is_hw_accelerated_;
@@ -113,17 +120,23 @@
     // Set the processing time. Start time is set to a fixed nonzero time since
     // we're only interested in the delta.
     webrtc::Timestamp start_time = webrtc::Timestamp::Seconds(1234);
+    bool use_min_decode_time = false;
+    {
+      base::AutoLock auto_lock(lock_);
+      use_min_decode_time = frame_counter_ % 100 < 90;
+      ++frame_counter_;
+    }
     webrtc::TimeDelta decode_time = webrtc::TimeDelta::Millis(
-        frame_counter_ % 100 < 90 ? min_decode_time_ms_ : p90_decode_time_ms_);
-    decodedImage.set_processing_time({start_time, start_time + decode_time});
+        use_min_decode_time ? min_decode_time_ms_ : p90_decode_time_ms_);
 
-    ++frame_counter_;
+    decodedImage.set_processing_time({start_time, start_time + decode_time});
   }
 
  private:
-  int frame_counter_{0};
-  float min_decode_time_ms_;
-  float p90_decode_time_ms_;
+  base::Lock lock_;
+  int frame_counter_ GUARDED_BY(lock_) = 0;
+  const float min_decode_time_ms_;
+  const float p90_decode_time_ms_;
 };
 
 class StatsCollectingDecoderTest : public ::testing::Test {
@@ -131,15 +144,19 @@
   StatsCollectingDecoderTest()
       : decoded_image_callback_(kMinDecodingTimeMs,
                                 kExpectedP99ProcessingTimeMs),
+        internal_decoder_(new MockDecoder(&is_hw_accelerated_)),
         stats_decoder_(kFormatVp9,
-                       std::make_unique<MockDecoder>(&is_hw_accelerated_),
+                       std::unique_ptr<MockDecoder>(internal_decoder_),
                        base::BindRepeating(
                            &StatsCollectingDecoderTest::StoreProcessingStatsCB,
                            base::Unretained(this))) {
     stats_decoder_.RegisterDecodeCompleteCallback(&decoded_image_callback_);
   }
 
-  void TearDown() override { stats_decoder_.Release(); }
+  void TearDown() override {
+    internal_decoder_ = nullptr;
+    stats_decoder_.Release();
+  }
 
   void StoreProcessingStatsCB(const StatsCollector::StatsKey& stats_key,
                               const StatsCollector::VideoStats& video_stats) {
@@ -191,6 +208,7 @@
 
   bool is_hw_accelerated_{false};
   MockDecodedImageCallback decoded_image_callback_;
+  raw_ptr<MockDecoder> internal_decoder_;
   StatsCollectingDecoder stats_decoder_;
 
   uint32_t frame_counter{0};
@@ -334,5 +352,39 @@
   }
 }
 
+TEST_F(StatsCollectingDecoderTest, ConcurrentDecoded) {
+  // This test is stochastic. It relies on a "thread storm" (10 threads rapidly
+  // firing 100 callbacks each) to create overlap rather than forcing a
+  // deterministic interleaving. We rely on TSAN to catch regressions; an
+  // occasional pass in a non-TSAN build does not guarantee the absence
+  // of data races.
+
+  // Decode one frame to initialize the decoder and set it as active.
+  CreateAndDecodeFrames(kHdWidth, kHdHeight, /*is_hw_accelerated=*/false, 1,
+                        kKeyframeInterval, kFramerate);
+
+  webrtc::DecodedImageCallback* callback = internal_decoder_->GetCallback();
+  ASSERT_TRUE(callback);
+
+  constexpr int kNumThreads = 10;
+  constexpr int kCallbacksPerThread = 100;
+
+  base::RunLoop run_loop;
+  auto barrier = base::BarrierClosure(kNumThreads, run_loop.QuitClosure());
+
+  for (int i = 0; i < kNumThreads; ++i) {
+    base::ThreadPool::PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&, i]() {
+          for (int j = 0; j < kCallbacksPerThread; ++j) {
+            webrtc::VideoFrame video_frame = CreateMockFrame(
+                kHdWidth, kHdHeight, 100 + i * kCallbacksPerThread + j);
+            callback->Decoded(video_frame, std::nullopt, std::nullopt);
+          }
+          barrier.Run();
+        }));
+  }
+  run_loop.Run();
+}
+
 }  // namespace
 }  // namespace blink
diff --git a/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder_test.cc b/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder_test.cc
index 301062e..7493d73d 100644
--- a/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder_test.cc
+++ b/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder_test.cc
@@ -6,10 +6,15 @@
 #include <optional>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/functional/callback_helpers.h"
 #include "base/memory/raw_ptr.h"
 #include "base/notreached.h"
+#include "base/run_loop.h"
+#include "base/task/thread_pool.h"
+#include "base/test/bind.h"
 #include "base/test/task_environment.h"
+#include "base/thread_annotations.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/webrtc/api/make_ref_counted.h"
@@ -106,6 +111,9 @@
     return WEBRTC_VIDEO_CODEC_OK;
   }
   int32_t Release() override { return WEBRTC_VIDEO_CODEC_OK; }
+
+  webrtc::EncodedImageCallback* GetCallback() const { return callback_; }
+
   EncoderInfo GetEncoderInfo() const override {
     EncoderInfo info;
     info.is_hardware_accelerated = *is_hw_accelerated_;
@@ -136,16 +144,21 @@
   Result OnEncodedImage(
       const webrtc::EncodedImage& encoded_image,
       const webrtc::CodecSpecificInfo* codec_specific_info) override {
+    base::AutoLock auto_lock(lock_);
     ++frame_counter_;
     return {Result::OK, encoded_image.RtpTimestamp()};
   }
   void OnFrameDropped(uint32_t rtp_timestamp,
                       int spatial_id,
                       bool is_end_of_temporal_unit) override {}
-  int get_frame_counter() const { return frame_counter_; }
+  int get_frame_counter() const {
+    base::AutoLock auto_lock(lock_);
+    return frame_counter_;
+  }
 
  private:
-  int frame_counter_ = 0;
+  mutable base::Lock lock_;
+  int frame_counter_ GUARDED_BY(lock_) = 0;
 };
 
 class StatsCollectingEncoderTest : public ::testing::Test {
@@ -466,5 +479,43 @@
   stats_encoder_.OnLossNotification(kLossNotification);
 }
 
+TEST_F(StatsCollectingEncoderTest, ConcurrentOnEncodedImage) {
+  // This test is stochastic. It relies on a "thread storm" (10 threads rapidly
+  // firing 100 callbacks each) to create overlap rather than forcing a
+  // deterministic interleaving. We rely on TSAN to catch regressions; an
+  // occasional pass in a non-TSAN build does not guarantee the absence
+  // of data races.
+
+  // Encode one frame to initialize the encoder and set it as active.
+  CreateAndEncodeFrames(kHdWidth, kHdHeight, /*spatial_layers=*/1,
+                        /*is_hw_accelerated=*/false, 1, kKeyframeInterval,
+                        kFramerate);
+
+  webrtc::EncodedImageCallback* callback = internal_encoder_->GetCallback();
+  ASSERT_TRUE(callback);
+
+  constexpr int kNumThreads = 10;
+  constexpr int kCallbacksPerThread = 100;
+
+  base::RunLoop run_loop;
+  auto barrier = base::BarrierClosure(kNumThreads, run_loop.QuitClosure());
+
+  for (int i = 0; i < kNumThreads; ++i) {
+    base::ThreadPool::PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&, i]() {
+          for (int j = 0; j < kCallbacksPerThread; ++j) {
+            webrtc::EncodedImage encoded_frame;
+            encoded_frame._encodedWidth = kHdWidth;
+            encoded_frame._encodedHeight = kHdHeight;
+            encoded_frame.SetSpatialIndex(0);
+            encoded_frame.SetRtpTimestamp(100 + i * kCallbacksPerThread + j);
+            callback->OnEncodedImage(encoded_frame, nullptr);
+          }
+          barrier.Run();
+        }));
+  }
+  run_loop.Run();
+}
+
 }  // namespace
 }  // namespace blink