From 11dc6571cb4ff3e71dee1557dfff8d9076e108d3 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 10 Aug 2020 14:41:03 +0200 Subject: [PATCH] Implement transceiver.stop() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds RtpTransceiver.StopStandard(), which behaves according to the specification at https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stop It modifies RTCPeerConnection.getTransceivers() to return only transceivers that have not been stopped. Rebase of armax' https://webrtc-review.googlesource.com/c/src/+/172762 Bug: chromium:980879 Change-Id: I7d383ee874ccc0a006fdcf280496b5d4235425ce Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180580 Reviewed-by: Kári Helgason Reviewed-by: Sami Kalliomäki Reviewed-by: Guido Urdaneta Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#31893} --- api/rtp_transceiver_interface.cc | 30 ++++ api/rtp_transceiver_interface.h | 30 +++- pc/media_session.cc | 17 +- pc/media_session_unittest.cc | 3 +- pc/peer_connection.cc | 153 +++++++++++++----- pc/peer_connection_bundle_unittest.cc | 2 +- pc/peer_connection_integrationtest.cc | 112 +++++++------ pc/peer_connection_interface_unittest.cc | 27 ++-- pc/peer_connection_jsep_unittest.cc | 134 ++++++++------- pc/peer_connection_media_unittest.cc | 8 +- pc/peer_connection_rtp_unittest.cc | 94 +++++++++-- pc/peer_connection_simulcast_unittest.cc | 6 +- pc/rtp_media_utils.cc | 1 + pc/rtp_sender.cc | 9 ++ pc/rtp_sender.h | 5 + pc/rtp_transceiver.cc | 119 ++++++++++++-- pc/rtp_transceiver.h | 20 ++- pc/rtp_transceiver_unittest.cc | 4 +- pc/test/mock_rtp_sender_internal.h | 28 ++-- pc/webrtc_sdp.cc | 4 +- .../api/org/webrtc/RtpTransceiver.java | 32 +++- sdk/android/src/jni/pc/rtp_transceiver.cc | 13 +- .../api/peerconnection/RTCRtpTransceiver.h | 2 +- .../api/peerconnection/RTCRtpTransceiver.mm | 4 +- 24 files changed, 604 insertions(+), 253 deletions(-) diff --git a/api/rtp_transceiver_interface.cc b/api/rtp_transceiver_interface.cc index e795e51dfb..1dc0fcc79e 100644 --- a/api/rtp_transceiver_interface.cc +++ b/api/rtp_transceiver_interface.cc @@ -25,6 +25,23 @@ RtpTransceiverInterface::fired_direction() const { return absl::nullopt; } +bool RtpTransceiverInterface::stopping() const { + return false; +} + +void RtpTransceiverInterface::Stop() { + StopInternal(); +} + +RTCError RtpTransceiverInterface::StopStandard() { + RTC_NOTREACHED() << "DEBUG: RtpTransceiverInterface::StopStandard called"; + return RTCError::OK(); +} + +void RtpTransceiverInterface::StopInternal() { + RTC_NOTREACHED() << "DEBUG: RtpTransceiverInterface::StopInternal called"; +} + RTCError RtpTransceiverInterface::SetCodecPreferences( rtc::ArrayView) { RTC_NOTREACHED() << "Not implemented"; @@ -47,4 +64,17 @@ webrtc::RTCError RtpTransceiverInterface::SetOfferedRtpHeaderExtensions( return webrtc::RTCError(webrtc::RTCErrorType::UNSUPPORTED_OPERATION); } +// TODO(bugs.webrtc.org/11839) Remove default implementations when clients +// are updated. +void RtpTransceiverInterface::SetDirection( + RtpTransceiverDirection new_direction) { + SetDirectionWithError(new_direction); +} + +RTCError RtpTransceiverInterface::SetDirectionWithError( + RtpTransceiverDirection new_direction) { + RTC_NOTREACHED() << "Default implementation called"; + return RTCError::OK(); +} + } // namespace webrtc diff --git a/api/rtp_transceiver_interface.h b/api/rtp_transceiver_interface.h index 13277d9a50..cdda34b193 100644 --- a/api/rtp_transceiver_interface.h +++ b/api/rtp_transceiver_interface.h @@ -89,6 +89,16 @@ class RTC_EXPORT RtpTransceiverInterface : public rtc::RefCountInterface { // https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stopped virtual bool stopped() const = 0; + // The stopping attribute indicates that the user has indicated that the + // sender of this transceiver will stop sending, and that the receiver will + // no longer receive. It is always true if stopped() is true. + // If stopping() is true and stopped() is false, it means that the + // transceiver's stop() method has been called, but the negotiation with + // the other end for shutting down the transceiver is not yet done. + // https://w3c.github.io/webrtc-pc/#dfn-stopping-0 + // TODO(hta): Remove default implementation. + virtual bool stopping() const; + // The direction attribute indicates the preferred direction of this // transceiver, which will be used in calls to CreateOffer and CreateAnswer. // https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-direction @@ -99,7 +109,10 @@ class RTC_EXPORT RtpTransceiverInterface : public rtc::RefCountInterface { // CreateOffer and CreateAnswer mark the corresponding media descriptions as // sendrecv, sendonly, recvonly, or inactive. // https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-direction - virtual void SetDirection(RtpTransceiverDirection new_direction) = 0; + // TODO(hta): Deprecate SetDirection without error and rename + // SetDirectionWithError to SetDirection, remove default implementations. + virtual void SetDirection(RtpTransceiverDirection new_direction); + virtual RTCError SetDirectionWithError(RtpTransceiverDirection new_direction); // The current_direction attribute indicates the current direction negotiated // for this transceiver. If this transceiver has never been represented in an @@ -114,10 +127,19 @@ class RTC_EXPORT RtpTransceiverInterface : public rtc::RefCountInterface { // Exposed in the public interface for use by Chromium. virtual absl::optional fired_direction() const; - // The Stop method irreversibly stops the RtpTransceiver. The sender of this - // transceiver will no longer send, the receiver will no longer receive. + // Initiates a stop of the transceiver. + // The stop is complete when stopped() returns true. + // A stopped transceiver can be reused for a different track. // https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stop - virtual void Stop() = 0; + // TODO(hta): Rename to Stop() when users of the non-standard Stop() are + // updated. + virtual RTCError StopStandard(); + + // Stops a transceiver immediately, without waiting for signalling. + // This is an internal function, and is exposed for historical reasons. + // https://w3c.github.io/webrtc-pc/#dfn-stop-the-rtcrtptransceiver + virtual void StopInternal(); + RTC_DEPRECATED virtual void Stop(); // The SetCodecPreferences method overrides the default codec preferences used // by WebRTC for this transceiver. diff --git a/pc/media_session.cc b/pc/media_session.cc index 69ddb0c895..0cff84d79b 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1507,7 +1507,6 @@ std::unique_ptr MediaSessionDescriptionFactory::CreateOffer( RtpDataCodecs offer_rtp_data_codecs; GetCodecsForOffer(current_active_contents, &offer_audio_codecs, &offer_video_codecs, &offer_rtp_data_codecs); - if (!session_options.vad_enabled) { // If application doesn't want CN codecs in offer. StripCNCodecs(&offer_audio_codecs); @@ -1796,15 +1795,13 @@ const AudioCodecs& MediaSessionDescriptionFactory::GetAudioCodecsForOffer( switch (direction) { // If stream is inactive - generate list as if sendrecv. case RtpTransceiverDirection::kSendRecv: + case RtpTransceiverDirection::kStopped: case RtpTransceiverDirection::kInactive: return audio_sendrecv_codecs_; case RtpTransceiverDirection::kSendOnly: return audio_send_codecs_; case RtpTransceiverDirection::kRecvOnly: return audio_recv_codecs_; - case RtpTransceiverDirection::kStopped: - RTC_NOTREACHED(); - return audio_sendrecv_codecs_; } } @@ -1815,6 +1812,7 @@ const AudioCodecs& MediaSessionDescriptionFactory::GetAudioCodecsForAnswer( // For inactive and sendrecv answers, generate lists as if we were to accept // the offer's direction. See RFC 3264 Section 6.1. case RtpTransceiverDirection::kSendRecv: + case RtpTransceiverDirection::kStopped: case RtpTransceiverDirection::kInactive: return GetAudioCodecsForOffer( webrtc::RtpTransceiverDirectionReversed(offer)); @@ -1822,9 +1820,6 @@ const AudioCodecs& MediaSessionDescriptionFactory::GetAudioCodecsForAnswer( return audio_send_codecs_; case RtpTransceiverDirection::kRecvOnly: return audio_recv_codecs_; - case RtpTransceiverDirection::kStopped: - RTC_NOTREACHED(); - return audio_sendrecv_codecs_; } } @@ -1833,15 +1828,13 @@ const VideoCodecs& MediaSessionDescriptionFactory::GetVideoCodecsForOffer( switch (direction) { // If stream is inactive - generate list as if sendrecv. case RtpTransceiverDirection::kSendRecv: + case RtpTransceiverDirection::kStopped: case RtpTransceiverDirection::kInactive: return video_sendrecv_codecs_; case RtpTransceiverDirection::kSendOnly: return video_send_codecs_; case RtpTransceiverDirection::kRecvOnly: return video_recv_codecs_; - case RtpTransceiverDirection::kStopped: - RTC_NOTREACHED(); - return video_sendrecv_codecs_; } } @@ -1852,6 +1845,7 @@ const VideoCodecs& MediaSessionDescriptionFactory::GetVideoCodecsForAnswer( // For inactive and sendrecv answers, generate lists as if we were to accept // the offer's direction. See RFC 3264 Section 6.1. case RtpTransceiverDirection::kSendRecv: + case RtpTransceiverDirection::kStopped: case RtpTransceiverDirection::kInactive: return GetVideoCodecsForOffer( webrtc::RtpTransceiverDirectionReversed(offer)); @@ -1859,9 +1853,6 @@ const VideoCodecs& MediaSessionDescriptionFactory::GetVideoCodecsForAnswer( return video_send_codecs_; case RtpTransceiverDirection::kRecvOnly: return video_recv_codecs_; - case RtpTransceiverDirection::kStopped: - RTC_NOTREACHED(); - return video_sendrecv_codecs_; } } diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index ac949fb630..e1ff313941 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -4863,7 +4863,8 @@ void TestAudioCodecsAnswer(RtpTransceiverDirection offer_direction, kResultSendrecv_SendrecvCodecs); } break; - default: + case RtpTransceiverDirection::kStopped: + // This does not happen in any current test. RTC_NOTREACHED(); } diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 982e42a2d6..541f04e5fc 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1048,7 +1048,7 @@ PeerConnection::~PeerConnection() { // AudioRtpSender has a reference to the StatsCollector it will update when // stopping. for (const auto& transceiver : transceivers_) { - transceiver->Stop(); + transceiver->StopInternal(); } stats_.reset(nullptr); @@ -1535,6 +1535,11 @@ PeerConnection::AddTrackUnifiedPlan( RTC_LOG(LS_INFO) << "Reusing an existing " << cricket::MediaTypeToString(transceiver->media_type()) << " transceiver for AddTrack."; + if (transceiver->stopping()) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, + "The existing transceiver is stopping."); + } + if (transceiver->direction() == RtpTransceiverDirection::kRecvOnly) { transceiver->internal()->set_direction( RtpTransceiverDirection::kSendRecv); @@ -1933,6 +1938,9 @@ PeerConnection::GetSendersInternal() const { std::vector>> all_senders; for (const auto& transceiver : transceivers_) { + if (IsUnifiedPlan() && transceiver->internal()->stopped()) + continue; + auto senders = transceiver->internal()->senders(); all_senders.insert(all_senders.end(), senders.begin(), senders.end()); } @@ -1956,6 +1964,9 @@ PeerConnection::GetReceiversInternal() const { rtc::scoped_refptr>> all_receivers; for (const auto& transceiver : transceivers_) { + if (IsUnifiedPlan() && transceiver->internal()->stopped()) + continue; + auto receivers = transceiver->internal()->receivers(); all_receivers.insert(all_receivers.end(), receivers.begin(), receivers.end()); @@ -1970,7 +1981,13 @@ PeerConnection::GetTransceivers() const { << "GetTransceivers is only supported with Unified Plan SdpSemantics."; std::vector> all_transceivers; for (const auto& transceiver : transceivers_) { - all_transceivers.push_back(transceiver); + // Temporary fix: Do not show stopped transceivers. + // The long term fix is to remove them from transceivers_, but this + // turns out to cause issues with audio channel lifetimes. + // TODO(https://crbug.com/webrtc/11840): Fix issue. + if (!transceiver->stopped()) { + all_transceivers.push_back(transceiver); + } } return all_transceivers; } @@ -2581,6 +2598,35 @@ void PeerConnection::DoSetLocalDescription( RTC_DCHECK(local_description()); if (local_description()->GetType() == SdpType::kAnswer) { + // 3.2.10.1: For each transceiver in the connection's set of transceivers + // run the following steps: + if (IsUnifiedPlan()) { + for (auto it = transceivers_.begin(); it != transceivers_.end();) { + const auto& transceiver = *it; + // 3.2.10.1.1: If transceiver is stopped, associated with an m= section + // and the associated m= section is rejected in + // connection.[[CurrentLocalDescription]] or + // connection.[[CurrentRemoteDescription]], remove the + // transceiver from the connection's set of transceivers. + if (transceiver->stopped()) { + const ContentInfo* content = + FindMediaSectionForTransceiver(transceiver, local_description()); + + if (content && content->rejected) { + RTC_LOG(LS_INFO) << "Dissociating transceiver" + << " since the media section is being recycled."; + (*it)->internal()->set_mid(absl::nullopt); + (*it)->internal()->set_mline_index(absl::nullopt); + it = transceivers_.erase(it); + } else { + ++it; + } + } else { + ++it; + } + } + } + // TODO(deadbeef): We already had to hop to the network thread for // MaybeStartGathering... network_thread()->Invoke( @@ -2670,6 +2716,10 @@ RTCError PeerConnection::ApplyLocalDescription( std::vector> remove_list; std::vector> removed_streams; for (const auto& transceiver : transceivers_) { + if (transceiver->stopped()) { + continue; + } + // 2.2.7.1.1.(6-9): Set sender and receiver's transport slots. // Note that code paths that don't set MID won't be able to use // information about DTLS transports. @@ -2755,6 +2805,9 @@ RTCError PeerConnection::ApplyLocalDescription( if (IsUnifiedPlan()) { for (const auto& transceiver : transceivers_) { + if (transceiver->stopped()) { + continue; + } const ContentInfo* content = FindMediaSectionForTransceiver(transceiver, local_description()); if (!content) { @@ -3263,7 +3316,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->Stop(); + transceiver->StopInternal(); } if (!content->rejected && RtpTransceiverDirectionHasRecv(local_direction)) { @@ -4339,7 +4392,9 @@ void PeerConnection::Close() { NoteUsageEvent(UsageEvent::CLOSE_CALLED); for (const auto& transceiver : transceivers_) { - transceiver->Stop(); + transceiver->internal()->SetPeerConnectionClosed(); + if (!transceiver->stopped()) + transceiver->StopInternal(); } // Ensure that all asynchronous stats requests are completed before destroying @@ -4905,10 +4960,15 @@ static cricket::MediaDescriptionOptions GetMediaDescriptionOptionsForTransceiver( rtc::scoped_refptr> transceiver, - const std::string& mid) { + const std::string& mid, + bool is_create_offer) { + // NOTE: a stopping transceiver should be treated as a stopped one in + // createOffer as specified in + // https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-createoffer. + bool stopped = + is_create_offer ? transceiver->stopping() : transceiver->stopped(); cricket::MediaDescriptionOptions media_description_options( - transceiver->media_type(), mid, transceiver->direction(), - transceiver->stopped()); + transceiver->media_type(), mid, transceiver->direction(), stopped); media_description_options.codec_preferences = transceiver->codec_preferences(); media_description_options.header_extensions = @@ -4918,9 +4978,8 @@ GetMediaDescriptionOptionsForTransceiver( // sendrecv. // 2. If the MSID is included, then it must be included in any subsequent // offer/answer exactly the same until the RtpTransceiver is stopped. - if (transceiver->stopped() || - (!RtpTransceiverDirectionHasSend(transceiver->direction()) && - !transceiver->internal()->has_ever_been_used_to_send())) { + if (stopped || (!RtpTransceiverDirectionHasSend(transceiver->direction()) && + !transceiver->internal()->has_ever_been_used_to_send())) { return media_description_options; } @@ -5014,25 +5073,39 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( : remote_content->media_description()->type()); if (media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO) { - auto transceiver = GetAssociatedTransceiver(mid); - RTC_CHECK(transceiver); // A media section is considered eligible for recycling if it is marked as // rejected in either the current local or current remote description. - if (had_been_rejected && transceiver->stopped()) { + auto transceiver = GetAssociatedTransceiver(mid); + if (!transceiver) { + // No associated transceiver. The media section has been stopped. + recycleable_mline_indices.push(i); session_options->media_description_options.push_back( - cricket::MediaDescriptionOptions(transceiver->media_type(), mid, + cricket::MediaDescriptionOptions(media_type, mid, RtpTransceiverDirection::kInactive, /*stopped=*/true)); - recycleable_mline_indices.push(i); } else { - session_options->media_description_options.push_back( - GetMediaDescriptionOptionsForTransceiver(transceiver, mid)); - // CreateOffer shouldn't really cause any state changes in - // PeerConnection, but we need a way to match new transceivers to new - // media sections in SetLocalDescription and JSEP specifies this is done - // by recording the index of the media section generated for the - // transceiver in the offer. - transceiver->internal()->set_mline_index(i); + // NOTE: a stopping transceiver should be treated as a stopped one in + // createOffer as specified in + // https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-createoffer. + if (had_been_rejected && transceiver->stopping()) { + session_options->media_description_options.push_back( + cricket::MediaDescriptionOptions( + transceiver->media_type(), mid, + RtpTransceiverDirection::kInactive, + /*stopped=*/true)); + recycleable_mline_indices.push(i); + } else { + session_options->media_description_options.push_back( + GetMediaDescriptionOptionsForTransceiver( + transceiver, mid, + /*is_create_offer=*/true)); + // CreateOffer shouldn't really cause any state changes in + // PeerConnection, but we need a way to match new transceivers to new + // media sections in SetLocalDescription and JSEP specifies this is + // done by recording the index of the media section generated for the + // transceiver in the offer. + transceiver->internal()->set_mline_index(i); + } } } else { RTC_CHECK_EQ(cricket::MEDIA_TYPE_DATA, media_type); @@ -5057,7 +5130,7 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( // otherwise append to the end of the offer. New media sections should be // added in the order they were added to the PeerConnection. for (const auto& transceiver : transceivers_) { - if (transceiver->mid() || transceiver->stopped()) { + if (transceiver->mid() || transceiver->stopping()) { continue; } size_t mline_index; @@ -5065,13 +5138,13 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( mline_index = recycleable_mline_indices.front(); recycleable_mline_indices.pop(); session_options->media_description_options[mline_index] = - GetMediaDescriptionOptionsForTransceiver(transceiver, - mid_generator_()); + GetMediaDescriptionOptionsForTransceiver( + transceiver, mid_generator_(), /*is_create_offer=*/true); } else { mline_index = session_options->media_description_options.size(); session_options->media_description_options.push_back( - GetMediaDescriptionOptionsForTransceiver(transceiver, - mid_generator_())); + GetMediaDescriptionOptionsForTransceiver( + transceiver, mid_generator_(), /*is_create_offer=*/true)); } // See comment above for why CreateOffer changes the transceiver's state. transceiver->internal()->set_mline_index(mline_index); @@ -5182,7 +5255,8 @@ void PeerConnection::GetOptionsForUnifiedPlanAnswer( auto transceiver = GetAssociatedTransceiver(content.name); RTC_CHECK(transceiver); session_options->media_description_options.push_back( - GetMediaDescriptionOptionsForTransceiver(transceiver, content.name)); + GetMediaDescriptionOptionsForTransceiver(transceiver, content.name, + /*is_create_offer=*/false)); } else { RTC_CHECK_EQ(cricket::MEDIA_TYPE_DATA, media_type); // Reject all data sections if data channels are disabled. @@ -7273,7 +7347,8 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { // 1. If any implementation-specific negotiation is required, as described at // the start of this section, return true. - // 2. If connection's [[RestartIce]] internal slot is true, return true. + // 2. If connection.[[LocalIceCredentialsToReplace]] is not empty, return + // true. if (local_ice_credentials_to_replace_->HasIceCredentials()) { return true; } @@ -7299,11 +7374,12 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { const ContentInfo* current_remote_msection = FindTransceiverMSection( transceiver.get(), current_remote_description()); - // 5.3 If transceiver is stopped and is associated with an m= section, + // 5.4 If transceiver is stopped and is associated with an m= section, // but the associated m= section is not yet rejected in // connection.[[CurrentLocalDescription]] or // connection.[[CurrentRemoteDescription]], return true. if (transceiver->stopped()) { + RTC_DCHECK(transceiver->stopping()); if (current_local_msection && !current_local_msection->rejected && ((current_remote_msection && !current_remote_msection->rejected) || !current_remote_msection)) { @@ -7312,17 +7388,22 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { continue; } - // 5.1 If transceiver isn't stopped and isn't yet associated with an m= + // 5.1 If transceiver.[[Stopping]] is true and transceiver.[[Stopped]] is + // false, return true. + if (transceiver->stopping() && !transceiver->stopped()) + return true; + + // 5.2 If transceiver isn't stopped and isn't yet associated with an m= // section in description, return true. if (!current_local_msection) return true; const MediaContentDescription* current_local_media_description = current_local_msection->media_description(); - // 5.2 If transceiver isn't stopped and is associated with an m= section + // 5.3 If transceiver isn't stopped and is associated with an m= section // in description then perform the following checks: - // 5.2.1 If transceiver.[[Direction]] is "sendrecv" or "sendonly", and the + // 5.3.1 If transceiver.[[Direction]] is "sendrecv" or "sendonly", and the // associated m= section in description either doesn't contain a single // "a=msid" line, or the number of MSIDs from the "a=msid" lines in this // m= section, or the MSID values themselves, differ from what is in @@ -7348,7 +7429,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { return true; } - // 5.2.2 If description is of type "offer", and the direction of the + // 5.3.2 If description is of type "offer", and the direction of the // associated m= section in neither connection.[[CurrentLocalDescription]] // nor connection.[[CurrentRemoteDescription]] matches // transceiver.[[Direction]], return true. @@ -7370,7 +7451,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { } } - // 5.2.3 If description is of type "answer", and the direction of the + // 5.3.3 If description is of type "answer", and the direction of the // associated m= section in the description does not match // transceiver.[[Direction]] intersected with the offered direction (as // described in [JSEP] (section 5.3.1.)), return true. diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index 543c9be81a..c544db396f 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/pc/peer_connection_bundle_unittest.cc @@ -873,7 +873,7 @@ TEST_F(PeerConnectionBundleTestUnifiedPlan, // Stop all transceivers, causing all m= sections to be rejected. for (const auto& transceiver : callee->pc()->GetTransceivers()) { - transceiver->Stop(); + transceiver->StopInternal(); } EXPECT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index afb5f2ba75..dd24163f3e 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -799,9 +799,7 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, EXPECT_TRUE(desc->ToString(&sdp)); RTC_LOG(LS_INFO) << debug_name_ << ": local SDP contents=\n" << sdp; pc()->SetLocalDescription(observer, desc.release()); - if (sdp_semantics_ == SdpSemantics::kUnifiedPlan) { - RemoveUnusedVideoRenderers(); - } + RemoveUnusedVideoRenderers(); // As mentioned above, we need to send the message immediately after // SetLocalDescription. SendSdpMessage(type, sdp); @@ -814,9 +812,7 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, new rtc::RefCountedObject()); RTC_LOG(LS_INFO) << debug_name_ << ": SetRemoteDescription"; pc()->SetRemoteDescription(observer, desc.release()); - if (sdp_semantics_ == SdpSemantics::kUnifiedPlan) { - RemoveUnusedVideoRenderers(); - } + RemoveUnusedVideoRenderers(); EXPECT_TRUE_WAIT(observer->called(), kDefaultTimeout); return observer->result(); } @@ -824,29 +820,26 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, // This is a work around to remove unused fake_video_renderers from // transceivers that have either stopped or are no longer receiving. void RemoveUnusedVideoRenderers() { + if (sdp_semantics_ != SdpSemantics::kUnifiedPlan) { + return; + } auto transceivers = pc()->GetTransceivers(); + std::set active_renderers; for (auto& transceiver : transceivers) { - if (transceiver->receiver()->media_type() != cricket::MEDIA_TYPE_VIDEO) { - continue; + // Note - we don't check for direction here. This function is called + // before direction is set, and in that case, we should not remove + // the renderer. + if (transceiver->receiver()->media_type() == cricket::MEDIA_TYPE_VIDEO) { + active_renderers.insert(transceiver->receiver()->track()->id()); } - // Remove fake video renderers from any stopped transceivers. - if (transceiver->stopped()) { - auto it = - fake_video_renderers_.find(transceiver->receiver()->track()->id()); - if (it != fake_video_renderers_.end()) { - fake_video_renderers_.erase(it); - } - } - // Remove fake video renderers from any transceivers that are no longer - // receiving. - if ((transceiver->current_direction() && - !webrtc::RtpTransceiverDirectionHasRecv( - *transceiver->current_direction()))) { - auto it = - fake_video_renderers_.find(transceiver->receiver()->track()->id()); - if (it != fake_video_renderers_.end()) { - fake_video_renderers_.erase(it); - } + } + for (auto it = fake_video_renderers_.begin(); + it != fake_video_renderers_.end();) { + // Remove fake video renderers belonging to any non-active transceivers. + if (!active_renderers.count(it->first)) { + it = fake_video_renderers_.erase(it); + } else { + it++; } } } @@ -936,8 +929,11 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, rtc::scoped_refptr receiver) override { if (receiver->media_type() == cricket::MEDIA_TYPE_VIDEO) { auto it = fake_video_renderers_.find(receiver->track()->id()); - RTC_DCHECK(it != fake_video_renderers_.end()); - fake_video_renderers_.erase(it); + if (it != fake_video_renderers_.end()) { + fake_video_renderers_.erase(it); + } else { + RTC_LOG(LS_ERROR) << "OnRemoveTrack called for non-active renderer"; + } } } void OnRenegotiationNeeded() override {} @@ -1561,6 +1557,9 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { // |media_expectations|. Returns false if any of the expectations were // not met. bool ExpectNewFrames(const MediaExpectations& media_expectations) { + // Make sure there are no bogus tracks confusing the issue. + caller()->RemoveUnusedVideoRenderers(); + callee()->RemoveUnusedVideoRenderers(); // First initialize the expected frame counts based upon the current // frame count. int total_caller_audio_frames_expected = caller()->audio_frames_received(); @@ -2237,7 +2236,9 @@ TEST_P(PeerConnectionIntegrationTest, AudioToVideoUpgrade) { callee()->SetOfferAnswerOptions(options); } else { callee()->SetRemoteOfferHandler([this] { - callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); + callee() + ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) + ->StopInternal(); }); } // Do offer/answer and make sure audio is still received end-to-end. @@ -2269,11 +2270,12 @@ TEST_P(PeerConnectionIntegrationTest, AudioToVideoUpgrade) { // The caller creates a new transceiver to receive video on when receiving // the offer, but by default it is send only. auto transceivers = caller()->pc()->GetTransceivers(); - ASSERT_EQ(3U, transceivers.size()); + ASSERT_EQ(2U, transceivers.size()); ASSERT_EQ(cricket::MEDIA_TYPE_VIDEO, - transceivers[2]->receiver()->media_type()); - transceivers[2]->sender()->SetTrack(caller()->CreateLocalVideoTrack()); - transceivers[2]->SetDirection(RtpTransceiverDirection::kSendRecv); + transceivers[1]->receiver()->media_type()); + transceivers[1]->sender()->SetTrack(caller()->CreateLocalVideoTrack()); + transceivers[1]->SetDirectionWithError( + RtpTransceiverDirection::kSendRecv); }); } callee()->CreateAndSetAndSignalOffer(); @@ -2485,7 +2487,9 @@ TEST_P(PeerConnectionIntegrationTest, AnswererRejectsAudioSection) { // Stopping the audio RtpTransceiver will cause the media section to be // rejected in the answer. callee()->SetRemoteOfferHandler([this] { - callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO)->Stop(); + callee() + ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO) + ->StopInternal(); }); } callee()->AddTrack(callee()->CreateLocalVideoTrack()); @@ -2504,10 +2508,10 @@ TEST_P(PeerConnectionIntegrationTest, AnswererRejectsAudioSection) { ASSERT_NE(nullptr, callee_audio_content); EXPECT_TRUE(callee_audio_content->rejected); if (sdp_semantics_ == SdpSemantics::kUnifiedPlan) { - // The caller's transceiver should have stopped after receiving the answer. - EXPECT_TRUE(caller() - ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO) - ->stopped()); + // The caller's transceiver should have stopped after receiving the answer, + // and thus no longer listed in transceivers. + EXPECT_EQ(nullptr, + caller()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO)); } } @@ -2527,7 +2531,9 @@ TEST_P(PeerConnectionIntegrationTest, AnswererRejectsVideoSection) { // Stopping the video RtpTransceiver will cause the media section to be // rejected in the answer. callee()->SetRemoteOfferHandler([this] { - callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); + callee() + ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) + ->StopInternal(); }); } callee()->AddTrack(callee()->CreateLocalAudioTrack()); @@ -2546,10 +2552,10 @@ TEST_P(PeerConnectionIntegrationTest, AnswererRejectsVideoSection) { ASSERT_NE(nullptr, callee_video_content); EXPECT_TRUE(callee_video_content->rejected); if (sdp_semantics_ == SdpSemantics::kUnifiedPlan) { - // The caller's transceiver should have stopped after receiving the answer. - EXPECT_TRUE(caller() - ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) - ->stopped()); + // The caller's transceiver should have stopped after receiving the answer, + // and thus is no longer present. + EXPECT_EQ(nullptr, + caller()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)); } } @@ -2573,7 +2579,7 @@ TEST_P(PeerConnectionIntegrationTest, AnswererRejectsAudioAndVideoSections) { callee()->SetRemoteOfferHandler([this] { // Stopping all transceivers will cause all media sections to be rejected. for (const auto& transceiver : callee()->pc()->GetTransceivers()) { - transceiver->Stop(); + transceiver->StopInternal(); } }); } @@ -2620,7 +2626,9 @@ TEST_P(PeerConnectionIntegrationTest, VideoRejectedInSubsequentOffer) { } }); } else { - caller()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); + caller() + ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) + ->StopInternal(); } caller()->CreateAndSetAndSignalOffer(); ASSERT_TRUE_WAIT(SignalingStateStable(), kMaxWaitForActivationMs); @@ -2735,7 +2743,7 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); // Add receive direction. - video_sender->SetDirection(RtpTransceiverDirection::kSendRecv); + video_sender->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); rtc::scoped_refptr callee_track = callee()->CreateLocalVideoTrack(); @@ -4345,7 +4353,9 @@ TEST_P(PeerConnectionIntegrationTest, callee()->SetOfferAnswerOptions(options); } else { callee()->SetRemoteOfferHandler([this] { - callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); + callee() + ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) + ->StopInternal(); }); } caller()->CreateAndSetAndSignalOffer(); @@ -4366,7 +4376,7 @@ TEST_P(PeerConnectionIntegrationTest, // The caller's transceiver is stopped, so we need to add another track. auto caller_transceiver = caller()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO); - EXPECT_TRUE(caller_transceiver->stopped()); + EXPECT_EQ(nullptr, caller_transceiver.get()); caller()->AddVideoTrack(); } callee()->AddVideoTrack(); @@ -4429,9 +4439,9 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, auto caller_video_sender = video_result.MoveValue()->sender(); callee()->SetRemoteOfferHandler([this] { ASSERT_EQ(2u, callee()->pc()->GetTransceivers().size()); - callee()->pc()->GetTransceivers()[0]->SetDirection( + callee()->pc()->GetTransceivers()[0]->SetDirectionWithError( RtpTransceiverDirection::kSendRecv); - callee()->pc()->GetTransceivers()[1]->SetDirection( + callee()->pc()->GetTransceivers()[1]->SetDirectionWithError( RtpTransceiverDirection::kSendRecv); }); caller()->CreateAndSetAndSignalOffer(); @@ -5552,7 +5562,7 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, ASSERT_TRUE(ExpectNewFrames(media_expectations)); } - audio_transceiver->Stop(); + audio_transceiver->StopInternal(); caller()->pc()->AddTransceiver(caller()->CreateLocalVideoTrack()); caller()->CreateAndSetAndSignalOffer(); diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 901e5c572c..046d1c93bc 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -2668,23 +2668,24 @@ TEST_P(PeerConnectionInterfaceTest, CloseAndTestStreamsAndStates) { EXPECT_EQ(1u, pc_->local_streams()->count()); EXPECT_EQ(1u, pc_->remote_streams()->count()); } else { - // Verify that the RtpTransceivers are still present but all stopped. - EXPECT_EQ(2u, pc_->GetTransceivers().size()); - for (const auto& transceiver : pc_->GetTransceivers()) { - EXPECT_TRUE(transceiver->stopped()); - } + // Verify that the RtpTransceivers are no longer returned. + EXPECT_EQ(0u, pc_->GetTransceivers().size()); } auto audio_receiver = GetFirstReceiverOfType(cricket::MEDIA_TYPE_AUDIO); - ASSERT_TRUE(audio_receiver); auto video_receiver = GetFirstReceiverOfType(cricket::MEDIA_TYPE_VIDEO); - ASSERT_TRUE(video_receiver); - - // Track state may be updated asynchronously. - EXPECT_EQ_WAIT(MediaStreamTrackInterface::kEnded, - audio_receiver->track()->state(), kTimeout); - EXPECT_EQ_WAIT(MediaStreamTrackInterface::kEnded, - video_receiver->track()->state(), kTimeout); + if (sdp_semantics_ == SdpSemantics::kPlanB) { + ASSERT_TRUE(audio_receiver); + ASSERT_TRUE(video_receiver); + // Track state may be updated asynchronously. + EXPECT_EQ_WAIT(MediaStreamTrackInterface::kEnded, + audio_receiver->track()->state(), kTimeout); + EXPECT_EQ_WAIT(MediaStreamTrackInterface::kEnded, + video_receiver->track()->state(), kTimeout); + } else { + ASSERT_FALSE(audio_receiver); + ASSERT_FALSE(video_receiver); + } } // Test that PeerConnection methods fails gracefully after diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index 0b2f375dde..c4b7de10f7 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -212,7 +212,7 @@ TEST_F(PeerConnectionJsepTest, StoppedTransceiverHasNoMediaSectionInInitialOffer) { auto caller = CreatePeerConnection(); auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - transceiver->Stop(); + transceiver->StopInternal(); auto offer = caller->CreateOffer(); EXPECT_EQ(0u, offer->description()->contents().size()); @@ -300,7 +300,7 @@ TEST_F(PeerConnectionJsepTest, auto caller = CreatePeerConnection(); caller->AddAudioTrack("a"); auto caller_audio = caller->pc()->GetTransceivers()[0]; - caller_audio->SetDirection(RtpTransceiverDirection::kSendOnly); + caller_audio->SetDirectionWithError(RtpTransceiverDirection::kSendOnly); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); @@ -358,16 +358,14 @@ TEST_F(PeerConnectionJsepTest, SetRemoteOfferDoesNotReuseStoppedTransceiver) { caller->AddAudioTrack("a"); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); - callee->pc()->GetTransceivers()[0]->Stop(); + callee->pc()->GetTransceivers()[0]->StopInternal(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto transceivers = callee->pc()->GetTransceivers(); - ASSERT_EQ(2u, transceivers.size()); - EXPECT_EQ(absl::nullopt, transceivers[0]->mid()); - EXPECT_TRUE(transceivers[0]->stopped()); - EXPECT_EQ(caller->pc()->GetTransceivers()[0]->mid(), transceivers[1]->mid()); - EXPECT_FALSE(transceivers[1]->stopped()); + ASSERT_EQ(1u, transceivers.size()); + EXPECT_EQ(caller->pc()->GetTransceivers()[0]->mid(), transceivers[0]->mid()); + EXPECT_FALSE(transceivers[0]->stopped()); } // Test that audio and video transceivers created on the remote side with @@ -432,7 +430,7 @@ TEST_F(PeerConnectionJsepTest, CreateAnswerRejectsStoppedTransceiver) { ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - callee->pc()->GetTransceivers()[0]->Stop(); + callee->pc()->GetTransceivers()[0]->StopInternal(); auto answer = callee->CreateAnswer(); auto contents = answer->description()->contents(); @@ -469,7 +467,7 @@ TEST_F(PeerConnectionJsepTest, CreateAnswerNegotiatesDirection) { TEST_F(PeerConnectionJsepTest, SetLocalAnswerUpdatesCurrentDirection) { auto caller = CreatePeerConnection(); auto caller_audio = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - caller_audio->SetDirection(RtpTransceiverDirection::kRecvOnly); + caller_audio->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); @@ -494,7 +492,7 @@ TEST_F(PeerConnectionJsepTest, SetRemoteAnswerUpdatesCurrentDirection) { auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); auto callee_audio = callee->pc()->GetTransceivers()[0]; - callee_audio->SetDirection(RtpTransceiverDirection::kSendOnly); + callee_audio->SetDirectionWithError(RtpTransceiverDirection::kSendOnly); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_TRUE( @@ -518,7 +516,7 @@ TEST_F(PeerConnectionJsepTest, SettingTransceiverInactiveDoesNotStopIt) { caller->AddAudioTrack("a"); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); - callee->pc()->GetTransceivers()[0]->SetDirection( + callee->pc()->GetTransceivers()[0]->SetDirectionWithError( RtpTransceiverDirection::kInactive); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); @@ -543,7 +541,7 @@ TEST_F(PeerConnectionJsepTest, caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); ASSERT_TRUE(transceiver->mid()); - transceiver->Stop(); + transceiver->StopInternal(); auto reoffer = caller->CreateOffer(); auto contents = reoffer->description()->contents(); @@ -564,13 +562,12 @@ TEST_F(PeerConnectionJsepTest, ASSERT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); - transceiver->Stop(); + transceiver->StopInternal(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto transceivers = callee->pc()->GetTransceivers(); - EXPECT_TRUE(transceivers[0]->stopped()); - EXPECT_TRUE(transceivers[0]->mid()); + EXPECT_EQ(0u, transceivers.size()); } // Test that CreateOffer will only generate a recycled media section if the @@ -586,7 +583,7 @@ TEST_F(PeerConnectionJsepTest, caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); auto second_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - first_transceiver->Stop(); + first_transceiver->StopInternal(); auto reoffer = caller->CreateOffer(); auto contents = reoffer->description()->contents(); @@ -605,14 +602,17 @@ TEST_F(PeerConnectionJsepTest, auto callee = CreatePeerConnection(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - callee->pc()->GetTransceivers()[0]->Stop(); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + callee->pc()->GetTransceivers()[0]->StopInternal(); + ASSERT_EQ(0u, callee->pc()->GetTransceivers().size()); ASSERT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); EXPECT_TRUE(first_transceiver->stopped()); - // First transceivers aren't dissociated yet. + // First transceivers aren't dissociated yet on caller side. ASSERT_NE(absl::nullopt, first_transceiver->mid()); std::string first_mid = *first_transceiver->mid(); - EXPECT_EQ(first_mid, callee->pc()->GetTransceivers()[0]->mid()); + // They are disassociated on callee side. + ASSERT_EQ(0u, callee->pc()->GetTransceivers().size()); // New offer exchange with new transceivers that recycles the m section // correctly. @@ -630,10 +630,11 @@ TEST_F(PeerConnectionJsepTest, ASSERT_TRUE( caller->SetLocalDescription(CloneSessionDescription(offer.get()))); EXPECT_EQ(absl::nullopt, first_transceiver->mid()); - EXPECT_EQ(second_mid, caller->pc()->GetTransceivers()[1]->mid()); + ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); + EXPECT_EQ(second_mid, caller->pc()->GetTransceivers()[0]->mid()); ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); - EXPECT_EQ(absl::nullopt, callee->pc()->GetTransceivers()[0]->mid()); - EXPECT_EQ(second_mid, callee->pc()->GetTransceivers()[1]->mid()); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + EXPECT_EQ(second_mid, callee->pc()->GetTransceivers()[0]->mid()); // The new answer should also recycle the m section correctly. auto answer = callee->CreateAnswer(); @@ -647,13 +648,11 @@ TEST_F(PeerConnectionJsepTest, callee->SetLocalDescription(CloneSessionDescription(answer.get()))); ASSERT_TRUE(caller->SetRemoteDescription(std::move(answer))); auto caller_transceivers = caller->pc()->GetTransceivers(); - ASSERT_EQ(2u, caller_transceivers.size()); - EXPECT_EQ(absl::nullopt, caller_transceivers[0]->mid()); - EXPECT_EQ(second_mid, caller_transceivers[1]->mid()); + ASSERT_EQ(1u, caller_transceivers.size()); + EXPECT_EQ(second_mid, caller_transceivers[0]->mid()); auto callee_transceivers = callee->pc()->GetTransceivers(); - ASSERT_EQ(2u, callee_transceivers.size()); - EXPECT_EQ(absl::nullopt, callee_transceivers[0]->mid()); - EXPECT_EQ(second_mid, callee_transceivers[1]->mid()); + ASSERT_EQ(1u, callee_transceivers.size()); + EXPECT_EQ(second_mid, callee_transceivers[0]->mid()); } // Test that creating/setting a local offer that recycles an m= section is @@ -664,7 +663,7 @@ TEST_F(PeerConnectionJsepTest, CreateOfferRecyclesWhenOfferingTwice) { auto first_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); auto callee = CreatePeerConnection(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - first_transceiver->Stop(); + first_transceiver->StopInternal(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); caller->AddAudioTrack("audio2"); @@ -675,7 +674,8 @@ TEST_F(PeerConnectionJsepTest, CreateOfferRecyclesWhenOfferingTwice) { ASSERT_EQ(1u, offer_contents.size()); EXPECT_FALSE(offer_contents[0].rejected); ASSERT_TRUE(caller->SetLocalDescription(std::move(offer))); - EXPECT_FALSE(caller->pc()->GetTransceivers()[1]->stopped()); + ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); + EXPECT_FALSE(caller->pc()->GetTransceivers()[0]->stopped()); std::string second_mid = offer_contents[0].name; // Create another new offer and set the local description again without the @@ -690,10 +690,9 @@ TEST_F(PeerConnectionJsepTest, CreateOfferRecyclesWhenOfferingTwice) { ASSERT_TRUE(caller->SetLocalDescription(std::move(second_offer))); // Make sure that the caller's transceivers are associated correctly. auto caller_transceivers = caller->pc()->GetTransceivers(); - ASSERT_EQ(2u, caller_transceivers.size()); - EXPECT_EQ(absl::nullopt, caller_transceivers[0]->mid()); - EXPECT_EQ(second_mid, caller_transceivers[1]->mid()); - EXPECT_FALSE(caller_transceivers[1]->stopped()); + ASSERT_EQ(1u, caller_transceivers.size()); + EXPECT_EQ(second_mid, caller_transceivers[0]->mid()); + EXPECT_FALSE(caller_transceivers[0]->stopped()); } // Test that the offer/answer and transceivers for both the caller and callee @@ -729,7 +728,7 @@ TEST_P(RecycleMediaSectionTest, CurrentLocalAndCurrentRemoteRejected) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); std::string first_mid = *first_transceiver->mid(); - first_transceiver->Stop(); + first_transceiver->StopInternal(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -756,11 +755,9 @@ TEST_P(RecycleMediaSectionTest, CurrentLocalAndCurrentRemoteRejected) { // create a new transceiver for the media section. ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); auto callee_transceivers = callee->pc()->GetTransceivers(); - ASSERT_EQ(2u, callee_transceivers.size()); - EXPECT_EQ(absl::nullopt, callee_transceivers[0]->mid()); - EXPECT_EQ(first_type_, callee_transceivers[0]->media_type()); - EXPECT_EQ(second_mid, callee_transceivers[1]->mid()); - EXPECT_EQ(second_type_, callee_transceivers[1]->media_type()); + ASSERT_EQ(1u, callee_transceivers.size()); + EXPECT_EQ(second_mid, callee_transceivers[0]->mid()); + EXPECT_EQ(second_type_, callee_transceivers[0]->media_type()); // The answer should have only one media section for the new transceiver. auto answer = callee->CreateAnswer(); @@ -777,8 +774,8 @@ TEST_P(RecycleMediaSectionTest, CurrentLocalAndCurrentRemoteRejected) { // Setting the remote answer should succeed and not create any new // transceivers. ASSERT_TRUE(caller->SetRemoteDescription(std::move(answer))); - ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); - ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); } // Test that recycling works properly when a new transceiver recycles an m= @@ -793,7 +790,7 @@ TEST_P(RecycleMediaSectionTest, CurrentRemoteOnlyRejected) { std::string first_mid = *caller_first_transceiver->mid(); ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; - callee_first_transceiver->Stop(); + callee_first_transceiver->StopInternal(); // The answer will have a rejected m= section. ASSERT_TRUE( @@ -821,11 +818,9 @@ TEST_P(RecycleMediaSectionTest, CurrentRemoteOnlyRejected) { // create a new transceiver for the media section. ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); auto callee_transceivers = callee->pc()->GetTransceivers(); - ASSERT_EQ(2u, callee_transceivers.size()); - EXPECT_EQ(absl::nullopt, callee_transceivers[0]->mid()); - EXPECT_EQ(first_type_, callee_transceivers[0]->media_type()); - EXPECT_EQ(second_mid, callee_transceivers[1]->mid()); - EXPECT_EQ(second_type_, callee_transceivers[1]->media_type()); + ASSERT_EQ(1u, callee_transceivers.size()); + EXPECT_EQ(second_mid, callee_transceivers[0]->mid()); + EXPECT_EQ(second_type_, callee_transceivers[0]->media_type()); // The answer should have only one media section for the new transceiver. auto answer = callee->CreateAnswer(); @@ -842,8 +837,8 @@ TEST_P(RecycleMediaSectionTest, CurrentRemoteOnlyRejected) { // Setting the remote answer should succeed and not create any new // transceivers. ASSERT_TRUE(caller->SetRemoteDescription(std::move(answer))); - ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); - ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); } // Test that recycling works properly when a new transceiver recycles an m= @@ -858,7 +853,7 @@ TEST_P(RecycleMediaSectionTest, CurrentLocalOnlyRejected) { std::string first_mid = *caller_first_transceiver->mid(); ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; - callee_first_transceiver->Stop(); + callee_first_transceiver->StopInternal(); // The answer will have a rejected m= section. ASSERT_TRUE( @@ -886,11 +881,9 @@ TEST_P(RecycleMediaSectionTest, CurrentLocalOnlyRejected) { // create a new transceiver for the media section. ASSERT_TRUE(caller->SetRemoteDescription(std::move(offer))); auto caller_transceivers = caller->pc()->GetTransceivers(); - ASSERT_EQ(2u, caller_transceivers.size()); - EXPECT_EQ(absl::nullopt, caller_transceivers[0]->mid()); - EXPECT_EQ(first_type_, caller_transceivers[0]->media_type()); - EXPECT_EQ(second_mid, caller_transceivers[1]->mid()); - EXPECT_EQ(second_type_, caller_transceivers[1]->media_type()); + ASSERT_EQ(1u, caller_transceivers.size()); + EXPECT_EQ(second_mid, caller_transceivers[0]->mid()); + EXPECT_EQ(second_type_, caller_transceivers[0]->media_type()); // The answer should have only one media section for the new transceiver. auto answer = caller->CreateAnswer(); @@ -907,8 +900,8 @@ TEST_P(RecycleMediaSectionTest, CurrentLocalOnlyRejected) { // Setting the remote answer should succeed and not create any new // transceivers. ASSERT_TRUE(callee->SetRemoteDescription(std::move(answer))); - ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); - ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); } // Test that a m= section is *not* recycled if the media section is only @@ -921,7 +914,7 @@ TEST_P(RecycleMediaSectionTest, PendingLocalRejectedAndNoRemote) { ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); std::string first_mid = *caller_first_transceiver->mid(); - caller_first_transceiver->Stop(); + caller_first_transceiver->StopInternal(); // The reoffer will have a rejected m= section. ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); @@ -959,7 +952,7 @@ TEST_P(RecycleMediaSectionTest, PendingLocalRejectedAndNotRejectedRemote) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); std::string first_mid = *caller_first_transceiver->mid(); - caller_first_transceiver->Stop(); + caller_first_transceiver->StopInternal(); // The reoffer will have a rejected m= section. ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); @@ -999,7 +992,7 @@ TEST_P(RecycleMediaSectionTest, PendingRemoteRejectedAndNoLocal) { ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; std::string first_mid = *callee_first_transceiver->mid(); - caller_first_transceiver->Stop(); + caller_first_transceiver->StopInternal(); // The reoffer will have a rejected m= section. ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); @@ -1036,7 +1029,7 @@ TEST_P(RecycleMediaSectionTest, PendingRemoteRejectedAndNotRejectedLocal) { ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; std::string first_mid = *callee_first_transceiver->mid(); - caller_first_transceiver->Stop(); + caller_first_transceiver->StopInternal(); // The reoffer will have a rejected m= section. ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); @@ -1080,7 +1073,7 @@ TEST_F(PeerConnectionJsepTest, DataChannelDoesNotRecycleMediaSection) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - transceiver->Stop(); + transceiver->StopInternal(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -1367,7 +1360,7 @@ TEST_F(PeerConnectionJsepTest, IncludeMsidEvenIfDirectionHasChanged) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->pc()->GetTransceivers()[0]->SetDirection( + caller->pc()->GetTransceivers()[0]->SetDirectionWithError( RtpTransceiverDirection::kInactive); // The transceiver direction on both sides will turn to inactive. @@ -1395,7 +1388,7 @@ TEST_F(PeerConnectionJsepTest, RemoveMsidIfTransceiverStopped) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - transceiver->Stop(); + transceiver->StopInternal(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -1552,8 +1545,9 @@ TEST_F(PeerConnectionJsepTest, CurrentDirectionResetWhenRtpTransceiverStopped) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); ASSERT_TRUE(transceiver->current_direction()); - transceiver->Stop(); - EXPECT_FALSE(transceiver->current_direction()); + transceiver->StopInternal(); + EXPECT_EQ(transceiver->current_direction(), + RtpTransceiverDirection::kStopped); } // Test that you can't set an answer on a PeerConnection before setting the @@ -2039,7 +2033,7 @@ TEST_F(PeerConnectionJsepTest, RollbackLocalDirectionChange) { EXPECT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); callee->AddAudioTrack("a"); - callee->pc()->GetTransceivers()[0]->SetDirection( + callee->pc()->GetTransceivers()[0]->SetDirectionWithError( RtpTransceiverDirection::kSendOnly); EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u); @@ -2063,7 +2057,7 @@ TEST_F(PeerConnectionJsepTest, RollbackRemoteDirectionChange) { EXPECT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); // In stable make remote audio receive only. - caller_transceiver->SetDirection(RtpTransceiverDirection::kRecvOnly); + caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly); EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u); // The direction attribute is not modified by the offer. diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc index 3c117c3ecd..f078144d4f 100644 --- a/pc/peer_connection_media_unittest.cc +++ b/pc/peer_connection_media_unittest.cc @@ -290,8 +290,8 @@ TEST_F(PeerConnectionMediaTestUnifiedPlan, // Stop both audio and video transceivers on the caller. auto transceivers = caller->pc()->GetTransceivers(); ASSERT_EQ(2u, transceivers.size()); - transceivers[0]->Stop(); - transceivers[1]->Stop(); + transceivers[0]->StopInternal(); + transceivers[1]->StopInternal(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -388,8 +388,8 @@ TEST_F(PeerConnectionMediaTestUnifiedPlan, // Stop both audio and video transceivers on the callee. auto transceivers = callee->pc()->GetTransceivers(); ASSERT_EQ(2u, transceivers.size()); - transceivers[0]->Stop(); - transceivers[1]->Stop(); + transceivers[0]->StopInternal(); + transceivers[1]->StopInternal(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index 9e4a816a45..e69a0882ae 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -370,19 +370,25 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, SetDirectionCallsOnTrack) { auto callee = CreatePeerConnection(); auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - transceiver->SetDirection(RtpTransceiverDirection::kInactive); + EXPECT_TRUE( + transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive) + .ok()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); EXPECT_EQ(0u, callee->observer()->on_track_transceivers_.size()); - transceiver->SetDirection(RtpTransceiverDirection::kSendOnly); + EXPECT_TRUE( + transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendOnly) + .ok()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); // If the direction changes but it is still receiving on the remote side, then // OnTrack should not be fired again. - transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); + EXPECT_TRUE( + transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv) + .ok()); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); @@ -401,8 +407,10 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, SetDirectionHoldCallsOnTrackTwice) { EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); // Put the call on hold by no longer receiving the track. - callee->pc()->GetTransceivers()[0]->SetDirection( - RtpTransceiverDirection::kInactive); + EXPECT_TRUE(callee->pc() + ->GetTransceivers()[0] + ->SetDirectionWithError(RtpTransceiverDirection::kInactive) + .ok()); ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); @@ -410,8 +418,10 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, SetDirectionHoldCallsOnTrackTwice) { // Resume the call by changing the direction to recvonly. This should call // OnTrack again on the callee side. - callee->pc()->GetTransceivers()[0]->SetDirection( - RtpTransceiverDirection::kRecvOnly); + EXPECT_TRUE(callee->pc() + ->GetTransceivers()[0] + ->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly) + .ok()); ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); @@ -470,7 +480,9 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, EXPECT_EQ(0u, callee->observer()->remove_track_events_.size()); auto callee_transceiver = callee->pc()->GetTransceivers()[0]; - callee_transceiver->SetDirection(RtpTransceiverDirection::kSendOnly); + EXPECT_TRUE(callee_transceiver + ->SetDirectionWithError(RtpTransceiverDirection::kSendOnly) + .ok()); ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); EXPECT_EQ(1u, callee->observer()->add_track_events_.size()); @@ -1409,7 +1421,7 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); caller->observer()->clear_negotiation_needed(); - transceiver->SetDirection(RtpTransceiverDirection::kInactive); + transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive); EXPECT_TRUE(caller->observer()->negotiation_needed()); } @@ -1422,7 +1434,7 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); caller->observer()->clear_negotiation_needed(); - transceiver->SetDirection(transceiver->direction()); + transceiver->SetDirectionWithError(transceiver->direction()); EXPECT_FALSE(caller->observer()->negotiation_needed()); } @@ -1433,13 +1445,71 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto caller = CreatePeerConnection(); auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - transceiver->Stop(); + transceiver->StopInternal(); caller->observer()->clear_negotiation_needed(); - transceiver->SetDirection(RtpTransceiverDirection::kInactive); + transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive); EXPECT_FALSE(caller->observer()->negotiation_needed()); } +// Test that currentDirection returnes "stopped" if the transceiver was stopped. +TEST_F(PeerConnectionRtpTestUnifiedPlan, + CheckStoppedCurrentDirectionOnStoppedTransceiver) { + auto caller = CreatePeerConnection(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + transceiver->StopInternal(); + + EXPECT_TRUE(transceiver->stopping()); + EXPECT_TRUE(transceiver->stopped()); + EXPECT_EQ(RtpTransceiverDirection::kStopped, + transceiver->current_direction()); +} + +// Test that InvalidState is thrown on a stopping transceiver. +TEST_F(PeerConnectionRtpTestUnifiedPlan, + CheckForInvalidStateOnStoppingTransceiver) { + auto caller = CreatePeerConnection(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + transceiver->StopStandard(); + + EXPECT_TRUE(transceiver->stopping()); + EXPECT_FALSE(transceiver->stopped()); + EXPECT_EQ( + RTCErrorType::INVALID_STATE, + transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive) + .type()); +} + +// Test that InvalidState is thrown on a stopped transceiver. +TEST_F(PeerConnectionRtpTestUnifiedPlan, + CheckForInvalidStateOnStoppedTransceiver) { + auto caller = CreatePeerConnection(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + transceiver->StopInternal(); + + EXPECT_TRUE(transceiver->stopping()); + EXPECT_TRUE(transceiver->stopped()); + EXPECT_EQ( + RTCErrorType::INVALID_STATE, + transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive) + .type()); +} + +// Test that TypeError is thrown if the direction is set to "stopped". +TEST_F(PeerConnectionRtpTestUnifiedPlan, + CheckForTypeErrorForStoppedOnTransceiver) { + auto caller = CreatePeerConnection(); + + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + EXPECT_EQ( + RTCErrorType::INVALID_PARAMETER, + transceiver->SetDirectionWithError(RtpTransceiverDirection::kStopped) + .type()); +} + // Test that AddTransceiver fails if trying to use unimplemented RTP encoding // parameters with the send_encodings parameters. TEST_F(PeerConnectionRtpTestUnifiedPlan, diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index 42bdae17b9..8822a980f7 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -455,7 +455,7 @@ TEST_F(PeerConnectionSimulcastTests, ServerSendsOfferToReceiveSimulcast) { std::string error; EXPECT_TRUE(remote->SetRemoteDescription(std::move(offer), &error)) << error; auto transceiver = remote->pc()->GetTransceivers()[0]; - transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); + transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); EXPECT_TRUE(remote->CreateAnswerAndSetAsLocal()); ValidateTransceiverParameters(transceiver, layers); } @@ -478,7 +478,7 @@ TEST_F(PeerConnectionSimulcastTests, TransceiverIsNotRecycledWithSimulcast) { auto transceivers = remote->pc()->GetTransceivers(); ASSERT_EQ(2u, transceivers.size()); auto transceiver = transceivers[1]; - transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); + transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); EXPECT_TRUE(remote->CreateAnswerAndSetAsLocal()); ValidateTransceiverParameters(transceiver, layers); } @@ -611,7 +611,7 @@ TEST_F(PeerConnectionSimulcastMetricsTests, IncomingSimulcastIsLogged) { ElementsAre(Pair(kSimulcastApiVersionSpecCompliant, 1))); auto transceiver = remote->pc()->GetTransceivers()[0]; - transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); + transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); EXPECT_TRUE(remote->CreateAnswerAndSetAsLocal()); EXPECT_THAT(LocalDescriptionSamples(), ElementsAre(Pair(kSimulcastApiVersionSpecCompliant, 2))); diff --git a/pc/rtp_media_utils.cc b/pc/rtp_media_utils.cc index 8fbfca1f98..c5d642b685 100644 --- a/pc/rtp_media_utils.cc +++ b/pc/rtp_media_utils.cc @@ -42,6 +42,7 @@ RtpTransceiverDirection RtpTransceiverDirectionReversed( switch (direction) { case RtpTransceiverDirection::kSendRecv: case RtpTransceiverDirection::kInactive: + case RtpTransceiverDirection::kStopped: return direction; case RtpTransceiverDirection::kSendOnly: return RtpTransceiverDirection::kRecvOnly; diff --git a/pc/rtp_sender.cc b/pc/rtp_sender.cc index 3c56bf0724..1430e299c4 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc @@ -184,6 +184,15 @@ RTCError RtpSenderBase::SetParametersInternal(const RtpParameters& parameters) { RTCError RtpSenderBase::SetParameters(const RtpParameters& parameters) { TRACE_EVENT0("webrtc", "RtpSenderBase::SetParameters"); + if (is_transceiver_stopped_) { + LOG_AND_RETURN_ERROR( + RTCErrorType::INVALID_STATE, + "Cannot set parameters on sender of a stopped transceiver."); + } + if (stopped_) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE, + "Cannot set parameters on a stopped sender."); + } if (stopped_) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE, "Cannot set parameters on a stopped sender."); diff --git a/pc/rtp_sender.h b/pc/rtp_sender.h index 15d47fd90d..c343ff085d 100644 --- a/pc/rtp_sender.h +++ b/pc/rtp_sender.h @@ -69,6 +69,8 @@ class RtpSenderInternal : public RtpSenderInterface { // If the specified list is empty, this is a no-op. virtual RTCError DisableEncodingLayers( const std::vector& rid) = 0; + + virtual void SetTransceiverAsStopped() = 0; }; // Shared implementation for RtpSenderInternal interface. @@ -152,6 +154,8 @@ class RtpSenderBase : public RtpSenderInternal, public ObserverInterface { void SetEncoderToPacketizerFrameTransformer( rtc::scoped_refptr frame_transformer) override; + void SetTransceiverAsStopped() override { is_transceiver_stopped_ = true; } + protected: // If |set_streams_observer| is not null, it is invoked when SetStreams() // is called. |set_streams_observer| is not owned by this object. If not @@ -180,6 +184,7 @@ class RtpSenderBase : public RtpSenderInternal, public ObserverInterface { rtc::Thread* worker_thread_; uint32_t ssrc_ = 0; bool stopped_ = false; + bool is_transceiver_stopped_ = false; int attachment_id_ = 0; const std::string id_; diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index b4e500bbc8..8d14e46b4b 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -97,10 +97,19 @@ RTCError VerifyCodecPreferences(const std::vector& codecs, return RTCError::OK(); } +TaskQueueBase* GetCurrentTaskQueueOrThread() { + TaskQueueBase* current = TaskQueueBase::Current(); + if (!current) + current = rtc::ThreadManager::Instance()->CurrentThread(); + return current; +} + } // namespace RtpTransceiver::RtpTransceiver(cricket::MediaType media_type) - : unified_plan_(false), media_type_(media_type) { + : thread_(GetCurrentTaskQueueOrThread()), + unified_plan_(false), + media_type_(media_type) { RTC_DCHECK(media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO); } @@ -111,7 +120,8 @@ RtpTransceiver::RtpTransceiver( receiver, cricket::ChannelManager* channel_manager, std::vector header_extensions_offered) - : unified_plan_(true), + : thread_(GetCurrentTaskQueueOrThread()), + unified_plan_(true), media_type_(sender->media_type()), channel_manager_(channel_manager), header_extensions_to_offer_(std::move(header_extensions_offered)) { @@ -123,7 +133,7 @@ RtpTransceiver::RtpTransceiver( } RtpTransceiver::~RtpTransceiver() { - Stop(); + StopInternal(); } void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { @@ -277,23 +287,51 @@ bool RtpTransceiver::stopped() const { return stopped_; } +bool RtpTransceiver::stopping() const { + RTC_DCHECK_RUN_ON(thread_); + return stopping_; +} + RtpTransceiverDirection RtpTransceiver::direction() const { + if (unified_plan_ && stopping()) + return webrtc::RtpTransceiverDirection::kStopped; + return direction_; } void RtpTransceiver::SetDirection(RtpTransceiverDirection new_direction) { - if (stopped()) { - return; + // Catch internal usage of SetDirection without error. + // We cannot deprecate this function while it is being proxied, so + // settle for causing a runtime error instead. + RTC_NOTREACHED(); + SetDirectionWithError(new_direction); +} + +RTCError RtpTransceiver::SetDirectionWithError( + RtpTransceiverDirection new_direction) { + if (unified_plan_ && stopping()) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE, + "Cannot set direction on a stopping transceiver."); } - if (new_direction == direction_) { - return; + if (new_direction == direction_) + return RTCError::OK(); + + if (new_direction == RtpTransceiverDirection::kStopped) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, + "The set direction 'stopped' is invalid."); } + direction_ = new_direction; SignalNegotiationNeeded(); + + return RTCError::OK(); } absl::optional RtpTransceiver::current_direction() const { + if (unified_plan_ && stopping()) + return webrtc::RtpTransceiverDirection::kStopped; + return current_direction_; } @@ -302,14 +340,69 @@ absl::optional RtpTransceiver::fired_direction() return fired_direction_; } -void RtpTransceiver::Stop() { - for (const auto& sender : senders_) { +void RtpTransceiver::StopSendingAndReceiving() { + // 1. Let sender be transceiver.[[Sender]]. + // 2. Let receiver be transceiver.[[Receiver]]. + // + // 3. Stop sending media with sender. + // + // 4. Send an RTCP BYE for each RTP stream that was being sent by sender, as + // specified in [RFC3550]. + RTC_DCHECK_RUN_ON(thread_); + for (const auto& sender : senders_) sender->internal()->Stop(); - } - for (const auto& receiver : receivers_) { + + // 5. Stop receiving media with receiver. + for (const auto& receiver : receivers_) receiver->internal()->Stop(); + + stopping_ = true; + direction_ = webrtc::RtpTransceiverDirection::kInactive; +} + +RTCError RtpTransceiver::StopStandard() { + RTC_DCHECK_RUN_ON(thread_); + RTC_DCHECK(unified_plan_); + // 1. Let transceiver be the RTCRtpTransceiver object on which the method is + // invoked. + // + // 2. Let connection be the RTCPeerConnection object associated with + // transceiver. + // + // 3. If connection.[[IsClosed]] is true, throw an InvalidStateError. + if (is_pc_closed_) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE, + "PeerConnection is closed."); } + + // 4. If transceiver.[[Stopping]] is true, abort these steps. + if (stopping_) + return RTCError::OK(); + + // 5. Stop sending and receiving given transceiver, and update the + // negotiation-needed flag for connection. + StopSendingAndReceiving(); + SignalNegotiationNeeded(); + + return RTCError::OK(); +} + +void RtpTransceiver::StopInternal() { + RTC_DCHECK_RUN_ON(thread_); + // 1. If transceiver.[[Stopping]] is false, stop sending and receiving given + // transceiver. + if (!stopping_) + StopSendingAndReceiving(); + + // 2. Set transceiver.[[Stopped]] to true. stopped_ = true; + + // Signal the updated change to the senders. + for (const auto& sender : senders_) + sender->internal()->SetTransceiverAsStopped(); + + // 3. Set transceiver.[[Receptive]] to false. + // 4. Set transceiver.[[CurrentDirection]] to null. current_direction_ = absl::nullopt; } @@ -403,4 +496,8 @@ RTCError RtpTransceiver::SetOfferedRtpHeaderExtensions( return RTCError::OK(); } +void RtpTransceiver::SetPeerConnectionClosed() { + is_pc_closed_ = true; +} + } // namespace webrtc diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index be46ccfd5c..980d64ca76 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -173,6 +173,10 @@ class RtpTransceiver final return has_ever_been_used_to_send_; } + // Informs the transceiver that its owning + // PeerConnection is closed. + void SetPeerConnectionClosed(); + // Fired when the RtpTransceiver state changes such that negotiation is now // needed (e.g., in response to a direction change). sigslot::signal0<> SignalNegotiationNeeded; @@ -183,11 +187,15 @@ class RtpTransceiver final rtc::scoped_refptr sender() const override; rtc::scoped_refptr receiver() const override; bool stopped() const override; + bool stopping() const override; RtpTransceiverDirection direction() const override; void SetDirection(RtpTransceiverDirection new_direction) override; + RTCError SetDirectionWithError( + RtpTransceiverDirection new_direction) override; absl::optional current_direction() const override; absl::optional fired_direction() const override; - void Stop() override; + RTCError StopStandard() override; + void StopInternal() override; RTCError SetCodecPreferences( rtc::ArrayView codecs) override; std::vector codec_preferences() const override { @@ -201,7 +209,10 @@ class RtpTransceiver final private: void OnFirstPacketReceived(cricket::ChannelInterface* channel); + void StopSendingAndReceiving(); + // Enforce that this object is created, used and destroyed on one thread. + const TaskQueueBase* thread_; const bool unified_plan_; const cricket::MediaType media_type_; std::vector>> @@ -211,6 +222,8 @@ class RtpTransceiver final receivers_; bool stopped_ = false; + bool stopping_ RTC_GUARDED_BY(thread_) = false; + bool is_pc_closed_ = false; RtpTransceiverDirection direction_ = RtpTransceiverDirection::kInactive; absl::optional current_direction_; absl::optional fired_direction_; @@ -233,11 +246,14 @@ PROXY_CONSTMETHOD0(absl::optional, mid) PROXY_CONSTMETHOD0(rtc::scoped_refptr, sender) PROXY_CONSTMETHOD0(rtc::scoped_refptr, receiver) PROXY_CONSTMETHOD0(bool, stopped) +PROXY_CONSTMETHOD0(bool, stopping) PROXY_CONSTMETHOD0(RtpTransceiverDirection, direction) PROXY_METHOD1(void, SetDirection, RtpTransceiverDirection) +PROXY_METHOD1(webrtc::RTCError, SetDirectionWithError, RtpTransceiverDirection) PROXY_CONSTMETHOD0(absl::optional, current_direction) PROXY_CONSTMETHOD0(absl::optional, fired_direction) -PROXY_METHOD0(void, Stop) +PROXY_METHOD0(webrtc::RTCError, StopStandard) +PROXY_METHOD0(void, StopInternal) PROXY_METHOD1(webrtc::RTCError, SetCodecPreferences, rtc::ArrayView) diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index e3f05c4dd9..78abfff8b9 100644 --- a/pc/rtp_transceiver_unittest.cc +++ b/pc/rtp_transceiver_unittest.cc @@ -45,7 +45,7 @@ TEST(RtpTransceiverTest, CannotSetChannelOnStoppedTransceiver) { EXPECT_EQ(&channel1, transceiver.channel()); // Stop the transceiver. - transceiver.Stop(); + transceiver.StopInternal(); EXPECT_EQ(&channel1, transceiver.channel()); cricket::MockChannelInterface channel2; @@ -71,7 +71,7 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { EXPECT_EQ(&channel, transceiver.channel()); // Stop the transceiver. - transceiver.Stop(); + transceiver.StopInternal(); EXPECT_EQ(&channel, transceiver.channel()); // Set the channel to |nullptr|. diff --git a/pc/test/mock_rtp_sender_internal.h b/pc/test/mock_rtp_sender_internal.h index 1a31c5dac6..5e7670ebf0 100644 --- a/pc/test/mock_rtp_sender_internal.h +++ b/pc/test/mock_rtp_sender_internal.h @@ -65,23 +65,17 @@ class MockRtpSenderInternal : public RtpSenderInternal { (const, override)); // RtpSenderInternal methods. - MOCK_METHOD(void, SetMediaChannel, (cricket::MediaChannel*), (override)); - MOCK_METHOD(void, SetSsrc, (uint32_t), (override)); - MOCK_METHOD(void, - set_stream_ids, - (const std::vector&), - (override)); - MOCK_METHOD(void, SetStreams, (const std::vector&), (override)); - MOCK_METHOD(void, - set_init_send_encodings, - (const std::vector&), - (override)); - MOCK_METHOD(void, Stop, (), (override)); - MOCK_METHOD(int, AttachmentId, (), (const, override)); - MOCK_METHOD(RTCError, - DisableEncodingLayers, - (const std::vector&), - (override)); + MOCK_METHOD1(SetMediaChannel, void(cricket::MediaChannel*)); + MOCK_METHOD1(SetSsrc, void(uint32_t)); + MOCK_METHOD1(set_stream_ids, void(const std::vector&)); + MOCK_METHOD1(SetStreams, void(const std::vector&)); + MOCK_METHOD1(set_init_send_encodings, + void(const std::vector&)); + MOCK_METHOD0(Stop, void()); + MOCK_CONST_METHOD0(AttachmentId, int()); + MOCK_METHOD1(DisableEncodingLayers, + RTCError(const std::vector&)); + MOCK_METHOD0(SetTransceiverAsStopped, void()); }; } // namespace webrtc diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index 7a2aeae3a4..4af121ddc3 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -1568,6 +1568,8 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, // RFC 3264 // a=sendrecv || a=sendonly || a=sendrecv || a=inactive switch (media_desc->direction()) { + // Special case that for sdp purposes should be treated same as inactive. + case RtpTransceiverDirection::kStopped: case RtpTransceiverDirection::kInactive: InitAttrLine(kAttributeInactive, &os); break; @@ -1580,9 +1582,7 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, case RtpTransceiverDirection::kSendRecv: InitAttrLine(kAttributeSendRecv, &os); break; - case RtpTransceiverDirection::kStopped: default: - // kStopped shouldn't be used in signalling. RTC_NOTREACHED(); InitAttrLine(kAttributeSendRecv, &os); break; diff --git a/sdk/android/api/org/webrtc/RtpTransceiver.java b/sdk/android/api/org/webrtc/RtpTransceiver.java index 64d8eb41d1..021cc90bc5 100644 --- a/sdk/android/api/org/webrtc/RtpTransceiver.java +++ b/sdk/android/api/org/webrtc/RtpTransceiver.java @@ -206,13 +206,34 @@ public class RtpTransceiver { } /** - * The Stop method irreversibly stops the RtpTransceiver. The sender of this - * transceiver will no longer send, the receiver will no longer receive. - * https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stop + * The Stop method will for the time being call the StopInternal method. + * After a migration procedure, stop() will be equivalent to StopStandard. */ public void stop() { checkRtpTransceiverExists(); - nativeStop(nativeRtpTransceiver); + nativeStopInternal(nativeRtpTransceiver); + } + + /** + * The StopInternal method stops the RtpTransceiver, like Stop, but goes + * immediately to Stopped state. + */ + public void stopInternal() { + checkRtpTransceiverExists(); + nativeStopInternal(nativeRtpTransceiver); + } + + /** + * The StopStandard method irreversibly stops the RtpTransceiver. The sender + * of this transceiver will no longer send, the receiver will no longer + * receive. + * + *

