[WebRTC] Deadlock when creating SwitchableVideoEncoder

It may be possible that RTCPeerConnection-iceConnectionState.https
TCT Test fails sometimes due to deadlock on following threads:

JS Main thread -> WebRTC Worker Thread -> Encoder Thread.

JS Main thread is calling closes RTCPeerConnection
(`RTCPeerConnection.close()`) which closes `PeerConnectionInterface` on
WebRTC worker thread. This call leads to removing VideoStreamEncoder
from `WebRtcVideoSendChannel`. This call sends request to Encoder
Thread, to stop any encoders. In the same time encoder is initialized
and it tries to connect to suspend_resume service. To do this it
delegates call to JS Main thread to get Mojo interfaces.

This patch changes way of registering in SuspendResume service, by
reusing `blink::Platform::GetBrowserInterfaceBroker()` mechanism, which
binds to mojo services on IO thread.

Bug: https://jira-eu.sec.samsung.net/browse/VDWASM-1942
Change-Id: I8a3a02c22b9f44f7b0ac80e04ddeeadfe1237aa1

diff --git a/content/browser/browser_interface_binders.cc b/content/browser/browser_interface_binders.cc
index 4888f1fcda2404afd587f4a6bdd47d036d708487..d828ef15e47ef93f72c16a6599acb498b1ee356b 100644 (file)
--- a/content/browser/browser_interface_binders.cc
+++ b/content/browser/browser_interface_binders.cc
@@ -781,15 +781,6 @@ void BindSocketManager(
           std::move(receiver), frame->GetGlobalId());
 }
 
-#if defined(TIZEN_TV_UPSTREAM_MULTIMEDIA)
-void BindSuspendResumeManager(
-    mojo::PendingReceiver<suspend_resume::mojom::SuspendResumeManager>
-        receiver) {
-  suspend_resume::SuspendResumeServiceImpl::GetInstance()->AddReceiver(
-      std::move(receiver));
-}
-#endif  // defined(TIZEN_TV_UPSTREAM_MULTIMEDIA)
-
 }  // namespace
 
 #if BUILDFLAG(IS_TIZEN_TV)
