From a88c9776de2825b3b892b154fd485b4ac2c3a384 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 10 Aug 2020 18:06:09 +0000 Subject: [PATCH] Revert "Implement transceiver.stop()" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 11dc6571cb4ff3e71dee1557dfff8d9076e108d3. Reason for revert: Breaks Chromium WPT tests Original change's description: > Implement transceiver.stop() > > 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} TBR=sakal@webrtc.org,kthelgason@webrtc.org,hta@webrtc.org,guidou@webrtc.org,marinaciocea@webrtc.org Change-Id: Ibdc24f7d41e481293ca74ba6d1572de64f7e4654 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:980879 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/181262 Reviewed-by: Harald Alvestrand Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#31897} --- 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 | 121 ++------------ 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, 254 insertions(+), 605 deletions(-) diff --git a/api/rtp_transceiver_interface.cc b/api/rtp_transceiver_interface.cc index 1dc0fcc79e..e795e51dfb 100644 --- a/api/rtp_transceiver_interface.cc +++ b/api/rtp_transceiver_interface.cc @@ -25,23 +25,6 @@ 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"; @@ -64,17 +47,4 @@ 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 cdda34b193..13277d9a50 100644 --- a/api/rtp_transceiver_interface.h +++ b/api/rtp_transceiver_interface.h @@ -89,16 +89,6 @@ 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 @@ -109,10 +99,7 @@ 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 - // 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); + virtual void SetDirection(RtpTransceiverDirection new_direction) = 0; // The current_direction attribute indicates the current direction negotiated // for this transceiver. If this transceiver has never been represented in an @@ -127,19 +114,10 @@ class RTC_EXPORT RtpTransceiverInterface : public rtc::RefCountInterface { // Exposed in the public interface for use by Chromium. virtual absl::optional fired_direction() const; - // Initiates a stop of the transceiver. - // The stop is complete when stopped() returns true. - // A stopped transceiver can be reused for a different track. + // 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 - // 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(); + virtual void Stop() = 0; // 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 0cff84d79b..69ddb0c895 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1507,6 +1507,7 @@ 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); @@ -1795,13 +1796,15 @@ 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_; } } @@ -1812,7 +1815,6 @@ 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)); @@ -1820,6 +1822,9 @@ const AudioCodecs& MediaSessionDescriptionFactory::GetAudioCodecsForAnswer( return audio_send_codecs_; case RtpTransceiverDirection::kRecvOnly: return audio_recv_codecs_; + case RtpTransceiverDirection::kStopped: + RTC_NOTREACHED(); + return audio_sendrecv_codecs_; } } @@ -1828,13 +1833,15 @@ 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_; } } @@ -1845,7 +1852,6 @@ 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)); @@ -1853,6 +1859,9 @@ 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 e1ff313941..ac949fb630 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -4863,8 +4863,7 @@ void TestAudioCodecsAnswer(RtpTransceiverDirection offer_direction, kResultSendrecv_SendrecvCodecs); } break; - case RtpTransceiverDirection::kStopped: - // This does not happen in any current test. + default: RTC_NOTREACHED(); } diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 541f04e5fc..982e42a2d6 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->StopInternal(); + transceiver->Stop(); } stats_.reset(nullptr); @@ -1535,11 +1535,6 @@ 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); @@ -1938,9 +1933,6 @@ 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()); } @@ -1964,9 +1956,6 @@ 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()); @@ -1981,13 +1970,7 @@ PeerConnection::GetTransceivers() const { << "GetTransceivers is only supported with Unified Plan SdpSemantics."; std::vector> all_transceivers; for (const auto& transceiver : transceivers_) { - // 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); - } + all_transceivers.push_back(transceiver); } return all_transceivers; } @@ -2598,35 +2581,6 @@ 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( @@ -2716,10 +2670,6 @@ 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. @@ -2805,9 +2755,6 @@ RTCError PeerConnection::ApplyLocalDescription( if (IsUnifiedPlan()) { for (const auto& transceiver : transceivers_) { - if (transceiver->stopped()) { - continue; - } const ContentInfo* content = FindMediaSectionForTransceiver(transceiver, local_description()); if (!content) { @@ -3316,7 +3263,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->Stop(); } if (!content->rejected && RtpTransceiverDirectionHasRecv(local_direction)) { @@ -4392,9 +4339,7 @@ void PeerConnection::Close() { NoteUsageEvent(UsageEvent::CLOSE_CALLED); for (const auto& transceiver : transceivers_) { - transceiver->internal()->SetPeerConnectionClosed(); - if (!transceiver->stopped()) - transceiver->StopInternal(); + transceiver->Stop(); } // Ensure that all asynchronous stats requests are completed before destroying @@ -4960,15 +4905,10 @@ static cricket::MediaDescriptionOptions GetMediaDescriptionOptionsForTransceiver( rtc::scoped_refptr> transceiver, - 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(); + const std::string& mid) { cricket::MediaDescriptionOptions media_description_options( - transceiver->media_type(), mid, transceiver->direction(), stopped); + transceiver->media_type(), mid, transceiver->direction(), + transceiver->stopped()); media_description_options.codec_preferences = transceiver->codec_preferences(); media_description_options.header_extensions = @@ -4978,8 +4918,9 @@ 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 (stopped || (!RtpTransceiverDirectionHasSend(transceiver->direction()) && - !transceiver->internal()->has_ever_been_used_to_send())) { + if (transceiver->stopped() || + (!RtpTransceiverDirectionHasSend(transceiver->direction()) && + !transceiver->internal()->has_ever_been_used_to_send())) { return media_description_options; } @@ -5073,39 +5014,25 @@ 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. - auto transceiver = GetAssociatedTransceiver(mid); - if (!transceiver) { - // No associated transceiver. The media section has been stopped. - recycleable_mline_indices.push(i); + if (had_been_rejected && transceiver->stopped()) { session_options->media_description_options.push_back( - cricket::MediaDescriptionOptions(media_type, mid, + cricket::MediaDescriptionOptions(transceiver->media_type(), mid, RtpTransceiverDirection::kInactive, /*stopped=*/true)); + recycleable_mline_indices.push(i); } else { - // 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); - } + 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); } } else { RTC_CHECK_EQ(cricket::MEDIA_TYPE_DATA, media_type); @@ -5130,7 +5057,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->stopping()) { + if (transceiver->mid() || transceiver->stopped()) { continue; } size_t mline_index; @@ -5138,13 +5065,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_(), /*is_create_offer=*/true); + GetMediaDescriptionOptionsForTransceiver(transceiver, + mid_generator_()); } else { mline_index = session_options->media_description_options.size(); session_options->media_description_options.push_back( - GetMediaDescriptionOptionsForTransceiver( - transceiver, mid_generator_(), /*is_create_offer=*/true)); + GetMediaDescriptionOptionsForTransceiver(transceiver, + mid_generator_())); } // See comment above for why CreateOffer changes the transceiver's state. transceiver->internal()->set_mline_index(mline_index); @@ -5255,8 +5182,7 @@ void PeerConnection::GetOptionsForUnifiedPlanAnswer( auto transceiver = GetAssociatedTransceiver(content.name); RTC_CHECK(transceiver); session_options->media_description_options.push_back( - GetMediaDescriptionOptionsForTransceiver(transceiver, content.name, - /*is_create_offer=*/false)); + GetMediaDescriptionOptionsForTransceiver(transceiver, content.name)); } else { RTC_CHECK_EQ(cricket::MEDIA_TYPE_DATA, media_type); // Reject all data sections if data channels are disabled. @@ -7347,8 +7273,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { // 1. If any implementation-specific negotiation is required, as described at // the start of this section, return true. - // 2. If connection.[[LocalIceCredentialsToReplace]] is not empty, return - // true. + // 2. If connection's [[RestartIce]] internal slot is true, return true. if (local_ice_credentials_to_replace_->HasIceCredentials()) { return true; } @@ -7374,12 +7299,11 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { const ContentInfo* current_remote_msection = FindTransceiverMSection( transceiver.get(), current_remote_description()); - // 5.4 If transceiver is stopped and is associated with an m= section, + // 5.3 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)) { @@ -7388,22 +7312,17 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { continue; } - // 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= + // 5.1 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.3 If transceiver isn't stopped and is associated with an m= section + // 5.2 If transceiver isn't stopped and is associated with an m= section // in description then perform the following checks: - // 5.3.1 If transceiver.[[Direction]] is "sendrecv" or "sendonly", and the + // 5.2.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 @@ -7429,7 +7348,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { return true; } - // 5.3.2 If description is of type "offer", and the direction of the + // 5.2.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. @@ -7451,7 +7370,7 @@ bool PeerConnection::CheckIfNegotiationIsNeeded() { } } - // 5.3.3 If description is of type "answer", and the direction of the + // 5.2.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 c544db396f..543c9be81a 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->StopInternal(); + transceiver->Stop(); } EXPECT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index dd24163f3e..afb5f2ba75 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -799,7 +799,9 @@ 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()); - RemoveUnusedVideoRenderers(); + if (sdp_semantics_ == SdpSemantics::kUnifiedPlan) { + RemoveUnusedVideoRenderers(); + } // As mentioned above, we need to send the message immediately after // SetLocalDescription. SendSdpMessage(type, sdp); @@ -812,7 +814,9 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, new rtc::RefCountedObject()); RTC_LOG(LS_INFO) << debug_name_ << ": SetRemoteDescription"; pc()->SetRemoteDescription(observer, desc.release()); - RemoveUnusedVideoRenderers(); + if (sdp_semantics_ == SdpSemantics::kUnifiedPlan) { + RemoveUnusedVideoRenderers(); + } EXPECT_TRUE_WAIT(observer->called(), kDefaultTimeout); return observer->result(); } @@ -820,26 +824,29 @@ 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) { - // 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()); + if (transceiver->receiver()->media_type() != cricket::MEDIA_TYPE_VIDEO) { + continue; } - } - 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++; + // 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); + } } } } @@ -929,11 +936,8 @@ 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()); - if (it != fake_video_renderers_.end()) { - fake_video_renderers_.erase(it); - } else { - RTC_LOG(LS_ERROR) << "OnRemoveTrack called for non-active renderer"; - } + RTC_DCHECK(it != fake_video_renderers_.end()); + fake_video_renderers_.erase(it); } } void OnRenegotiationNeeded() override {} @@ -1557,9 +1561,6 @@ 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(); @@ -2236,9 +2237,7 @@ TEST_P(PeerConnectionIntegrationTest, AudioToVideoUpgrade) { callee()->SetOfferAnswerOptions(options); } else { callee()->SetRemoteOfferHandler([this] { - callee() - ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) - ->StopInternal(); + callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); }); } // Do offer/answer and make sure audio is still received end-to-end. @@ -2270,12 +2269,11 @@ 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(2U, transceivers.size()); + ASSERT_EQ(3U, transceivers.size()); ASSERT_EQ(cricket::MEDIA_TYPE_VIDEO, - transceivers[1]->receiver()->media_type()); - transceivers[1]->sender()->SetTrack(caller()->CreateLocalVideoTrack()); - transceivers[1]->SetDirectionWithError( - RtpTransceiverDirection::kSendRecv); + transceivers[2]->receiver()->media_type()); + transceivers[2]->sender()->SetTrack(caller()->CreateLocalVideoTrack()); + transceivers[2]->SetDirection(RtpTransceiverDirection::kSendRecv); }); } callee()->CreateAndSetAndSignalOffer(); @@ -2487,9 +2485,7 @@ 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) - ->StopInternal(); + callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO)->Stop(); }); } callee()->AddTrack(callee()->CreateLocalVideoTrack()); @@ -2508,10 +2504,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, - // and thus no longer listed in transceivers. - EXPECT_EQ(nullptr, - caller()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO)); + // The caller's transceiver should have stopped after receiving the answer. + EXPECT_TRUE(caller() + ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_AUDIO) + ->stopped()); } } @@ -2531,9 +2527,7 @@ 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) - ->StopInternal(); + callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); }); } callee()->AddTrack(callee()->CreateLocalAudioTrack()); @@ -2552,10 +2546,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, - // and thus is no longer present. - EXPECT_EQ(nullptr, - caller()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)); + // The caller's transceiver should have stopped after receiving the answer. + EXPECT_TRUE(caller() + ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) + ->stopped()); } } @@ -2579,7 +2573,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->StopInternal(); + transceiver->Stop(); } }); } @@ -2626,9 +2620,7 @@ TEST_P(PeerConnectionIntegrationTest, VideoRejectedInSubsequentOffer) { } }); } else { - caller() - ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) - ->StopInternal(); + caller()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); } caller()->CreateAndSetAndSignalOffer(); ASSERT_TRUE_WAIT(SignalingStateStable(), kMaxWaitForActivationMs); @@ -2743,7 +2735,7 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); // Add receive direction. - video_sender->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); + video_sender->SetDirection(RtpTransceiverDirection::kSendRecv); rtc::scoped_refptr callee_track = callee()->CreateLocalVideoTrack(); @@ -4353,9 +4345,7 @@ TEST_P(PeerConnectionIntegrationTest, callee()->SetOfferAnswerOptions(options); } else { callee()->SetRemoteOfferHandler([this] { - callee() - ->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO) - ->StopInternal(); + callee()->GetFirstTransceiverOfType(cricket::MEDIA_TYPE_VIDEO)->Stop(); }); } caller()->CreateAndSetAndSignalOffer(); @@ -4376,7 +4366,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_EQ(nullptr, caller_transceiver.get()); + EXPECT_TRUE(caller_transceiver->stopped()); caller()->AddVideoTrack(); } callee()->AddVideoTrack(); @@ -4439,9 +4429,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]->SetDirectionWithError( + callee()->pc()->GetTransceivers()[0]->SetDirection( RtpTransceiverDirection::kSendRecv); - callee()->pc()->GetTransceivers()[1]->SetDirectionWithError( + callee()->pc()->GetTransceivers()[1]->SetDirection( RtpTransceiverDirection::kSendRecv); }); caller()->CreateAndSetAndSignalOffer(); @@ -5562,7 +5552,7 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, ASSERT_TRUE(ExpectNewFrames(media_expectations)); } - audio_transceiver->StopInternal(); + audio_transceiver->Stop(); caller()->pc()->AddTransceiver(caller()->CreateLocalVideoTrack()); caller()->CreateAndSetAndSignalOffer(); diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 046d1c93bc..901e5c572c 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -2668,24 +2668,23 @@ TEST_P(PeerConnectionInterfaceTest, CloseAndTestStreamsAndStates) { EXPECT_EQ(1u, pc_->local_streams()->count()); EXPECT_EQ(1u, pc_->remote_streams()->count()); } else { - // Verify that the RtpTransceivers are no longer returned. - EXPECT_EQ(0u, pc_->GetTransceivers().size()); + // 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()); + } } auto audio_receiver = GetFirstReceiverOfType(cricket::MEDIA_TYPE_AUDIO); + ASSERT_TRUE(audio_receiver); auto video_receiver = GetFirstReceiverOfType(cricket::MEDIA_TYPE_VIDEO); - 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); - } + 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); } // Test that PeerConnection methods fails gracefully after diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index c4b7de10f7..0b2f375dde 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->StopInternal(); + transceiver->Stop(); 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->SetDirectionWithError(RtpTransceiverDirection::kSendOnly); + caller_audio->SetDirection(RtpTransceiverDirection::kSendOnly); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); @@ -358,14 +358,16 @@ TEST_F(PeerConnectionJsepTest, SetRemoteOfferDoesNotReuseStoppedTransceiver) { caller->AddAudioTrack("a"); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); - callee->pc()->GetTransceivers()[0]->StopInternal(); + callee->pc()->GetTransceivers()[0]->Stop(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto transceivers = callee->pc()->GetTransceivers(); - ASSERT_EQ(1u, transceivers.size()); - EXPECT_EQ(caller->pc()->GetTransceivers()[0]->mid(), transceivers[0]->mid()); - EXPECT_FALSE(transceivers[0]->stopped()); + 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()); } // Test that audio and video transceivers created on the remote side with @@ -430,7 +432,7 @@ TEST_F(PeerConnectionJsepTest, CreateAnswerRejectsStoppedTransceiver) { ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - callee->pc()->GetTransceivers()[0]->StopInternal(); + callee->pc()->GetTransceivers()[0]->Stop(); auto answer = callee->CreateAnswer(); auto contents = answer->description()->contents(); @@ -467,7 +469,7 @@ TEST_F(PeerConnectionJsepTest, CreateAnswerNegotiatesDirection) { TEST_F(PeerConnectionJsepTest, SetLocalAnswerUpdatesCurrentDirection) { auto caller = CreatePeerConnection(); auto caller_audio = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - caller_audio->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly); + caller_audio->SetDirection(RtpTransceiverDirection::kRecvOnly); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); @@ -492,7 +494,7 @@ TEST_F(PeerConnectionJsepTest, SetRemoteAnswerUpdatesCurrentDirection) { auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); auto callee_audio = callee->pc()->GetTransceivers()[0]; - callee_audio->SetDirectionWithError(RtpTransceiverDirection::kSendOnly); + callee_audio->SetDirection(RtpTransceiverDirection::kSendOnly); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_TRUE( @@ -516,7 +518,7 @@ TEST_F(PeerConnectionJsepTest, SettingTransceiverInactiveDoesNotStopIt) { caller->AddAudioTrack("a"); auto callee = CreatePeerConnection(); callee->AddAudioTrack("a"); - callee->pc()->GetTransceivers()[0]->SetDirectionWithError( + callee->pc()->GetTransceivers()[0]->SetDirection( RtpTransceiverDirection::kInactive); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); @@ -541,7 +543,7 @@ TEST_F(PeerConnectionJsepTest, caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); ASSERT_TRUE(transceiver->mid()); - transceiver->StopInternal(); + transceiver->Stop(); auto reoffer = caller->CreateOffer(); auto contents = reoffer->description()->contents(); @@ -562,12 +564,13 @@ TEST_F(PeerConnectionJsepTest, ASSERT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); - transceiver->StopInternal(); + transceiver->Stop(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto transceivers = callee->pc()->GetTransceivers(); - EXPECT_EQ(0u, transceivers.size()); + EXPECT_TRUE(transceivers[0]->stopped()); + EXPECT_TRUE(transceivers[0]->mid()); } // Test that CreateOffer will only generate a recycled media section if the @@ -583,7 +586,7 @@ TEST_F(PeerConnectionJsepTest, caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); auto second_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - first_transceiver->StopInternal(); + first_transceiver->Stop(); auto reoffer = caller->CreateOffer(); auto contents = reoffer->description()->contents(); @@ -602,17 +605,14 @@ TEST_F(PeerConnectionJsepTest, auto callee = CreatePeerConnection(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); - callee->pc()->GetTransceivers()[0]->StopInternal(); - ASSERT_EQ(0u, callee->pc()->GetTransceivers().size()); + callee->pc()->GetTransceivers()[0]->Stop(); ASSERT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); EXPECT_TRUE(first_transceiver->stopped()); - // First transceivers aren't dissociated yet on caller side. + // First transceivers aren't dissociated yet. ASSERT_NE(absl::nullopt, first_transceiver->mid()); std::string first_mid = *first_transceiver->mid(); - // They are disassociated on callee side. - ASSERT_EQ(0u, callee->pc()->GetTransceivers().size()); + EXPECT_EQ(first_mid, callee->pc()->GetTransceivers()[0]->mid()); // New offer exchange with new transceivers that recycles the m section // correctly. @@ -630,11 +630,10 @@ TEST_F(PeerConnectionJsepTest, ASSERT_TRUE( caller->SetLocalDescription(CloneSessionDescription(offer.get()))); EXPECT_EQ(absl::nullopt, first_transceiver->mid()); - ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); - EXPECT_EQ(second_mid, caller->pc()->GetTransceivers()[0]->mid()); + EXPECT_EQ(second_mid, caller->pc()->GetTransceivers()[1]->mid()); ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); - ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); - EXPECT_EQ(second_mid, callee->pc()->GetTransceivers()[0]->mid()); + EXPECT_EQ(absl::nullopt, callee->pc()->GetTransceivers()[0]->mid()); + EXPECT_EQ(second_mid, callee->pc()->GetTransceivers()[1]->mid()); // The new answer should also recycle the m section correctly. auto answer = callee->CreateAnswer(); @@ -648,11 +647,13 @@ TEST_F(PeerConnectionJsepTest, callee->SetLocalDescription(CloneSessionDescription(answer.get()))); ASSERT_TRUE(caller->SetRemoteDescription(std::move(answer))); auto caller_transceivers = caller->pc()->GetTransceivers(); - ASSERT_EQ(1u, caller_transceivers.size()); - EXPECT_EQ(second_mid, caller_transceivers[0]->mid()); + ASSERT_EQ(2u, caller_transceivers.size()); + EXPECT_EQ(absl::nullopt, caller_transceivers[0]->mid()); + EXPECT_EQ(second_mid, caller_transceivers[1]->mid()); auto callee_transceivers = callee->pc()->GetTransceivers(); - ASSERT_EQ(1u, callee_transceivers.size()); - EXPECT_EQ(second_mid, callee_transceivers[0]->mid()); + ASSERT_EQ(2u, callee_transceivers.size()); + EXPECT_EQ(absl::nullopt, callee_transceivers[0]->mid()); + EXPECT_EQ(second_mid, callee_transceivers[1]->mid()); } // Test that creating/setting a local offer that recycles an m= section is @@ -663,7 +664,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->StopInternal(); + first_transceiver->Stop(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); caller->AddAudioTrack("audio2"); @@ -674,8 +675,7 @@ TEST_F(PeerConnectionJsepTest, CreateOfferRecyclesWhenOfferingTwice) { ASSERT_EQ(1u, offer_contents.size()); EXPECT_FALSE(offer_contents[0].rejected); ASSERT_TRUE(caller->SetLocalDescription(std::move(offer))); - ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); - EXPECT_FALSE(caller->pc()->GetTransceivers()[0]->stopped()); + EXPECT_FALSE(caller->pc()->GetTransceivers()[1]->stopped()); std::string second_mid = offer_contents[0].name; // Create another new offer and set the local description again without the @@ -690,9 +690,10 @@ 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(1u, caller_transceivers.size()); - EXPECT_EQ(second_mid, caller_transceivers[0]->mid()); - EXPECT_FALSE(caller_transceivers[0]->stopped()); + 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()); } // Test that the offer/answer and transceivers for both the caller and callee @@ -728,7 +729,7 @@ TEST_P(RecycleMediaSectionTest, CurrentLocalAndCurrentRemoteRejected) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); std::string first_mid = *first_transceiver->mid(); - first_transceiver->StopInternal(); + first_transceiver->Stop(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -755,9 +756,11 @@ 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(1u, callee_transceivers.size()); - EXPECT_EQ(second_mid, callee_transceivers[0]->mid()); - EXPECT_EQ(second_type_, callee_transceivers[0]->media_type()); + 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()); // The answer should have only one media section for the new transceiver. auto answer = callee->CreateAnswer(); @@ -774,8 +777,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(1u, caller->pc()->GetTransceivers().size()); - ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); } // Test that recycling works properly when a new transceiver recycles an m= @@ -790,7 +793,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->StopInternal(); + callee_first_transceiver->Stop(); // The answer will have a rejected m= section. ASSERT_TRUE( @@ -818,9 +821,11 @@ 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(1u, callee_transceivers.size()); - EXPECT_EQ(second_mid, callee_transceivers[0]->mid()); - EXPECT_EQ(second_type_, callee_transceivers[0]->media_type()); + 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()); // The answer should have only one media section for the new transceiver. auto answer = callee->CreateAnswer(); @@ -837,8 +842,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(1u, caller->pc()->GetTransceivers().size()); - ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); } // Test that recycling works properly when a new transceiver recycles an m= @@ -853,7 +858,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->StopInternal(); + callee_first_transceiver->Stop(); // The answer will have a rejected m= section. ASSERT_TRUE( @@ -881,9 +886,11 @@ 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(1u, caller_transceivers.size()); - EXPECT_EQ(second_mid, caller_transceivers[0]->mid()); - EXPECT_EQ(second_type_, caller_transceivers[0]->media_type()); + 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()); // The answer should have only one media section for the new transceiver. auto answer = caller->CreateAnswer(); @@ -900,8 +907,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(1u, callee->pc()->GetTransceivers().size()); - ASSERT_EQ(1u, caller->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); } // Test that a m= section is *not* recycled if the media section is only @@ -914,7 +921,7 @@ TEST_P(RecycleMediaSectionTest, PendingLocalRejectedAndNoRemote) { ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); std::string first_mid = *caller_first_transceiver->mid(); - caller_first_transceiver->StopInternal(); + caller_first_transceiver->Stop(); // The reoffer will have a rejected m= section. ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); @@ -952,7 +959,7 @@ TEST_P(RecycleMediaSectionTest, PendingLocalRejectedAndNotRejectedRemote) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); std::string first_mid = *caller_first_transceiver->mid(); - caller_first_transceiver->StopInternal(); + caller_first_transceiver->Stop(); // The reoffer will have a rejected m= section. ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); @@ -992,7 +999,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->StopInternal(); + caller_first_transceiver->Stop(); // The reoffer will have a rejected m= section. ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); @@ -1029,7 +1036,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->StopInternal(); + caller_first_transceiver->Stop(); // The reoffer will have a rejected m= section. ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); @@ -1073,7 +1080,7 @@ TEST_F(PeerConnectionJsepTest, DataChannelDoesNotRecycleMediaSection) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - transceiver->StopInternal(); + transceiver->Stop(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -1360,7 +1367,7 @@ TEST_F(PeerConnectionJsepTest, IncludeMsidEvenIfDirectionHasChanged) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - caller->pc()->GetTransceivers()[0]->SetDirectionWithError( + caller->pc()->GetTransceivers()[0]->SetDirection( RtpTransceiverDirection::kInactive); // The transceiver direction on both sides will turn to inactive. @@ -1388,7 +1395,7 @@ TEST_F(PeerConnectionJsepTest, RemoveMsidIfTransceiverStopped) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - transceiver->StopInternal(); + transceiver->Stop(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -1545,9 +1552,8 @@ TEST_F(PeerConnectionJsepTest, CurrentDirectionResetWhenRtpTransceiverStopped) { ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); ASSERT_TRUE(transceiver->current_direction()); - transceiver->StopInternal(); - EXPECT_EQ(transceiver->current_direction(), - RtpTransceiverDirection::kStopped); + transceiver->Stop(); + EXPECT_FALSE(transceiver->current_direction()); } // Test that you can't set an answer on a PeerConnection before setting the @@ -2033,7 +2039,7 @@ TEST_F(PeerConnectionJsepTest, RollbackLocalDirectionChange) { EXPECT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); callee->AddAudioTrack("a"); - callee->pc()->GetTransceivers()[0]->SetDirectionWithError( + callee->pc()->GetTransceivers()[0]->SetDirection( RtpTransceiverDirection::kSendOnly); EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u); @@ -2057,7 +2063,7 @@ TEST_F(PeerConnectionJsepTest, RollbackRemoteDirectionChange) { EXPECT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); // In stable make remote audio receive only. - caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly); + caller_transceiver->SetDirection(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 f078144d4f..3c117c3ecd 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]->StopInternal(); - transceivers[1]->StopInternal(); + transceivers[0]->Stop(); + transceivers[1]->Stop(); 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]->StopInternal(); - transceivers[1]->StopInternal(); + transceivers[0]->Stop(); + transceivers[1]->Stop(); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index e69a0882ae..9e4a816a45 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -370,25 +370,19 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, SetDirectionCallsOnTrack) { auto callee = CreatePeerConnection(); auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - EXPECT_TRUE( - transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive) - .ok()); + transceiver->SetDirection(RtpTransceiverDirection::kInactive); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); EXPECT_EQ(0u, callee->observer()->on_track_transceivers_.size()); - EXPECT_TRUE( - transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendOnly) - .ok()); + transceiver->SetDirection(RtpTransceiverDirection::kSendOnly); 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. - EXPECT_TRUE( - transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv) - .ok()); + transceiver->SetDirection(RtpTransceiverDirection::kSendRecv); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); @@ -407,10 +401,8 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, SetDirectionHoldCallsOnTrackTwice) { EXPECT_EQ(1u, callee->observer()->on_track_transceivers_.size()); // Put the call on hold by no longer receiving the track. - EXPECT_TRUE(callee->pc() - ->GetTransceivers()[0] - ->SetDirectionWithError(RtpTransceiverDirection::kInactive) - .ok()); + callee->pc()->GetTransceivers()[0]->SetDirection( + RtpTransceiverDirection::kInactive); ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); @@ -418,10 +410,8 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, SetDirectionHoldCallsOnTrackTwice) { // Resume the call by changing the direction to recvonly. This should call // OnTrack again on the callee side. - EXPECT_TRUE(callee->pc() - ->GetTransceivers()[0] - ->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly) - .ok()); + callee->pc()->GetTransceivers()[0]->SetDirection( + RtpTransceiverDirection::kRecvOnly); ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); EXPECT_EQ(0u, caller->observer()->on_track_transceivers_.size()); @@ -480,9 +470,7 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, EXPECT_EQ(0u, callee->observer()->remove_track_events_.size()); auto callee_transceiver = callee->pc()->GetTransceivers()[0]; - EXPECT_TRUE(callee_transceiver - ->SetDirectionWithError(RtpTransceiverDirection::kSendOnly) - .ok()); + callee_transceiver->SetDirection(RtpTransceiverDirection::kSendOnly); ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); EXPECT_EQ(1u, callee->observer()->add_track_events_.size()); @@ -1421,7 +1409,7 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); caller->observer()->clear_negotiation_needed(); - transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive); + transceiver->SetDirection(RtpTransceiverDirection::kInactive); EXPECT_TRUE(caller->observer()->negotiation_needed()); } @@ -1434,7 +1422,7 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); caller->observer()->clear_negotiation_needed(); - transceiver->SetDirectionWithError(transceiver->direction()); + transceiver->SetDirection(transceiver->direction()); EXPECT_FALSE(caller->observer()->negotiation_needed()); } @@ -1445,71 +1433,13 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, auto caller = CreatePeerConnection(); auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - transceiver->StopInternal(); + transceiver->Stop(); caller->observer()->clear_negotiation_needed(); - transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive); + transceiver->SetDirection(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 8822a980f7..42bdae17b9 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->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); + transceiver->SetDirection(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->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); + transceiver->SetDirection(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->SetDirectionWithError(RtpTransceiverDirection::kSendRecv); + transceiver->SetDirection(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 c5d642b685..8fbfca1f98 100644 --- a/pc/rtp_media_utils.cc +++ b/pc/rtp_media_utils.cc @@ -42,7 +42,6 @@ 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 1430e299c4..3c56bf0724 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc @@ -184,15 +184,6 @@ 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 c343ff085d..15d47fd90d 100644 --- a/pc/rtp_sender.h +++ b/pc/rtp_sender.h @@ -69,8 +69,6 @@ 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. @@ -154,8 +152,6 @@ 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 @@ -184,7 +180,6 @@ 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 8d14e46b4b..b4e500bbc8 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -97,19 +97,10 @@ 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) - : thread_(GetCurrentTaskQueueOrThread()), - unified_plan_(false), - media_type_(media_type) { + : unified_plan_(false), media_type_(media_type) { RTC_DCHECK(media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO); } @@ -120,8 +111,7 @@ RtpTransceiver::RtpTransceiver( receiver, cricket::ChannelManager* channel_manager, std::vector header_extensions_offered) - : thread_(GetCurrentTaskQueueOrThread()), - unified_plan_(true), + : unified_plan_(true), media_type_(sender->media_type()), channel_manager_(channel_manager), header_extensions_to_offer_(std::move(header_extensions_offered)) { @@ -133,7 +123,7 @@ RtpTransceiver::RtpTransceiver( } RtpTransceiver::~RtpTransceiver() { - StopInternal(); + Stop(); } void RtpTransceiver::SetChannel(cricket::ChannelInterface* channel) { @@ -287,51 +277,23 @@ 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) { - // 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 (stopped()) { + 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."); + if (new_direction == direction_) { + return; } - direction_ = new_direction; SignalNegotiationNeeded(); - - return RTCError::OK(); } absl::optional RtpTransceiver::current_direction() const { - if (unified_plan_ && stopping()) - return webrtc::RtpTransceiverDirection::kStopped; - return current_direction_; } @@ -340,69 +302,14 @@ absl::optional RtpTransceiver::fired_direction() return fired_direction_; } -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_) +void RtpTransceiver::Stop() { + for (const auto& sender : senders_) { sender->internal()->Stop(); - - // 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. + for (const auto& receiver : receivers_) { + receiver->internal()->Stop(); + } 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; } @@ -496,8 +403,4 @@ 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 980d64ca76..be46ccfd5c 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -173,10 +173,6 @@ 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; @@ -187,15 +183,11 @@ 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; - RTCError StopStandard() override; - void StopInternal() override; + void Stop() override; RTCError SetCodecPreferences( rtc::ArrayView codecs) override; std::vector codec_preferences() const override { @@ -209,10 +201,7 @@ 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>> @@ -222,8 +211,6 @@ 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_; @@ -246,14 +233,11 @@ 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(webrtc::RTCError, StopStandard) -PROXY_METHOD0(void, StopInternal) +PROXY_METHOD0(void, Stop) PROXY_METHOD1(webrtc::RTCError, SetCodecPreferences, rtc::ArrayView) diff --git a/pc/rtp_transceiver_unittest.cc b/pc/rtp_transceiver_unittest.cc index 78abfff8b9..e3f05c4dd9 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.StopInternal(); + transceiver.Stop(); EXPECT_EQ(&channel1, transceiver.channel()); cricket::MockChannelInterface channel2; @@ -71,7 +71,7 @@ TEST(RtpTransceiverTest, CanUnsetChannelOnStoppedTransceiver) { EXPECT_EQ(&channel, transceiver.channel()); // Stop the transceiver. - transceiver.StopInternal(); + transceiver.Stop(); 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 5e7670ebf0..1a31c5dac6 100644 --- a/pc/test/mock_rtp_sender_internal.h +++ b/pc/test/mock_rtp_sender_internal.h @@ -65,17 +65,23 @@ class MockRtpSenderInternal : public RtpSenderInternal { (const, override)); // RtpSenderInternal methods. - 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()); + 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)); }; } // namespace webrtc diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index 4af121ddc3..7a2aeae3a4 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -1568,8 +1568,6 @@ 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; @@ -1582,7 +1580,9 @@ 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 021cc90bc5..64d8eb41d1 100644 --- a/sdk/android/api/org/webrtc/RtpTransceiver.java +++ b/sdk/android/api/org/webrtc/RtpTransceiver.java @@ -206,34 +206,13 @@ public class RtpTransceiver { } /** - * The Stop method will for the time being call the StopInternal method. - * After a migration procedure, stop() will be equivalent to StopStandard. + * 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 */ public void stop() { checkRtpTransceiverExists(); - 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); + nativeStop(nativeRtpTransceiver); } @CalledByNative @@ -258,8 +237,7 @@ 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 nativeStopInternal(long rtpTransceiver); - private static native void nativeStopStandard(long rtpTransceiver); + private static native void nativeStop(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 a0b3c20fdd..7d8cfdef49 100644 --- a/sdk/android/src/jni/pc/rtp_transceiver.cc +++ b/sdk/android/src/jni/pc/rtp_transceiver.cc @@ -139,16 +139,9 @@ ScopedJavaLocalRef JNI_RtpTransceiver_CurrentDirection( : nullptr; } -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_Stop(JNIEnv* jni, + jlong j_rtp_transceiver_pointer) { + reinterpret_cast(j_rtp_transceiver_pointer)->Stop(); } void JNI_RtpTransceiver_SetDirection( diff --git a/sdk/objc/api/peerconnection/RTCRtpTransceiver.h b/sdk/objc/api/peerconnection/RTCRtpTransceiver.h index 17054f5e43..f8996ccafb 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)stopInternal; +- (void)stop; @end diff --git a/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm b/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm index 5d0d8eda37..2995e5fceb 100644 --- a/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm +++ b/sdk/objc/api/peerconnection/RTCRtpTransceiver.mm @@ -90,8 +90,8 @@ } } -- (void)stopInternal { - _nativeRtpTransceiver->StopInternal(); +- (void)stop { + _nativeRtpTransceiver->Stop(); } - (NSString *)description {