From c75c4280763435eeb44785dfc76f098bf06743bb Mon Sep 17 00:00:00 2001
From: Harald Alvestrand <hta@webrtc.org>
Date: Wed, 26 Aug 2020 12:17:54 +0000
Subject: [PATCH] Fix current_direction() when stopping_ but not stopped_
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Also add an unit test for RtpTransceiver under Unified Plan, and
refactor so that we no longer use StopInternal() internally.
This will make removing it easier.

Bug: chromium:980879
Change-Id: I46219112e3aba8e7513c08336b10e95b1ea5d68b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/182681
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31999}
---
 pc/peer_connection.cc          |  2 +-
 pc/rtp_transceiver.cc          | 13 +++++++++++--
 pc/rtp_transceiver.h           |  4 ++++
 pc/rtp_transceiver_unittest.cc | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 34e638f3cf..3665a82e03 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -3337,7 +3337,7 @@ RTCError PeerConnection::ApplyRemoteDescription(
       if (content->rejected && !transceiver->stopped()) {
         RTC_LOG(LS_INFO) << "Stopping transceiver for MID=" << content->name
                          << " since the media section was rejected.";
-        transceiver->StopInternal();
+        transceiver->internal()->StopTransceiverProcedure();
       }
       if (!content->rejected &&
           RtpTransceiverDirectionHasRecv(local_direction)) {
diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc
index b267e4e333..fd6f4bbd17 100644
--- a/pc/rtp_transceiver.cc
+++ b/pc/rtp_transceiver.cc
@@ -321,7 +321,7 @@ RTCError RtpTransceiver::SetDirectionWithError(
 
 absl::optional<RtpTransceiverDirection> RtpTransceiver::current_direction()
     const {
-  if (unified_plan_ && stopping())
+  if (unified_plan_ && stopped())
     return webrtc::RtpTransceiverDirection::kStopped;
 
   return current_direction_;
@@ -354,7 +354,11 @@ void RtpTransceiver::StopSendingAndReceiving() {
 
 RTCError RtpTransceiver::StopStandard() {
   RTC_DCHECK_RUN_ON(thread_);
-  RTC_DCHECK(unified_plan_);
+  // If we're on Plan B, do what Stop() used to do there.
+  if (!unified_plan_) {
+    StopInternal();
+    return RTCError::OK();
+  }
   // 1. Let transceiver be the RTCRtpTransceiver object on which the method is
   // invoked.
   //
@@ -380,7 +384,12 @@ RTCError RtpTransceiver::StopStandard() {
 }
 
 void RtpTransceiver::StopInternal() {
+  StopTransceiverProcedure();
+}
+
+void RtpTransceiver::StopTransceiverProcedure() {
   RTC_DCHECK_RUN_ON(thread_);
+  // As specified in the "Stop the RTCRtpTransceiver" procedure
   // 1. If transceiver.[[Stopping]] is false, stop sending and receiving given
   // transceiver.
   if (!stopping_)
diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h
index 1d3ceaef61..9c048adc06 100644
--- a/pc/rtp_transceiver.h
+++ b/pc/rtp_transceiver.h
@@ -177,6 +177,10 @@ class RtpTransceiver final
   // PeerConnection is closed.
   void SetPeerConnectionClosed();
 
+  // Executes the "stop the RTCRtpTransceiver" procedure from
+  // the webrtc-pc specification, described under the stop() method.
+  void StopTransceiverProcedure();
+
   // Fired when the RtpTransceiver state changes such that negotiation is now
   // needed (e.g., in response to a direction change).
   sigslot::signal0<> SignalNegotiationNeeded;
diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc
index 78abfff8b9..e4883f32f6 100644
--- a/pc/rtp_transceiver_unittest.cc
+++ b/pc/rtp_transceiver_unittest.cc
@@ -79,6 +79,40 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) {
   EXPECT_EQ(nullptr, transceiver.channel());
 }
 
+class RtpTransceiverUnifiedPlanTest : public ::testing::Test {
+ public:
+  RtpTransceiverUnifiedPlanTest()
+      : channel_manager_(std::make_unique<cricket::FakeMediaEngine>(),
+                         std::make_unique<cricket::FakeDataEngine>(),
+                         rtc::Thread::Current(),
+                         rtc::Thread::Current()),
+        transceiver_(RtpSenderProxyWithInternal<RtpSenderInternal>::Create(
+                         rtc::Thread::Current(),
+                         new rtc::RefCountedObject<MockRtpSenderInternal>()),
+                     RtpReceiverProxyWithInternal<RtpReceiverInternal>::Create(
+                         rtc::Thread::Current(),
+                         new rtc::RefCountedObject<MockRtpReceiverInternal>()),
+                     &channel_manager_,
+                     channel_manager_.GetSupportedAudioRtpHeaderExtensions()) {}
+
+  cricket::ChannelManager channel_manager_;
+  RtpTransceiver transceiver_;
+};
+
+// Basic tests for Stop()
+TEST_F(RtpTransceiverUnifiedPlanTest, StopSetsDirection) {
+  EXPECT_EQ(RtpTransceiverDirection::kInactive, transceiver_.direction());
+  EXPECT_FALSE(transceiver_.current_direction());
+  transceiver_.StopStandard();
+  EXPECT_EQ(RtpTransceiverDirection::kStopped, transceiver_.direction());
+  EXPECT_FALSE(transceiver_.current_direction());
+  transceiver_.StopTransceiverProcedure();
+  EXPECT_TRUE(transceiver_.current_direction());
+  EXPECT_EQ(RtpTransceiverDirection::kStopped, transceiver_.direction());
+  EXPECT_EQ(RtpTransceiverDirection::kStopped,
+            *transceiver_.current_direction());
+}
+
 class RtpTransceiverTestForHeaderExtensions : public ::testing::Test {
  public:
   RtpTransceiverTestForHeaderExtensions()