Android: Fix a race condition in VideoDecoderWrapper.

Fixes a race condition where frame_extra_infos_ is accessed from
multiple threads by adding a lock.

Adds thread safety idioms to the file to guard agains similar mistakes
in the future.

Bug: b/72979294
Change-Id: I0f2f947282a5b3414f1351e9e8e52ad523f7d2f6
Reviewed-on: https://webrtc-review.googlesource.com/48641
Reviewed-by: Magnus Jedvert <magjed@webrtc.org>
Commit-Queue: Sami Kalliomäki <sakal@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21926}

sdk/android/src/jni/videodecoderwrapper.cc

@@ -38,17 +38,21 @@ inline rtc::Optional<Dst> cast_optional(const rtc::Optional<Src>& value) {
 
 VideoDecoderWrapper::VideoDecoderWrapper(JNIEnv* jni,
                                          const JavaRef<jobject>& decoder)
-    : decoder_(jni, decoder) {
+    : decoder_(jni, decoder),
-  initialized_ = false;
+      implementation_name_(JavaToStdString(
-  // QP parsing starts enabled and we disable it if the decoder provides frames.
+          jni,
-  qp_parsing_enabled_ = true;
+          Java_VideoDecoder_getImplementationName(jni, decoder))),
+      initialized_(false),
+      qp_parsing_enabled_(true)  // QP parsing starts enabled and we disable it
+                                 // if the decoder provides frames.
 
-  implementation_name_ = JavaToStdString(
+{
-      jni, Java_VideoDecoder_getImplementationName(jni, decoder));
+  decoder_thread_checker_.DetachFromThread();
 }
 
 int32_t VideoDecoderWrapper::InitDecode(const VideoCodec* codec_settings,
                                         int32_t number_of_cores) {
+  RTC_DCHECK_RUN_ON(&decoder_thread_checker_);
   JNIEnv* jni = AttachCurrentThreadIfNeeded();
   codec_settings_ = *codec_settings;
   number_of_cores_ = number_of_cores;
@@ -82,6 +86,7 @@ int32_t VideoDecoderWrapper::Decode(
     const RTPFragmentationHeader* fragmentation,
     const CodecSpecificInfo* codec_specific_info,
     int64_t render_time_ms) {
+  RTC_DCHECK_RUN_ON(&decoder_thread_checker_);
   if (!initialized_) {
     // Most likely initializing the codec failed.
     return WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE;
@@ -100,7 +105,10 @@ int32_t VideoDecoderWrapper::Decode(
   frame_extra_info.timestamp_ntp = input_image.ntp_time_ms_;
   frame_extra_info.qp =
       qp_parsing_enabled_ ? ParseQP(input_image) : rtc::nullopt;
+  {
+    rtc::CritScope cs(&frame_extra_infos_lock_);
     frame_extra_infos_.push_back(frame_extra_info);
+  }
 
   JNIEnv* env = AttachCurrentThreadIfNeeded();
   ScopedJavaLocalRef<jobject> jinput_image =
@@ -113,6 +121,7 @@ int32_t VideoDecoderWrapper::Decode(
 
 int32_t VideoDecoderWrapper::RegisterDecodeCompleteCallback(
     DecodedImageCallback* callback) {
+  RTC_DCHECK_RUNS_SERIALIZED(&callback_race_checker_);
   callback_ = callback;
   return WEBRTC_VIDEO_CODEC_OK;
 }
@@ -120,9 +129,15 @@ int32_t VideoDecoderWrapper::RegisterDecodeCompleteCallback(
 int32_t VideoDecoderWrapper::Release() {
   JNIEnv* jni = AttachCurrentThreadIfNeeded();
   ScopedJavaLocalRef<jobject> ret = Java_VideoDecoder_release(jni, decoder_);
+  {
+    rtc::CritScope cs(&frame_extra_infos_lock_);
     frame_extra_infos_.clear();
+  }
   initialized_ = false;
-  return HandleReturnCode(jni, ret);
+  int32_t status = HandleReturnCode(jni, ret);
+  // It is allowed to reinitialize the codec on a different thread.
+  decoder_thread_checker_.DetachFromThread();
+  return status;
 }
 
 bool VideoDecoderWrapper::PrefersLateDecoding() const {
@@ -140,13 +155,17 @@ void VideoDecoderWrapper::OnDecodedFrame(
     const JavaRef<jobject>& j_frame,
     const JavaRef<jobject>& j_decode_time_ms,
     const JavaRef<jobject>& j_qp) {
+  RTC_DCHECK_RUNS_SERIALIZED(&callback_race_checker_);
   const uint64_t timestamp_ns = GetJavaVideoFrameTimestampNs(env, j_frame);
 
   FrameExtraInfo frame_extra_info;
+  {
+    rtc::CritScope cs(&frame_extra_infos_lock_);
+
     do {
       if (frame_extra_infos_.empty()) {
-      RTC_LOG(LS_WARNING) << "Java decoder produced an unexpected frame: "
+        RTC_LOG(LS_WARNING)
-                          << timestamp_ns;
+            << "Java decoder produced an unexpected frame: " << timestamp_ns;
         return;
       }
 
@@ -155,6 +174,7 @@ void VideoDecoderWrapper::OnDecodedFrame(
       // If the decoder might drop frames so iterate through the queue until we
       // find a matching timestamp.
     } while (frame_extra_info.timestamp_ns != timestamp_ns);
+  }
 
   VideoFrame frame =
       JavaToNativeFrame(env, j_frame, frame_extra_info.timestamp_rtp);

sdk/android/src/jni/videodecoderwrapper.h

@@ -12,10 +12,13 @@
 #define SDK_ANDROID_SRC_JNI_VIDEODECODERWRAPPER_H_
 
 #include <jni.h>
+#include <atomic>
 #include <deque>
 
 #include "api/video_codecs/video_decoder.h"
 #include "common_video/h264/h264_bitstream_parser.h"
+#include "rtc_base/race_checker.h"
+#include "rtc_base/thread_checker.h"
 #include "sdk/android/src/jni/jni_helpers.h"
 
 namespace webrtc {
@@ -38,7 +41,10 @@ class VideoDecoderWrapper : public VideoDecoder {
   int32_t RegisterDecodeCompleteCallback(
       DecodedImageCallback* callback) override;
 
-  int32_t Release() override;
+  // TODO(sakal): This is not always called on the correct thread. It is called
+  // from VCMGenericDecoder destructor which is on a different thread but is
+  // still safe and synchronous.
+  int32_t Release() override RTC_NO_THREAD_SAFETY_ANALYSIS;
 
   // Returns true if the decoder prefer to decode frames late.
   // That is, it can not decode infinite number of frames before the decoded
@@ -63,26 +69,39 @@ class VideoDecoderWrapper : public VideoDecoder {
     rtc::Optional<uint8_t> qp;
   };
 
-  int32_t InitDecodeInternal(JNIEnv* jni);
+  int32_t InitDecodeInternal(JNIEnv* jni) RTC_RUN_ON(decoder_thread_checker_);
 
   // Takes Java VideoCodecStatus, handles it and returns WEBRTC_VIDEO_CODEC_*
   // status code.
-  int32_t HandleReturnCode(JNIEnv* jni, const JavaRef<jobject>& code);
+  int32_t HandleReturnCode(JNIEnv* jni, const JavaRef<jobject>& code)
+      RTC_RUN_ON(decoder_thread_checker_);
 
-  rtc::Optional<uint8_t> ParseQP(const EncodedImage& input_image);
+  rtc::Optional<uint8_t> ParseQP(const EncodedImage& input_image)
-
+      RTC_RUN_ON(decoder_thread_checker_);
-  VideoCodec codec_settings_;
-  int32_t number_of_cores_;
-
-  bool initialized_;
-  std::deque<FrameExtraInfo> frame_extra_infos_;
-  bool qp_parsing_enabled_;
-  H264BitstreamParser h264_bitstream_parser_;
-  std::string implementation_name_;
-
-  DecodedImageCallback* callback_;
 
   const ScopedJavaGlobalRef<jobject> decoder_;
+  const std::string implementation_name_;
+
+  rtc::ThreadChecker decoder_thread_checker_;
+  // Callbacks must be executed sequentially on an arbitrary thread. We do not
+  // own this thread so a thread checker cannot be used.
+  rtc::RaceChecker callback_race_checker_;
+
+  // Initialized on InitDecode and immutable after that.
+  VideoCodec codec_settings_ RTC_ACCESS_ON(decoder_thread_checker_);
+  int32_t number_of_cores_ RTC_ACCESS_ON(decoder_thread_checker_);
+
+  bool initialized_ RTC_ACCESS_ON(decoder_thread_checker_);
+  H264BitstreamParser h264_bitstream_parser_
+      RTC_ACCESS_ON(decoder_thread_checker_);
+
+  DecodedImageCallback* callback_ RTC_ACCESS_ON(callback_race_checker_);
+
+  // Accessed both on the decoder thread and the callback thread.
+  std::atomic<bool> qp_parsing_enabled_;
+  rtc::CriticalSection frame_extra_infos_lock_;
+  std::deque<FrameExtraInfo> frame_extra_infos_
+      RTC_ACCESS_ON(frame_extra_infos_lock_);
 };
 
 }  // namespace jni