The transceiver will enter Stopping state and signal NegotiationNeeded. + * https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stop + */ + public void stopStandard() { + checkRtpTransceiverExists(); + nativeStopStandard(nativeRtpTransceiver); } @CalledByNative @@ -237,7 +258,8 @@ public class RtpTransceiver { private static native boolean nativeStopped(long rtpTransceiver); private static native RtpTransceiverDirection nativeDirection(long rtpTransceiver); private static native RtpTransceiverDirection nativeCurrentDirection(long rtpTransceiver); - private static native void nativeStop(long rtpTransceiver); + private static native void nativeStopInternal(long rtpTransceiver); + private static native void nativeStopStandard(long rtpTransceiver); private static native void nativeSetDirection( long rtpTransceiver, RtpTransceiverDirection rtpTransceiverDirection); } diff --git a/sdk/android/src/jni/pc/rtp_transceiver.cc b/sdk/android/src/jni/pc/rtp_transceiver.cc index 7d8cfdef49..a0b3c20fdd 100644 --- a/sdk/android/src/jni/pc/rtp_transceiver.cc +++ b/sdk/android/src/jni/pc/rtp_transceiver.cc @@ -139,9 +139,16 @@ ScopedJavaLocalRef JNI_RtpTransceiver_CurrentDirection( : nullptr; } -void JNI_RtpTransceiver_Stop(JNIEnv* jni, - jlong j_rtp_transceiver_pointer) { - reinterpret_cast(j_rtp_transceiver_pointer)->Stop(); +void JNI_RtpTransceiver_StopInternal(JNIEnv* jni, + jlong j_rtp_transceiver_pointer) { + reinterpret_cast(j_rtp_transceiver_pointer) + ->StopInternal(); +} + +void JNI_RtpTransceiver_StopStandard(JNIEnv* jni, + jlong j_rtp_transceiver_pointer) { + reinterpret_cast(j_rtp_transceiver_pointer) + ->StopStandard(); } void JNI_RtpTransceiver_SetDirection( diff --git a/sdk/objc/api/peerconnection/RTCRtpTransceiver.h b/sdk/objc/api/peerconnection/RTCRtpTransceiver.h index f8996ccafb..17054f5e43 100644 --- a/sdk/objc/api/peerconnection/RTCRtpTransceiver.h +++ b/sdk/objc/api/peerconnection/RTCRtpTransceiver.h @@ -117,7 +117,7 @@ RTC_OBJC_EXPORT * this transceiver will no longer send, the receiver will no longer receive. * https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver-stop */ -- (void)stop; +- (void)stopInternal; @end diff --git a/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm b/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm index 2995e5fceb..5d0d8eda37 100644 --- a/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm +++ b/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm @@ -90,8 +90,8 @@ } } -- (void)stop { - _nativeRtpTransceiver->Stop(); +- (void)stopInternal { + _nativeRtpTransceiver->StopInternal(); } - (NSString *)description {