From dc4eb8c5b3c996389046694d3b4fdb2ab315376f Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Thu, 12 May 2016 08:14:50 -0700 Subject: [PATCH] Refactoring some tests in peerconnectioninterface_unittest.cc. Some tests were passing in a local description created from hard-coded SDP strings, which won't work in the future (since some attributes such as the fingerprint and ICE ufrag/pwd are non-modifiable). These tests now do the typical approach of calling CreateOffer and modifying the result if necessary. Also added some non-const versions of the SessionDescription accessor helper functions, since that makes it much easier to modify a SessionDescription. Previous alternatives were re-implementing the helper methods from scratch, or converting the description to SDP, modifying it, and converting it back. R=tommi@webrtc.org Review URL: https://codereview.webrtc.org/1966333002 . Cr-Commit-Position: refs/heads/master@{#12704} --- .../api/peerconnectioninterface_unittest.cc | 186 ++++++++++-------- webrtc/pc/mediasession.cc | 80 +++++++- webrtc/pc/mediasession.h | 15 ++ 3 files changed, 194 insertions(+), 87 deletions(-) diff --git a/webrtc/api/peerconnectioninterface_unittest.cc b/webrtc/api/peerconnectioninterface_unittest.cc index 1386e8c210..795a66d7f6 100644 --- a/webrtc/api/peerconnectioninterface_unittest.cc +++ b/webrtc/api/peerconnectioninterface_unittest.cc @@ -239,8 +239,9 @@ static const char kSdpStringMs1Video1[] = return; \ } -using rtc::scoped_refptr; using ::testing::Exactly; +using cricket::StreamParams; +using rtc::scoped_refptr; using webrtc::AudioSourceInterface; using webrtc::AudioTrack; using webrtc::AudioTrackInterface; @@ -248,6 +249,7 @@ using webrtc::DataBuffer; using webrtc::DataChannelInterface; using webrtc::FakeConstraints; using webrtc::IceCandidateInterface; +using webrtc::JsepSessionDescription; using webrtc::MediaConstraintsInterface; using webrtc::MediaStream; using webrtc::MediaStreamInterface; @@ -325,12 +327,26 @@ bool ContainsSender( return false; } +// Check if |senders| contains the specified sender, by id and stream id. +bool ContainsSender( + const std::vector>& senders, + const std::string& id, + const std::string& stream_id) { + for (const auto& sender : senders) { + if (sender->id() == id && sender->stream_id() == stream_id) { + return true; + } + } + return false; +} + // Create a collection of streams. // CreateStreamCollection(1) creates a collection that // correspond to kSdpStringWithStream1. // CreateStreamCollection(2) correspond to kSdpStringWithStream1And2. rtc::scoped_refptr CreateStreamCollection( - int number_of_streams) { + int number_of_streams, + int tracks_per_stream) { rtc::scoped_refptr local_collection( StreamCollection::Create()); @@ -338,16 +354,19 @@ rtc::scoped_refptr CreateStreamCollection( rtc::scoped_refptr stream( webrtc::MediaStream::Create(kStreams[i])); - // Add a local audio track. - rtc::scoped_refptr audio_track( - webrtc::AudioTrack::Create(kAudioTracks[i], nullptr)); - stream->AddTrack(audio_track); + for (int j = 0; j < tracks_per_stream; ++j) { + // Add a local audio track. + rtc::scoped_refptr audio_track( + webrtc::AudioTrack::Create(kAudioTracks[i * tracks_per_stream + j], + nullptr)); + stream->AddTrack(audio_track); - // Add a local video track. - rtc::scoped_refptr video_track( - webrtc::VideoTrack::Create(kVideoTracks[i], - webrtc::FakeVideoTrackSource::Create())); - stream->AddTrack(video_track); + // Add a local video track. + rtc::scoped_refptr video_track( + webrtc::VideoTrack::Create(kVideoTracks[i * tracks_per_stream + j], + webrtc::FakeVideoTrackSource::Create())); + stream->AddTrack(video_track); + } local_collection->AddStream(stream); } @@ -1984,7 +2003,7 @@ TEST_F(PeerConnectionInterfaceTest, UpdateRemoteStreams) { CreatePeerConnection(&constraints); CreateAndSetRemoteOffer(kSdpStringWithStream1); - rtc::scoped_refptr reference(CreateStreamCollection(1)); + rtc::scoped_refptr reference(CreateStreamCollection(1, 1)); EXPECT_TRUE( CompareStreamCollections(observer_.remote_streams(), reference.get())); MediaStreamInterface* remote_stream = observer_.remote_streams()->at(0); @@ -1994,7 +2013,7 @@ TEST_F(PeerConnectionInterfaceTest, UpdateRemoteStreams) { // MediaStream. CreateAndSetRemoteOffer(kSdpStringWithStream1And2); - rtc::scoped_refptr reference2(CreateStreamCollection(2)); + rtc::scoped_refptr reference2(CreateStreamCollection(2, 1)); EXPECT_TRUE( CompareStreamCollections(observer_.remote_streams(), reference2.get())); } @@ -2260,7 +2279,7 @@ TEST_F(PeerConnectionInterfaceTest, VerifyDefaultStreamIsNotCreated) { true); CreatePeerConnection(&constraints); CreateAndSetRemoteOffer(kSdpStringWithStream1); - rtc::scoped_refptr reference(CreateStreamCollection(1)); + rtc::scoped_refptr reference(CreateStreamCollection(1, 1)); EXPECT_TRUE( CompareStreamCollections(observer_.remote_streams(), reference.get())); @@ -2277,16 +2296,15 @@ TEST_F(PeerConnectionInterfaceTest, LocalDescriptionChanged) { constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, true); CreatePeerConnection(&constraints); - // Create an offer just to ensure we have an identity before we manually - // call SetLocalDescription. - std::unique_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(&throwaway, nullptr)); - std::unique_ptr desc_1 = - CreateSessionDescriptionAndReference(2, 2); + // Create an offer with 1 stream with 2 tracks of each type. + rtc::scoped_refptr stream_collection = + CreateStreamCollection(1, 2); + pc_->AddStream(stream_collection->at(0)); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, nullptr)); + EXPECT_TRUE(DoSetLocalDescription(offer.release())); - pc_->AddStream(reference_collection_->at(0)); - EXPECT_TRUE(DoSetLocalDescription(desc_1.release())); auto senders = pc_->GetSenders(); EXPECT_EQ(4u, senders.size()); EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0])); @@ -2295,11 +2313,12 @@ TEST_F(PeerConnectionInterfaceTest, LocalDescriptionChanged) { EXPECT_TRUE(ContainsSender(senders, kVideoTracks[1])); // Remove an audio and video track. - pc_->RemoveStream(reference_collection_->at(0)); - std::unique_ptr desc_2 = - CreateSessionDescriptionAndReference(1, 1); - pc_->AddStream(reference_collection_->at(0)); - EXPECT_TRUE(DoSetLocalDescription(desc_2.release())); + pc_->RemoveStream(stream_collection->at(0)); + stream_collection = CreateStreamCollection(1, 1); + pc_->AddStream(stream_collection->at(0)); + ASSERT_TRUE(DoCreateOffer(&offer, nullptr)); + EXPECT_TRUE(DoSetLocalDescription(offer.release())); + senders = pc_->GetSenders(); EXPECT_EQ(2u, senders.size()); EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0])); @@ -2316,19 +2335,20 @@ TEST_F(PeerConnectionInterfaceTest, constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, true); CreatePeerConnection(&constraints); - // Create an offer just to ensure we have an identity before we manually - // call SetLocalDescription. - std::unique_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(&throwaway, nullptr)); - std::unique_ptr desc_1 = - CreateSessionDescriptionAndReference(2, 2); + rtc::scoped_refptr stream_collection = + CreateStreamCollection(1, 2); + // Add a stream to create the offer, but remove it afterwards. + pc_->AddStream(stream_collection->at(0)); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, nullptr)); + pc_->RemoveStream(stream_collection->at(0)); - EXPECT_TRUE(DoSetLocalDescription(desc_1.release())); + EXPECT_TRUE(DoSetLocalDescription(offer.release())); auto senders = pc_->GetSenders(); EXPECT_EQ(0u, senders.size()); - pc_->AddStream(reference_collection_->at(0)); + pc_->AddStream(stream_collection->at(0)); senders = pc_->GetSenders(); EXPECT_EQ(4u, senders.size()); EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0])); @@ -2345,37 +2365,44 @@ TEST_F(PeerConnectionInterfaceTest, constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, true); CreatePeerConnection(&constraints); - // Create an offer just to ensure we have an identity before we manually - // call SetLocalDescription. - std::unique_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(&throwaway, nullptr)); - std::unique_ptr desc = - CreateSessionDescriptionAndReference(1, 1); - std::string sdp; - desc->ToString(&sdp); + rtc::scoped_refptr stream_collection = + CreateStreamCollection(2, 1); + pc_->AddStream(stream_collection->at(0)); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, nullptr)); + // Grab a copy of the offer before it gets passed into the PC. + std::unique_ptr modified_offer( + new JsepSessionDescription(JsepSessionDescription::kOffer)); + modified_offer->Initialize(offer->description()->Copy(), offer->session_id(), + offer->session_version()); + EXPECT_TRUE(DoSetLocalDescription(offer.release())); - pc_->AddStream(reference_collection_->at(0)); - EXPECT_TRUE(DoSetLocalDescription(desc.release())); auto senders = pc_->GetSenders(); EXPECT_EQ(2u, senders.size()); EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0])); EXPECT_TRUE(ContainsSender(senders, kVideoTracks[0])); // Change the ssrc of the audio and video track. - std::string ssrc_org = "a=ssrc:1"; - std::string ssrc_to = "a=ssrc:97"; - rtc::replace_substrs(ssrc_org.c_str(), ssrc_org.length(), ssrc_to.c_str(), - ssrc_to.length(), &sdp); - ssrc_org = "a=ssrc:2"; - ssrc_to = "a=ssrc:98"; - rtc::replace_substrs(ssrc_org.c_str(), ssrc_org.length(), ssrc_to.c_str(), - ssrc_to.length(), &sdp); - std::unique_ptr updated_desc( - webrtc::CreateSessionDescription(SessionDescriptionInterface::kOffer, sdp, - nullptr)); + cricket::MediaContentDescription* desc = + cricket::GetFirstAudioContentDescription(modified_offer->description()); + ASSERT_TRUE(desc != NULL); + for (StreamParams& stream : desc->mutable_streams()) { + for (unsigned int& ssrc : stream.ssrcs) { + ++ssrc; + } + } - EXPECT_TRUE(DoSetLocalDescription(updated_desc.release())); + desc = + cricket::GetFirstVideoContentDescription(modified_offer->description()); + ASSERT_TRUE(desc != NULL); + for (StreamParams& stream : desc->mutable_streams()) { + for (unsigned int& ssrc : stream.ssrcs) { + ++ssrc; + } + } + + EXPECT_TRUE(DoSetLocalDescription(modified_offer.release())); senders = pc_->GetSenders(); EXPECT_EQ(2u, senders.size()); EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0])); @@ -2392,43 +2419,36 @@ TEST_F(PeerConnectionInterfaceTest, constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, true); CreatePeerConnection(&constraints); - // Create an offer just to ensure we have an identity before we manually - // call SetLocalDescription. - std::unique_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(&throwaway, nullptr)); - std::unique_ptr desc = - CreateSessionDescriptionAndReference(1, 1); - std::string sdp; - desc->ToString(&sdp); + rtc::scoped_refptr stream_collection = + CreateStreamCollection(2, 1); + pc_->AddStream(stream_collection->at(0)); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, nullptr)); + EXPECT_TRUE(DoSetLocalDescription(offer.release())); - pc_->AddStream(reference_collection_->at(0)); - EXPECT_TRUE(DoSetLocalDescription(desc.release())); auto senders = pc_->GetSenders(); EXPECT_EQ(2u, senders.size()); - EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0])); - EXPECT_TRUE(ContainsSender(senders, kVideoTracks[0])); + EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0], kStreams[0])); + EXPECT_TRUE(ContainsSender(senders, kVideoTracks[0], kStreams[0])); // Add a new MediaStream but with the same tracks as in the first stream. rtc::scoped_refptr stream_1( webrtc::MediaStream::Create(kStreams[1])); - stream_1->AddTrack(reference_collection_->at(0)->GetVideoTracks()[0]); - stream_1->AddTrack(reference_collection_->at(0)->GetAudioTracks()[0]); + stream_1->AddTrack(stream_collection->at(0)->GetVideoTracks()[0]); + stream_1->AddTrack(stream_collection->at(0)->GetAudioTracks()[0]); pc_->AddStream(stream_1); - // Replace msid in the original SDP. - rtc::replace_substrs(kStreams[0], strlen(kStreams[0]), kStreams[1], - strlen(kStreams[1]), &sdp); + ASSERT_TRUE(DoCreateOffer(&offer, nullptr)); + EXPECT_TRUE(DoSetLocalDescription(offer.release())); - std::unique_ptr updated_desc( - webrtc::CreateSessionDescription(SessionDescriptionInterface::kOffer, sdp, - nullptr)); - - EXPECT_TRUE(DoSetLocalDescription(updated_desc.release())); - senders = pc_->GetSenders(); - EXPECT_EQ(2u, senders.size()); - EXPECT_TRUE(ContainsSender(senders, kAudioTracks[0])); - EXPECT_TRUE(ContainsSender(senders, kVideoTracks[0])); + auto new_senders = pc_->GetSenders(); + // Should be the same senders as before, but with updated stream id. + // Note that this behavior is subject to change in the future. + // We may decide the PC should ignore existing tracks in AddStream. + EXPECT_EQ(senders, new_senders); + EXPECT_TRUE(ContainsSender(new_senders, kAudioTracks[0], kStreams[1])); + EXPECT_TRUE(ContainsSender(new_senders, kVideoTracks[0], kStreams[1])); } // The PeerConnectionMediaConfig tests below verify that configuration diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc index 52abfe855f..0fa20d8bd5 100644 --- a/webrtc/pc/mediasession.cc +++ b/webrtc/pc/mediasession.cc @@ -1919,10 +1919,9 @@ bool IsDataContent(const ContentInfo* content) { const ContentInfo* GetFirstMediaContent(const ContentInfos& contents, MediaType media_type) { - for (ContentInfos::const_iterator content = contents.begin(); - content != contents.end(); content++) { - if (IsMediaContentOfType(&*content, media_type)) { - return &*content; + for (const ContentInfo& content : contents) { + if (IsMediaContentOfType(&content, media_type)) { + return &content; } } return nullptr; @@ -1986,4 +1985,77 @@ const DataContentDescription* GetFirstDataContentDescription( GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_DATA)); } +// +// Non-const versions of the above functions. +// + +ContentInfo* GetFirstMediaContent(ContentInfos& contents, + MediaType media_type) { + for (ContentInfo& content : contents) { + if (IsMediaContentOfType(&content, media_type)) { + return &content; + } + } + return nullptr; +} + +ContentInfo* GetFirstAudioContent(ContentInfos& contents) { + return GetFirstMediaContent(contents, MEDIA_TYPE_AUDIO); +} + +ContentInfo* GetFirstVideoContent(ContentInfos& contents) { + return GetFirstMediaContent(contents, MEDIA_TYPE_VIDEO); +} + +ContentInfo* GetFirstDataContent(ContentInfos& contents) { + return GetFirstMediaContent(contents, MEDIA_TYPE_DATA); +} + +static ContentInfo* GetFirstMediaContent(SessionDescription* sdesc, + MediaType media_type) { + if (sdesc == nullptr) { + return nullptr; + } + + return GetFirstMediaContent(sdesc->contents(), media_type); +} + +ContentInfo* GetFirstAudioContent(SessionDescription* sdesc) { + return GetFirstMediaContent(sdesc, MEDIA_TYPE_AUDIO); +} + +ContentInfo* GetFirstVideoContent(SessionDescription* sdesc) { + return GetFirstMediaContent(sdesc, MEDIA_TYPE_VIDEO); +} + +ContentInfo* GetFirstDataContent(SessionDescription* sdesc) { + return GetFirstMediaContent(sdesc, MEDIA_TYPE_DATA); +} + +MediaContentDescription* GetFirstMediaContentDescription( + SessionDescription* sdesc, + MediaType media_type) { + ContentInfo* content = GetFirstMediaContent(sdesc, media_type); + ContentDescription* description = content ? content->description : NULL; + return static_cast(description); +} + +AudioContentDescription* GetFirstAudioContentDescription( + SessionDescription* sdesc) { + return static_cast( + GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_AUDIO)); +} + +VideoContentDescription* GetFirstVideoContentDescription( + SessionDescription* sdesc) { + return static_cast( + GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_VIDEO)); +} + +DataContentDescription* GetFirstDataContentDescription( + SessionDescription* sdesc) { + return static_cast( + GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_DATA)); +} + } // namespace cricket diff --git a/webrtc/pc/mediasession.h b/webrtc/pc/mediasession.h index 39ac26bd8d..22291c42a6 100644 --- a/webrtc/pc/mediasession.h +++ b/webrtc/pc/mediasession.h @@ -532,6 +532,21 @@ const VideoContentDescription* GetFirstVideoContentDescription( const SessionDescription* sdesc); const DataContentDescription* GetFirstDataContentDescription( const SessionDescription* sdesc); +// Non-const versions of the above functions. +// Useful when modifying an existing description. +ContentInfo* GetFirstMediaContent(ContentInfos& contents, MediaType media_type); +ContentInfo* GetFirstAudioContent(ContentInfos& contents); +ContentInfo* GetFirstVideoContent(ContentInfos& contents); +ContentInfo* GetFirstDataContent(ContentInfos& contents); +ContentInfo* GetFirstAudioContent(SessionDescription* sdesc); +ContentInfo* GetFirstVideoContent(SessionDescription* sdesc); +ContentInfo* GetFirstDataContent(SessionDescription* sdesc); +AudioContentDescription* GetFirstAudioContentDescription( + SessionDescription* sdesc); +VideoContentDescription* GetFirstVideoContentDescription( + SessionDescription* sdesc); +DataContentDescription* GetFirstDataContentDescription( + SessionDescription* sdesc); void GetSupportedAudioCryptoSuites(std::vector* crypto_suites); void GetSupportedVideoCryptoSuites(std::vector* crypto_suites);