Fire OnRenegotiationNeeded when changing transceiver direction

This is specified by the WebRTC specification:
https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-direction

Bug: webrtc:7600
Change-Id: If45ba0383e5040d250cd3c1c2525ff3b03b1eb4f
Reviewed-on: https://webrtc-review.googlesource.com/55880
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22139}

pc/peerconnection.cc

@@ -1157,9 +1157,11 @@ PeerConnection::AddTrackUnifiedPlan(
   auto transceiver = FindFirstTransceiverForAddedTrack(track);
   if (transceiver) {
     if (transceiver->direction() == RtpTransceiverDirection::kRecvOnly) {
-      transceiver->SetDirection(RtpTransceiverDirection::kSendRecv);
+      transceiver->internal()->set_direction(
+          RtpTransceiverDirection::kSendRecv);
     } else if (transceiver->direction() == RtpTransceiverDirection::kInactive) {
-      transceiver->SetDirection(RtpTransceiverDirection::kSendOnly);
+      transceiver->internal()->set_direction(
+          RtpTransceiverDirection::kSendOnly);
     }
     transceiver->sender()->SetTrack(track);
     transceiver->internal()->sender_internal()->set_stream_ids(stream_labels);
@@ -1172,7 +1174,7 @@ PeerConnection::AddTrackUnifiedPlan(
     auto receiver = CreateReceiver(media_type, rtc::CreateRandomUuid());
     transceiver = CreateAndAddTransceiver(sender, receiver);
     transceiver->internal()->set_created_by_addtrack(true);
-    transceiver->SetDirection(RtpTransceiverDirection::kSendRecv);
+    transceiver->internal()->set_direction(RtpTransceiverDirection::kSendRecv);
   }
   return transceiver->sender();
 }
@@ -1214,9 +1216,11 @@ RTCError PeerConnection::RemoveTrackInternal(
     }
     sender->SetTrack(nullptr);
     if (transceiver->direction() == RtpTransceiverDirection::kSendRecv) {
-      transceiver->internal()->SetDirection(RtpTransceiverDirection::kRecvOnly);
+      transceiver->internal()->set_direction(
+          RtpTransceiverDirection::kRecvOnly);
     } else if (transceiver->direction() == RtpTransceiverDirection::kSendOnly) {
-      transceiver->internal()->SetDirection(RtpTransceiverDirection::kInactive);
+      transceiver->internal()->set_direction(
+          RtpTransceiverDirection::kInactive);
     }
   } else {
     bool removed;
@@ -1390,9 +1394,17 @@ PeerConnection::CreateAndAddTransceiver(
   auto transceiver = RtpTransceiverProxyWithInternal<RtpTransceiver>::Create(
       signaling_thread(), new RtpTransceiver(sender, receiver));
   transceivers_.push_back(transceiver);
+  transceiver->internal()->SignalNegotiationNeeded.connect(
+      this, &PeerConnection::OnNegotiationNeeded);
   return transceiver;
 }
 
+void PeerConnection::OnNegotiationNeeded() {
+  RTC_DCHECK_RUN_ON(signaling_thread());
+  RTC_DCHECK(!IsClosed());
+  observer_->OnRenegotiationNeeded();
+}
+
 rtc::scoped_refptr<DtmfSenderInterface> PeerConnection::CreateDtmfSender(
     AudioTrackInterface* track) {
   TRACE_EVENT0("webrtc", "PeerConnection::CreateDtmfSender");

pc/peerconnection.h

@@ -449,6 +449,8 @@ class PeerConnection : public PeerConnectionInternal,
           transceiver,
       const SessionDescriptionInterface* sdesc) const;
 
+  void OnNegotiationNeeded();
+
   bool IsClosed() const {
     return signaling_state_ == PeerConnectionInterface::kClosed;
   }

pc/peerconnection_rtp_unittest.cc

@@ -976,6 +976,46 @@ TEST_F(PeerConnectionRtpUnifiedPlanTest,
   EXPECT_FALSE(caller->observer()->negotiation_needed());
 }
 
+// Test that OnRenegotiationNeeded is fired if SetDirection is called on an
+// active RtpTransceiver with a new direction.
+TEST_F(PeerConnectionRtpUnifiedPlanTest,
+       RenegotiationNeededAfterTransceiverSetDirection) {
+  auto caller = CreatePeerConnectionWithUnifiedPlan();
+
+  auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+
+  caller->observer()->clear_negotiation_needed();
+  transceiver->SetDirection(RtpTransceiverDirection::kInactive);
+  EXPECT_TRUE(caller->observer()->negotiation_needed());
+}
+
+// Test that OnRenegotiationNeeded is not fired if SetDirection is called on an
+// active RtpTransceiver with current direction.
+TEST_F(PeerConnectionRtpUnifiedPlanTest,
+       NoRenegotiationNeededAfterTransceiverSetSameDirection) {
+  auto caller = CreatePeerConnectionWithUnifiedPlan();
+
+  auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+
+  caller->observer()->clear_negotiation_needed();
+  transceiver->SetDirection(transceiver->direction());
+  EXPECT_FALSE(caller->observer()->negotiation_needed());
+}
+
+// Test that OnRenegotiationNeeded is not fired if SetDirection is called on a
+// stopped RtpTransceiver.
+TEST_F(PeerConnectionRtpUnifiedPlanTest,
+       NoRenegotiationNeededAfterSetDirectionOnStoppedTransceiver) {
+  auto caller = CreatePeerConnectionWithUnifiedPlan();
+
+  auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+  transceiver->Stop();
+
+  caller->observer()->clear_negotiation_needed();
+  transceiver->SetDirection(RtpTransceiverDirection::kInactive);
+  EXPECT_FALSE(caller->observer()->negotiation_needed());
+}
+
 // Test MSID signaling between Unified Plan and Plan B endpoints. There are two
 // options for this kind of signaling: media section based (a=msid) and ssrc
 // based (a=ssrc MSID). While JSEP only specifies media section MSID signaling,

pc/rtptransceiver.cc

@@ -190,8 +190,14 @@ RtpTransceiverDirection RtpTransceiver::direction() const {
 }
 
 void RtpTransceiver::SetDirection(RtpTransceiverDirection new_direction) {
-  // TODO(steveanton): This should fire OnNegotiationNeeded.
-  set_direction(new_direction);
+  if (stopped()) {
+    return;
+  }
+  if (new_direction == direction_) {
+    return;
+  }
+  direction_ = new_direction;
+  SignalNegotiationNeeded();
 }
 
 rtc::Optional<RtpTransceiverDirection> RtpTransceiver::current_direction()

pc/rtptransceiver.h

@@ -154,6 +154,10 @@ class RtpTransceiver final
     return has_ever_been_used_to_send_;
   }
 
+  // Fired when the RtpTransceiver state changes such that negotiation is now
+  // needed (e.g., in response to a direction change).
+  sigslot::signal0<> SignalNegotiationNeeded;
+
   // RtpTransceiverInterface implementation.
   cricket::MediaType media_type() const override;
   rtc::Optional<std::string> mid() const override;