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 @@
 //
 // It's assumed that Configure(), Decode(), and RegisterDecodeCompleteCallback()
 // are called on the decode sequence. Decoded() may be called on either the
-// decode sequecene or the media sequence depending on if the underlying decoder
-// is a HW or SW decoder. However, the calls to Decoded() on these sequences are
-// mutual exclusive. Release() may be called on any sequence as long as the
-// decoding sequence has stopped.
+// decode sequence or the media sequence depending on if the underlying decoder
+// is a HW or SW decoder. During a fallback from HW to SW decoding, Decoded()
+// callbacks can arrive concurrently on both sequences. Release() may be called
+// on any sequence as long as the decoding sequence has stopped.
 class PLATFORM_EXPORT StatsCollectingDecoder
     : public webrtc::VideoDecoder,
       private webrtc::DecodedImageCallback {
@@ -61,7 +61,8 @@
   DecoderInfo GetDecoderInfo() const override;
 
  private:
-  void ReportStats(const StatsCollector::Stats& stats) const;
+  void ReportStats(const StatsCollector::Stats& stats) const
+      LOCKS_EXCLUDED(lock_);
 
   // Implementation of webrtc::DecodedImageCallback.
   int32_t Decoded(webrtc::VideoFrame& decodedImage) override;
@@ -78,14 +79,14 @@
   // the decode sequence and the media sequence.
   base::Lock lock_;
 
-  bool first_frame_decoded_{false};
+  bool first_frame_decoded_ GUARDED_BY(lock_) = false;
   // `number_of_new_keyframes_` is used to count the number of processed key
   // frames, which is only known in Decode(). The value of this counter is
   // continuously read out in the Decoded() callback.
   size_t number_of_new_keyframes_ GUARDED_BY(lock_){0};
-  base::TimeTicks last_check_for_simultaneous_decoders_;
+  base::TimeTicks last_check_for_simultaneous_decoders_ GUARDED_BY(lock_);
 
-  StatsCollector stats_collector_;
+  StatsCollector stats_collector_ GUARDED_BY(lock_);
 
   SEQUENCE_CHECKER(decoding_sequence_checker_);
 };
diff --git a/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder.cc b/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder.cc
index 8826ed13..cc88095 100644
--- a/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder.cc
+++ b/third_party/blink/renderer/platform/peerconnection/stats_collecting_encoder.cc
@@ -68,7 +68,10 @@
   // the call to encoder->InitEncode().
   int ret = encoder_->InitEncode(codec_settings, settings);
   // Reset to the default value.
-  highest_observed_stream_index_ = 0;
+  {
+    base::AutoLock auto_lock(lock_);
+    highest_observed_stream_index_ = 0;
+  }
   return ret;
 }
 
@@ -85,21 +88,28 @@
   // Release is called after encode_sequence has been stopped.
   DVLOG(3) << __func__;
   int32_t ret = encoder_->Release();
-  // There will be no new calls to Encoded() after the call to
-  // encoder_->Release(). Any outstanding calls to Encoded() will also finish
-  // before encoder_->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_);
 
-  if (first_frame_encoded_) {
-    --(*GetEncoderCounter());
-    first_frame_encoded_ = false;
-  }
+    // There shouldn't be any new calls to OnEncodedImage() after the call to
+    // encoder_->Release(). Any outstanding calls to Encoded() will typically
+    // finish before encoder_->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_encoded_) {
+      --(*GetEncoderCounter());
+      first_frame_encoded_ = false;
+    }
+  }
+  if (stats_to_report) {
+    ReportStats(*stats_to_report);
+  }
   return ret;
 }
 
@@ -107,15 +117,14 @@
     const webrtc::VideoFrame& frame,
     const std::vector<webrtc::VideoFrameType>* frame_types) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(encoding_sequence_checker_);
