From 10aeb2a5dc2e84db0eea0ebd64384be3834ca920 Mon Sep 17 00:00:00 2001 From: "Piotr (Peter) Slatala" Date: Wed, 14 Nov 2018 10:57:24 -0800 Subject: [PATCH] MediaTransportTests should use audio-only peer connection. Currently (and this has to change), media transport is created two times if audio&video is used (even if bundling is enabled). The second time it's destroyed really quickly (but given lack of 'Connect' method, the connection has already started). This change adds a TODO and modifies existing tests to prevent creation of 2 media transports. Bug: webrtc:9719 Change-Id: I872e98dcd10685beb0326d501f0e0abf36c0fdfc Reviewed-on: https://webrtc-review.googlesource.com/c/110887 Reviewed-by: Anton Sukhanov Reviewed-by: Steve Anton Commit-Queue: Peter Slatala Cr-Commit-Position: refs/heads/master@{#25660} --- pc/jseptransportcontroller.cc | 4 +++ pc/peerconnection_media_unittest.cc | 42 ++++++++++++++--------------- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index cb99686a98..9457ed7893 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -762,6 +762,10 @@ bool JsepTransportController::HandleBundledContent( // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first, // then destroy the cricket::JsepTransport. if (SetTransportForMid(content_info.name, jsep_transport)) { + // TODO(bugs.webrtc.org/9719) For media transport this is far from ideal, + // because it means that we first create media transport and start + // connecting it, and then we destroy it. We will need to address it before + // video path is enabled. MaybeDestroyJsepTransport(content_info.name); return true; } diff --git a/pc/peerconnection_media_unittest.cc b/pc/peerconnection_media_unittest.cc index 3c069aea7a..dd39141a0c 100644 --- a/pc/peerconnection_media_unittest.cc +++ b/pc/peerconnection_media_unittest.cc @@ -112,6 +112,18 @@ class PeerConnectionMediaBaseTest : public ::testing::Test { return wrapper; } + // Accepts the same arguments as CreatePeerConnection and adds default audio + // track (but no video). + template + WrapperPtr CreatePeerConnectionWithAudio(Args&&... args) { + auto wrapper = CreatePeerConnection(std::forward(args)...); + if (!wrapper) { + return nullptr; + } + wrapper->AddAudioTrack("a"); + return wrapper; + } + // Accepts the same arguments as CreatePeerConnection and adds default audio // and video tracks. template @@ -1096,8 +1108,8 @@ TEST_P(PeerConnectionMediaTest, MediaTransportPropagatedToVoiceEngine) { // Force SDES. config.enable_dtls_srtp = false; - auto caller = CreatePeerConnectionWithAudioVideo(config); - auto callee = CreatePeerConnectionWithAudioVideo(config); + auto caller = CreatePeerConnectionWithAudio(config); + auto callee = CreatePeerConnectionWithAudio(config); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto answer = callee->CreateAnswer(); @@ -1120,13 +1132,9 @@ TEST_P(PeerConnectionMediaTest, MediaTransportPropagatedToVoiceEngine) { EXPECT_TRUE(caller_voice_media_transport->is_caller()); EXPECT_FALSE(callee_voice_media_transport->is_caller()); - // TODO(sukhanov): Propagate media transport to video channel. This test - // will fail once media transport is propagated to video channel and it will - // serve as a reminder to add a test for video channel propagation. - auto caller_video = caller->media_engine()->GetVideoChannel(0); - auto callee_video = callee->media_engine()->GetVideoChannel(0); - ASSERT_EQ(nullptr, caller_video->media_transport()); - ASSERT_EQ(nullptr, callee_video->media_transport()); + // TODO(sukhanov): Propagate media transport to video channel. + // This test does NOT set up video channels, because currently it causes + // us to create two media transports. } TEST_P(PeerConnectionMediaTest, MediaTransportOnlyForDataChannels) { @@ -1138,8 +1146,8 @@ TEST_P(PeerConnectionMediaTest, MediaTransportOnlyForDataChannels) { // Force SDES. config.enable_dtls_srtp = false; - auto caller = CreatePeerConnectionWithAudioVideo(config); - auto callee = CreatePeerConnectionWithAudioVideo(config); + auto caller = CreatePeerConnectionWithAudio(config); + auto callee = CreatePeerConnectionWithAudio(config); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); @@ -1165,8 +1173,8 @@ TEST_P(PeerConnectionMediaTest, MediaTransportForMediaAndDataChannels) { // Force SDES. config.enable_dtls_srtp = false; - auto caller = CreatePeerConnectionWithAudioVideo(config); - auto callee = CreatePeerConnectionWithAudioVideo(config); + auto caller = CreatePeerConnectionWithAudio(config); + auto callee = CreatePeerConnectionWithAudio(config); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); @@ -1187,14 +1195,6 @@ TEST_P(PeerConnectionMediaTest, MediaTransportForMediaAndDataChannels) { // Make sure media transport is created with correct is_caller. EXPECT_TRUE(caller_voice_media_transport->is_caller()); EXPECT_FALSE(callee_voice_media_transport->is_caller()); - - // TODO(sukhanov): Propagate media transport to video channel. This test - // will fail once media transport is propagated to video channel and it will - // serve as a reminder to add a test for video channel propagation. - auto caller_video = caller->media_engine()->GetVideoChannel(0); - auto callee_video = callee->media_engine()->GetVideoChannel(0); - ASSERT_EQ(nullptr, caller_video->media_transport()); - ASSERT_EQ(nullptr, callee_video->media_transport()); } TEST_P(PeerConnectionMediaTest, MediaTransportNotPropagatedToVoiceEngine) {