@@ -814,8 +805,8 @@ void PopulateFrameBinders(RenderFrameHostImpl* host, mojo::BinderMap* map) {
 #endif
 
 #if defined(TIZEN_TV_UPSTREAM_MULTIMEDIA)
-  map->Add<suspend_resume::mojom::SuspendResumeManager>(
-      base::BindRepeating(&BindSuspendResumeManager));
+  map->Add<suspend_resume::mojom::SuspendResumeManager>(base::BindRepeating(
+      &suspend_resume::SuspendResumeServiceImpl::BindSuspendResumeManager));
 #endif  // defined(TIZEN_TV_UPSTREAM_MULTIMEDIA)
 
   map->Add<blink::mojom::AudioContextManager>(base::BindRepeating(

diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 4297fd2e61b540bc895ce7a03cba61d004e8bcd5..1e139aa68b82a088bfe875d7eba639f1bdd851d9 100644 (file)
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -311,6 +311,10 @@
 #include "tizen_src/ewk/efl_integration/common/content_switches_efl.h"
 #endif
 
+#if defined(TIZEN_TV_UPSTREAM_MULTIMEDIA)
+#include "tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.h"
+#endif
+
 namespace content {
 
 namespace {
@@ -2486,6 +2490,13 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
                           instance_weak_factory_.GetWeakPtr()));
 #endif
 
+#if defined(TIZEN_TV_UPSTREAM_MULTIMEDIA)
+  AddUIThreadInterface(
+      registry.get(),
+      base::BindRepeating(
+          &suspend_resume::SuspendResumeServiceImpl::BindSuspendResumeManager));
+#endif
+
   // ---- Please do not register interfaces below this line ------
   //
   // This call should be done after registering all interfaces above, so that

diff --git a/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.cc b/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.cc
index 797cd6645bd9870da6b6127d1668cf5ed28448b5..27f32affef7e008d0dac087fcf09ef3361fdbcac 100644 (file)
--- a/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.cc
+++ b/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.cc
@@ -8,7 +8,16 @@ namespace suspend_resume {
 
 // static
 SuspendResumeServiceImpl* SuspendResumeServiceImpl::GetInstance() {
-  return base::Singleton<SuspendResumeServiceImpl>::get();
+  static base::NoDestructor<SuspendResumeServiceImpl> instance;
+  return instance.get();
+}
+
+// static
+void SuspendResumeServiceImpl::BindSuspendResumeManager(
+    mojo::PendingReceiver<suspend_resume::mojom::SuspendResumeManager>
+        receiver) {
+  suspend_resume::SuspendResumeServiceImpl::GetInstance()->AddReceiver(
+      std::move(receiver));
 }
 
 void SuspendResumeServiceImpl::Notify(mojom::State new_state) {

diff --git a/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.h b/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.h
index 629a794461ae0a90920d8902c4c58f6cb0755b6c..18430bd34248f67962300e4df8c60f16570a2594 100644 (file)
--- a/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.h
+++ b/tizen_src/chromium_impl/services/suspend_resume/suspend_resume_service_impl.h
@@ -7,7 +7,7 @@
 
 #include "tizen_src/chromium_impl/services/suspend_resume/public/mojom/suspend_resume_service.mojom.h"
 
-#include "base/memory/singleton.h"
+#include "base/no_destructor.h"
 #include "base/synchronization/lock.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
@@ -19,6 +19,9 @@ namespace suspend_resume {
 class SuspendResumeServiceImpl : public mojom::SuspendResumeManager {
  public:
   static SuspendResumeServiceImpl* GetInstance();
+  static void BindSuspendResumeManager(
+      mojo::PendingReceiver<suspend_resume::mojom::SuspendResumeManager>
+          receiver);
 
   void Notify(mojom::State new_state);
   absl::optional<mojom::State> RegisterClient(
@@ -43,7 +46,7 @@ class SuspendResumeServiceImpl : public mojom::SuspendResumeManager {
   mojo::ReceiverSet<mojom::SuspendResumeManager> receivers_;
   mojo::RemoteSet<mojom::SuspendResumeObserver> observers_;
 
-  friend base::DefaultSingletonTraits<SuspendResumeServiceImpl>;
+  friend base::NoDestructor<SuspendResumeServiceImpl>;
 };
 
 }  // namespace suspend_resume

diff --git a/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_decoder_wrapper.cc b/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_decoder_wrapper.cc
index 52c8bc4679ff036bc5b386b565b7a2c32990b3f4..e8b615f250286ad84c26fde18bdfc7b318887819 100644 (file)
--- a/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_decoder_wrapper.cc
+++ b/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_decoder_wrapper.cc
@@ -5,10 +5,10 @@
 #include "third_party/blink/renderer/platform/peerconnection/switchable_video_decoder_wrapper.h"
 
 #include "base/logging.h"
-#include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
-#include "third_party/blink/renderer/platform/peerconnection/rtc_tools.h"
+#include "third_party/blink/public/common/thread_safe_browser_interface_broker_proxy.h"
+#include "third_party/blink/public/platform/platform.h"
 #include "third_party/webrtc/modules/video_coding/include/video_error_codes.h"
 #include "tizen_src/chromium_impl/services/suspend_resume/public/cpp/suspend_resume.h"
 
@@ -16,7 +16,7 @@ namespace blink {
 
 namespace {
 
-constexpr size_t kMaxConsequtiveHwErrors = 4;
+constexpr size_t kMaxConsecutiveHwErrors = 4;
 
 enum class DecodingMode {
   kNone,
@@ -83,7 +83,7 @@ class SwitchableVideoDecoderWrapper final
   webrtc::VideoDecoder::Settings decoder_settings_;
   const std::string fallback_implementation_name_;
   webrtc::DecodedImageCallback* callback_;
-  size_t hw_consequtive_generic_errors_;
+  size_t hw_consecutive_generic_errors_;
 
   suspend_resume::State last_state_;
 
@@ -100,14 +100,21 @@ SwitchableVideoDecoderWrapper::SwitchableVideoDecoderWrapper(
           " (fallback from: " +
           hw_decoder_->GetDecoderInfo().implementation_name + ")"),
       callback_(nullptr),
-      hw_consequtive_generic_errors_(0) {
-  mojo::PendingRemote<suspend_resume::mojom::SuspendResumeManager>
-      pending_manager;
-  GetInterface(pending_manager.InitWithNewPipeAndPassReceiver());
+      hw_consecutive_generic_errors_(0) {
+  mojo::Remote<suspend_resume::mojom::SuspendResumeManager> manager;
+  Platform::Current()->GetBrowserInterfaceBroker()->GetInterface(
+      manager.BindNewPipeAndPassReceiver());
   absl::optional<suspend_resume::State> state;
-  mojo::Remote<suspend_resume::mojom::SuspendResumeManager>(
-      std::move(pending_manager))
-      ->AddObserver(receiver_.BindNewPipeAndPassRemote(), &state);
+  bool ret = manager->AddObserver(receiver_.BindNewPipeAndPassRemote(), &state);
+  if (!ret) {
+    LOG(ERROR) << "Failed to register in suspend resume service! "
+               << "Only software decoding will be available.";
+  } else if (!state) {
+    LOG(WARNING) << "Unknown suspend_resume::State! Assuming SUSPENDED";
+  } else {
+    LOG(INFO) << "Initial state: " << *state;
+  }
+
   last_state_ = state.value_or(suspend_resume::State::SUSPENDED);
 }
 
@@ -168,16 +175,16 @@ int32_t SwitchableVideoDecoderWrapper::Decode(
       ret = hw_decoder_->Decode(input_image, missing_frames, render_time_ms);
       if (ret != WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE) {
         if (ret != WEBRTC_VIDEO_CODEC_ERROR) {
-          hw_consequtive_generic_errors_ = 0;
+          hw_consecutive_generic_errors_ = 0;
           return ret;
         }
         if (input_image._frameType == webrtc::VideoFrameType::kVideoFrameKey) {
           // Only count errors on key-frames, since generic errors can happen
           // with hw decoder due to many arbitrary reasons.
           // However, requesting a key-frame is supposed to fix the issue.
-          ++hw_consequtive_generic_errors_;
+          ++hw_consecutive_generic_errors_;
         }
-        if (hw_consequtive_generic_errors_ < kMaxConsequtiveHwErrors) {
+        if (hw_consecutive_generic_errors_ < kMaxConsecutiveHwErrors) {
           return ret;
         }
       }

diff --git a/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_encoder_wrapper.cc b/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_encoder_wrapper.cc
index b0fa6ed0ca039833e020a8e3a614fcf5c250966e..9841a33dc42a4242f9eb4034ecd2a262c595f863 100644 (file)
--- a/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_encoder_wrapper.cc
+++ b/tizen_src/chromium_impl/third_party/blink/renderer/platform/peerconnection/switchable_video_encoder_wrapper.cc
@@ -7,12 +7,11 @@
 #include <list>
 #include <vector>
 
-#include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/suspend_resume/public/cpp/suspend_resume.h"
-#include "third_party/blink/renderer/platform/peerconnection/rtc_tools.h"
-#include "third_party/blink/renderer/platform/webrtc/webrtc_video_frame_adapter.h"
+#include "third_party/blink/public/common/thread_safe_browser_interface_broker_proxy.h"
+#include "third_party/blink/public/platform/platform.h"
 #include "third_party/webrtc/api/video/video_codec_constants.h"
 #include "third_party/webrtc/modules/video_coding/include/video_codec_interface.h"
 #include "third_party/webrtc/modules/video_coding/include/video_error_codes.h"
@@ -146,13 +145,20 @@ SwitchableVideoEncoderWrapper::SwitchableVideoEncoderWrapper(
       sw_encoder_factory_(sw_encoder_factory),
       format_(format),
       settings_(webrtc::VideoEncoder::Capabilities{false}, 1, 0) {
-  mojo::PendingRemote<suspend_resume::mojom::SuspendResumeManager>
-      pending_manager;
-  GetInterface(pending_manager.InitWithNewPipeAndPassReceiver());
+  mojo::Remote<suspend_resume::mojom::SuspendResumeManager> manager;
+  Platform::Current()->GetBrowserInterfaceBroker()->GetInterface(
+      manager.BindNewPipeAndPassReceiver());
   absl::optional<suspend_resume::State> state;
-  mojo::Remote<suspend_resume::mojom::SuspendResumeManager>(
-      std::move(pending_manager))
-      ->AddObserver(receiver_.BindNewPipeAndPassRemote(), &state);
+  bool ret = manager->AddObserver(receiver_.BindNewPipeAndPassRemote(), &state);
+  if (!ret) {
+    LOG(ERROR) << "Failed to register in suspend resume service! "
+               << "Only software encoding will be available.";
+  } else if (!state) {
+    LOG(WARNING) << "Unknown suspend_resume::State! Assuming SUSPENDED";
+  } else {
+    LOG(INFO) << "Initial state: " << *state;
+  }
+
   // When the actual state is not yet known (shortly after process start),
   // it's treated as suspended. This prevents from stealing resources from
   // other app. After receiving any notification it'll be changed, possibly