-  if (!first_frame_encoded_) {
-    first_frame_encoded_ = true;
-    ++(*GetEncoderCounter());
-  }
-
   base::TimeTicks now = base::TimeTicks::Now();
   {
     // Store the timestamp.
     base::AutoLock auto_lock(lock_);
+    if (!first_frame_encoded_) {
+      first_frame_encoded_ = true;
+      ++(*GetEncoderCounter());
+    }
     constexpr size_t kMaxEncodeStartInfoSize = 10;
     // If encode_start_info_.size() increases it means that stats collection is
     // not active in the OnEncodedImage() callback. Pop the oldest element here
@@ -153,79 +162,81 @@
 webrtc::EncodedImageCallback::Result StatsCollectingEncoder::OnEncodedImage(
     const webrtc::EncodedImage& encoded_image,
     const webrtc::CodecSpecificInfo* codec_specific_info) {
-  // OnEncodedImage may be called on either the encoding sequence (SW encoding)
-  // or gpu sequence (HW encoding). However, these calls are not happening at
-  // the same time. If there's a fallback from SW encoding to HW encoding, a
-  // call to HW encoder->Release() ensures that any potential callbacks on the
-  // gpu sequence are finished before the encoding continues on the encoding
-  // sequence.
   DCHECK(encoded_callback_);
   webrtc::EncodedImageCallback::Result result =
       encoded_callback_->OnEncodedImage(encoded_image, codec_specific_info);
 
-  const size_t encoded_image_stream_index =
-      encoded_image.SimulcastIndex().value_or(
-          encoded_image.SpatialIndex().value_or(0));
-  highest_observed_stream_index_ =
-      std::max(highest_observed_stream_index_, encoded_image_stream_index);
+  // In multi-encoder simulcast mode (mixed HW/SW layers), per-layer
+  // OnEncodedImage callbacks can arrive concurrently on the encoder thread (SW)
+  // and the GPU media thread (HW). All StatsCollector state below is shared, so
+  // serialize the entire stats-accounting section. The forward to
+  // `encoded_callback_` above is intentionally kept outside the lock.
 
-  if (stats_collector_.has_finished() ||
-      encoded_image_stream_index != highest_observed_stream_index_) {
-    // Return early if we've already finished the stats collection or if this is
-    // a lower stream layer. We only do stats collection for the highest
-    // observed stream layer.
-    return result;
-  }
+  std::optional<StatsCollector::Stats> stats_to_report;
+  {
+    base::AutoLock auto_lock(lock_);
 
-  base::TimeTicks now = base::TimeTicks::Now();
-  // Verify that there's only a single encoder when data collection is taking
-  // place.
-  if ((now - last_check_for_simultaneous_encoders_) >
-      kCheckSimultaneousEncodersInterval) {
-    last_check_for_simultaneous_encoders_ = now;
-    DVLOG(3) << "Simultaneous encoders: " << *GetEncoderCounter();
-    if (stats_collector_.is_active()) {
-      if (*GetEncoderCounter() > kMaximumEncodersToCollectStats) {
-        // Too many encoders, cancel stats collection.
-        stats_collector_.Clear();
-      }
-    } else if (*GetEncoderCounter() <= kMaximumEncodersToCollectStats) {
-      // Start up stats collection since there's only a single encoder active.
-      stats_collector_.Start();
+    const size_t encoded_image_stream_index =
+        encoded_image.SimulcastIndex().value_or(
+            encoded_image.SpatialIndex().value_or(0));
+    highest_observed_stream_index_ =
+        std::max(highest_observed_stream_index_, encoded_image_stream_index);
+
+    if (stats_collector_.has_finished() ||
+        encoded_image_stream_index != highest_observed_stream_index_) {
+      // Return early if we've already finished the stats collection or if this
+      // is a lower stream layer. We only do stats collection for the highest
+      // observed stream layer.
+      return result;
     }
-  }
 
-  if (stats_collector_.is_active()) {
-    std::optional<base::TimeTicks> encode_start;
-    {
+    base::TimeTicks now = base::TimeTicks::Now();
+    // Verify that there's only a single encoder when data collection is taking
+    // place.
+    if ((now - last_check_for_simultaneous_encoders_) >
+        kCheckSimultaneousEncodersInterval) {
+      last_check_for_simultaneous_encoders_ = now;
+      DVLOG(3) << "Simultaneous encoders: " << *GetEncoderCounter();
+      if (stats_collector_.is_active()) {
+        if (*GetEncoderCounter() > kMaximumEncodersToCollectStats) {
+          // Too many encoders, cancel stats collection.
+          stats_collector_.Clear();
+        }
... (file diff truncated)

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