From 9160b627d79a26c586c6bfdb6dec30e8c78ee91b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sami=20Kalliom=C3=A4ki?= <sakal@webrtc.org>
Date: Thu, 1 Aug 2019 17:16:29 +0200
Subject: [PATCH] Improve thread safety of AndroidVideoTrackSource::SetState.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

1. Prevents deadlocks from AsyncInvoker destructor
2. Makes future state() calls are guaranteed to return the new state after
   SetState() completes.

I am not sure if it is allowed to call FireOnChanged from non-signaling
threads so I will leave the post for now.

Bug: webrtc:10813
Change-Id: I5712a45f71431765898037867382397d537570a0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147727
Commit-Queue: Sami Kalliomäki <sakal@webrtc.org>
Reviewed-by: Magnus Jedvert <magjed@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28741}
---
 .../src/jni/android_video_track_source.cc     | 31 ++++++++++---------
 .../src/jni/android_video_track_source.h      |  6 ++--
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/sdk/android/src/jni/android_video_track_source.cc b/sdk/android/src/jni/android_video_track_source.cc
index a72a3f667a..df0f72284c 100644
--- a/sdk/android/src/jni/android_video_track_source.cc
+++ b/sdk/android/src/jni/android_video_track_source.cc
@@ -14,6 +14,7 @@
 
 #include <utility>
 
+#include "rtc_base/bind.h"
 #include "rtc_base/logging.h"
 
 namespace webrtc {
@@ -60,25 +61,25 @@ absl::optional<bool> AndroidVideoTrackSource::needs_denoising() const {
 
 void AndroidVideoTrackSource::SetState(JNIEnv* env,
                                        jboolean j_is_live) {
-  InternalSetState(j_is_live ? kLive : kEnded);
-}
-
-void AndroidVideoTrackSource::InternalSetState(SourceState state) {
-  if (rtc::Thread::Current() != signaling_thread_) {
-    invoker_.AsyncInvoke<void>(
-        RTC_FROM_HERE, signaling_thread_,
-        rtc::Bind(&AndroidVideoTrackSource::InternalSetState, this, state));
-    return;
-  }
-
-  if (state_ != state) {
-    state_ = state;
-    FireOnChanged();
+  const SourceState state = j_is_live ? kLive : kEnded;
+  if (state_.exchange(state) != state) {
+    if (rtc::Thread::Current() == signaling_thread_) {
+      FireOnChanged();
+    } else {
+      // TODO(sakal): Is this even necessary, does FireOnChanged have to be
+      // called from signaling thread?
+      signaling_thread_->PostTask(
+          RTC_FROM_HERE,
+          rtc::Bind(
+              &AndroidVideoTrackSource::FireOnChanged,
+              static_cast<webrtc::Notifier<webrtc::VideoTrackSourceInterface>*>(
+                  this)));
+    }
   }
 }
 
 AndroidVideoTrackSource::SourceState AndroidVideoTrackSource::state() const {
-  return state_;
+  return state_.load();
 }
 
 bool AndroidVideoTrackSource::remote() const {
diff --git a/sdk/android/src/jni/android_video_track_source.h b/sdk/android/src/jni/android_video_track_source.h
index 98333cb32c..d272275aed 100644
--- a/sdk/android/src/jni/android_video_track_source.h
+++ b/sdk/android/src/jni/android_video_track_source.h
@@ -18,6 +18,7 @@
 #include "media/base/adapted_video_track_source.h"
 #include "rtc_base/async_invoker.h"
 #include "rtc_base/checks.h"
+#include "rtc_base/thread.h"
 #include "rtc_base/timestamp_aligner.h"
 #include "sdk/android/src/jni/video_frame.h"
 
@@ -84,11 +85,8 @@ class AndroidVideoTrackSource : public rtc::AdaptedVideoTrackSource {
                          const JavaRef<jobject>& j_max_fps);
 
  private:
-  void InternalSetState(SourceState state);
-
   rtc::Thread* signaling_thread_;
-  rtc::AsyncInvoker invoker_;
-  SourceState state_;
+  std::atomic<SourceState> state_;
   const bool is_screencast_;
   rtc::TimestampAligner timestamp_aligner_;
   const bool align_timestamps_;