From c80741f8957b537e968397ac54ff5b5df8a2c318 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Thu, 22 Oct 2015 13:14:45 -0700 Subject: [PATCH] Fixing some issues with the direction attribute of m-lines in offers. By default, we'll now offer to receive if already receiving (meaning that the last remote description contained a track). Also, m-lines that are neither receiving nor sending are now correctly marked "inactive". Also moved some logic relating to default tracks out of webrtcsdp.cc, such that now the direction seen by upper layers will always be consistent with the consumed/produced SDP. BUG=528089 Review URL: https://codereview.webrtc.org/1406803004 Cr-Commit-Position: refs/heads/master@{#10376} --- talk/app/webrtc/peerconnection.cc | 57 +++--- talk/app/webrtc/peerconnection.h | 2 - .../peerconnectioninterface_unittest.cc | 166 ++++++++++++------ talk/app/webrtc/webrtcsdp.cc | 35 +--- talk/app/webrtc/webrtcsdp_unittest.cc | 4 + talk/app/webrtc/webrtcsession_unittest.cc | 24 ++- talk/session/media/mediasession.cc | 35 +++- webrtc/p2p/base/sessiondescription.h | 5 + 8 files changed, 216 insertions(+), 112 deletions(-) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index 3ddb3560de..fecc19ba0e 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -463,27 +463,16 @@ bool ConvertRtcOptionsForOffer( return false; } - // According to the spec, offer to receive audio/video if the constraint is - // not set and there are send streams. - if (rtc_options.offer_to_receive_audio == RTCOfferAnswerOptions::kUndefined) { - session_options->recv_audio = - session_options->HasSendMediaStream(cricket::MEDIA_TYPE_AUDIO); - } else { + if (rtc_options.offer_to_receive_audio != RTCOfferAnswerOptions::kUndefined) { session_options->recv_audio = (rtc_options.offer_to_receive_audio > 0); } - if (rtc_options.offer_to_receive_video == RTCOfferAnswerOptions::kUndefined) { - session_options->recv_video = - session_options->HasSendMediaStream(cricket::MEDIA_TYPE_VIDEO); - } else { + if (rtc_options.offer_to_receive_video != RTCOfferAnswerOptions::kUndefined) { session_options->recv_video = (rtc_options.offer_to_receive_video > 0); } session_options->vad_enabled = rtc_options.voice_activity_detection; session_options->transport_options.ice_restart = rtc_options.ice_restart; - session_options->bundle_enabled = - rtc_options.use_rtp_mux && - (session_options->has_audio() || session_options->has_video() || - session_options->has_data()); + session_options->bundle_enabled = rtc_options.use_rtp_mux; return true; } @@ -525,10 +514,6 @@ bool ParseConstraintsForAnswer(const MediaConstraintsInterface* constraints, // kUseRtpMux defaults to true according to spec. session_options->bundle_enabled = true; } - session_options->bundle_enabled = - session_options->bundle_enabled && - (session_options->has_audio() || session_options->has_video() || - session_options->has_data()); if (FindConstraint(constraints, MediaConstraintsInterface::kIceRestart, &value, &mandatory_constraints_satisfied)) { @@ -1043,8 +1028,8 @@ void PeerConnection::SetRemoteDescription( audio_content->description); UpdateRemoteStreamsList(GetActiveStreams(desc), desc->type(), new_streams); remote_info_.default_audio_track_needed = - MediaContentDirectionHasSend(desc->direction()) && - desc->streams().empty(); + !remote_desc->msid_supported() && desc->streams().empty() && + MediaContentDirectionHasSend(desc->direction()); } // Find all video rtp streams and create corresponding remote VideoTracks @@ -1056,8 +1041,8 @@ void PeerConnection::SetRemoteDescription( video_content->description); UpdateRemoteStreamsList(GetActiveStreams(desc), desc->type(), new_streams); remote_info_.default_video_track_needed = - MediaContentDirectionHasSend(desc->direction()) && - desc->streams().empty(); + !remote_desc->msid_supported() && desc->streams().empty() && + MediaContentDirectionHasSend(desc->direction()); } // Update the DataChannels with the information from the remote peer. @@ -1396,12 +1381,28 @@ void PeerConnection::PostCreateSessionDescriptionFailure( bool PeerConnection::GetOptionsForOffer( const PeerConnectionInterface::RTCOfferAnswerOptions& rtc_options, cricket::MediaSessionOptions* session_options) { - SetStreams(session_options, local_streams_, rtp_data_channels_); - if (!ConvertRtcOptionsForOffer(rtc_options, session_options)) { return false; } + SetStreams(session_options, local_streams_, rtp_data_channels_); + // Offer to receive audio/video if the constraint is not set and there are + // send streams, or we're currently receiving. + if (rtc_options.offer_to_receive_audio == RTCOfferAnswerOptions::kUndefined) { + session_options->recv_audio = + session_options->HasSendMediaStream(cricket::MEDIA_TYPE_AUDIO) || + !remote_audio_tracks_.empty(); + } + if (rtc_options.offer_to_receive_video == RTCOfferAnswerOptions::kUndefined) { + session_options->recv_video = + session_options->HasSendMediaStream(cricket::MEDIA_TYPE_VIDEO) || + !remote_video_tracks_.empty(); + } + session_options->bundle_enabled = + session_options->bundle_enabled && + (session_options->has_audio() || session_options->has_video() || + session_options->has_data()); + if (session_->data_channel_type() == cricket::DCT_SCTP && HasDataChannels()) { session_options->data_channel_type = cricket::DCT_SCTP; } @@ -1411,14 +1412,18 @@ bool PeerConnection::GetOptionsForOffer( bool PeerConnection::GetOptionsForAnswer( const MediaConstraintsInterface* constraints, cricket::MediaSessionOptions* session_options) { - SetStreams(session_options, local_streams_, rtp_data_channels_); session_options->recv_audio = false; session_options->recv_video = false; - if (!ParseConstraintsForAnswer(constraints, session_options)) { return false; } + SetStreams(session_options, local_streams_, rtp_data_channels_); + session_options->bundle_enabled = + session_options->bundle_enabled && + (session_options->has_audio() || session_options->has_video() || + session_options->has_data()); + // RTP data channel is handled in MediaSessionOptions::AddStream. SCTP streams // are not signaled in the SDP so does not go through that path and must be // handled here. diff --git a/talk/app/webrtc/peerconnection.h b/talk/app/webrtc/peerconnection.h index c47f90372e..2d388ae9f9 100644 --- a/talk/app/webrtc/peerconnection.h +++ b/talk/app/webrtc/peerconnection.h @@ -51,8 +51,6 @@ typedef std::vector // Populates |session_options| from |rtc_options|, and returns true if options // are valid. -// Send streams should already be added to |session_options| before this method -// is called, as this affects the values of recv_audio and recv_video. bool ConvertRtcOptionsForOffer( const PeerConnectionInterface::RTCOfferAnswerOptions& rtc_options, cricket::MediaSessionOptions* session_options); diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc index bee6380bf4..63163fd651 100644 --- a/talk/app/webrtc/peerconnectioninterface_unittest.cc +++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc @@ -260,6 +260,7 @@ using webrtc::DataChannelInterface; using webrtc::FakeConstraints; using webrtc::FakePortAllocatorFactory; using webrtc::IceCandidateInterface; +using webrtc::MediaConstraintsInterface; using webrtc::MediaStream; using webrtc::MediaStreamInterface; using webrtc::MediaStreamTrackInterface; @@ -658,26 +659,30 @@ class PeerConnectionInterfaceTest : public testing::Test { observer_.renegotiation_needed_ = false; } - bool DoCreateOfferAnswer(SessionDescriptionInterface** desc, bool offer) { + bool DoCreateOfferAnswer(SessionDescriptionInterface** desc, + bool offer, + MediaConstraintsInterface* constraints) { rtc::scoped_refptr observer(new rtc::RefCountedObject< MockCreateSessionDescriptionObserver>()); if (offer) { - pc_->CreateOffer(observer, NULL); + pc_->CreateOffer(observer, constraints); } else { - pc_->CreateAnswer(observer, NULL); + pc_->CreateAnswer(observer, constraints); } EXPECT_EQ_WAIT(true, observer->called(), kTimeout); *desc = observer->release_desc(); return observer->result(); } - bool DoCreateOffer(SessionDescriptionInterface** desc) { - return DoCreateOfferAnswer(desc, true); + bool DoCreateOffer(SessionDescriptionInterface** desc, + MediaConstraintsInterface* constraints) { + return DoCreateOfferAnswer(desc, true, constraints); } - bool DoCreateAnswer(SessionDescriptionInterface** desc) { - return DoCreateOfferAnswer(desc, false); + bool DoCreateAnswer(SessionDescriptionInterface** desc, + MediaConstraintsInterface* constraints) { + return DoCreateOfferAnswer(desc, false, constraints); } bool DoSetSessionDescription(SessionDescriptionInterface* desc, bool local) { @@ -737,7 +742,7 @@ class PeerConnectionInterfaceTest : public testing::Test { void CreateOfferAsRemoteDescription() { rtc::scoped_ptr offer; - ASSERT_TRUE(DoCreateOffer(offer.use())); + ASSERT_TRUE(DoCreateOffer(offer.use(), nullptr)); std::string sdp; EXPECT_TRUE(offer->ToString(&sdp)); SessionDescriptionInterface* remote_offer = @@ -757,7 +762,7 @@ class PeerConnectionInterfaceTest : public testing::Test { void CreateAnswerAsLocalDescription() { scoped_ptr answer; - ASSERT_TRUE(DoCreateAnswer(answer.use())); + ASSERT_TRUE(DoCreateAnswer(answer.use(), nullptr)); // TODO(perkj): Currently SetLocalDescription fails if any parameters in an // audio codec change, even if the parameter has nothing to do with @@ -777,7 +782,7 @@ class PeerConnectionInterfaceTest : public testing::Test { void CreatePrAnswerAsLocalDescription() { scoped_ptr answer; - ASSERT_TRUE(DoCreateAnswer(answer.use())); + ASSERT_TRUE(DoCreateAnswer(answer.use(), nullptr)); std::string sdp; EXPECT_TRUE(answer->ToString(&sdp)); @@ -797,7 +802,7 @@ class PeerConnectionInterfaceTest : public testing::Test { void CreateOfferAsLocalDescription() { rtc::scoped_ptr offer; - ASSERT_TRUE(DoCreateOffer(offer.use())); + ASSERT_TRUE(DoCreateOffer(offer.use(), nullptr)); // TODO(perkj): Currently SetLocalDescription fails if any parameters in an // audio codec change, even if the parameter has nothing to do with // receiving. Not all parameters are serialized to SDP. @@ -967,7 +972,7 @@ TEST_F(PeerConnectionInterfaceTest, AddedStreamsPresentInOffer) { CreatePeerConnection(); AddAudioVideoStream(kStreamLabel1, "audio_track", "video_track"); scoped_ptr offer; - ASSERT_TRUE(DoCreateOffer(offer.accept())); + ASSERT_TRUE(DoCreateOffer(offer.accept(), nullptr)); const cricket::ContentInfo* audio_content = cricket::GetFirstAudioContent(offer->description()); @@ -988,7 +993,7 @@ TEST_F(PeerConnectionInterfaceTest, AddedStreamsPresentInOffer) { // Add another stream and ensure the offer includes both the old and new // streams. AddAudioVideoStream(kStreamLabel2, "audio_track2", "video_track2"); - ASSERT_TRUE(DoCreateOffer(offer.accept())); + ASSERT_TRUE(DoCreateOffer(offer.accept(), nullptr)); audio_content = cricket::GetFirstAudioContent(offer->description()); audio_desc = static_cast( @@ -1084,12 +1089,12 @@ TEST_F(PeerConnectionInterfaceTest, IceCandidates) { // SetRemoteDescription takes ownership of offer. SessionDescriptionInterface* offer = NULL; AddVideoStream(kStreamLabel1); - EXPECT_TRUE(DoCreateOffer(&offer)); + EXPECT_TRUE(DoCreateOffer(&offer, nullptr)); EXPECT_TRUE(DoSetRemoteDescription(offer)); // SetLocalDescription takes ownership of answer. SessionDescriptionInterface* answer = NULL; - EXPECT_TRUE(DoCreateAnswer(&answer)); + EXPECT_TRUE(DoCreateAnswer(&answer, nullptr)); EXPECT_TRUE(DoSetLocalDescription(answer)); EXPECT_TRUE_WAIT(observer_.last_candidate_.get() != NULL, kTimeout); @@ -1104,7 +1109,7 @@ TEST_F(PeerConnectionInterfaceTest, CreateOfferAnswerWithInvalidStream) { CreatePeerConnection(); // Create a regular offer for the CreateAnswer test later. SessionDescriptionInterface* offer = NULL; - EXPECT_TRUE(DoCreateOffer(&offer)); + EXPECT_TRUE(DoCreateOffer(&offer, nullptr)); EXPECT_TRUE(offer != NULL); delete offer; offer = NULL; @@ -1113,11 +1118,11 @@ TEST_F(PeerConnectionInterfaceTest, CreateOfferAnswerWithInvalidStream) { AddAudioVideoStream(kStreamLabel1, "track_label", "track_label"); // Test CreateOffer - EXPECT_FALSE(DoCreateOffer(&offer)); + EXPECT_FALSE(DoCreateOffer(&offer, nullptr)); // Test CreateAnswer SessionDescriptionInterface* answer = NULL; - EXPECT_FALSE(DoCreateAnswer(&answer)); + EXPECT_FALSE(DoCreateAnswer(&answer, nullptr)); } // Test that we will get different SSRCs for each tracks in the offer and answer @@ -1129,7 +1134,7 @@ TEST_F(PeerConnectionInterfaceTest, SsrcInOfferAnswer) { // Test CreateOffer scoped_ptr offer; - ASSERT_TRUE(DoCreateOffer(offer.use())); + ASSERT_TRUE(DoCreateOffer(offer.use(), nullptr)); int audio_ssrc = 0; int video_ssrc = 0; EXPECT_TRUE(GetFirstSsrc(GetFirstAudioContent(offer->description()), @@ -1141,7 +1146,7 @@ TEST_F(PeerConnectionInterfaceTest, SsrcInOfferAnswer) { // Test CreateAnswer EXPECT_TRUE(DoSetRemoteDescription(offer.release())); scoped_ptr answer; - ASSERT_TRUE(DoCreateAnswer(answer.use())); + ASSERT_TRUE(DoCreateAnswer(answer.use(), nullptr)); audio_ssrc = 0; video_ssrc = 0; EXPECT_TRUE(GetFirstSsrc(GetFirstAudioContent(answer->description()), @@ -1588,6 +1593,73 @@ TEST_F(PeerConnectionInterfaceTest, ReceiveUpdatedAudioOfferWithBadCodecs) { CreateAnswerAsLocalDescription(); } +// Test that if we're receiving (but not sending) a track, subsequent offers +// will have m-lines with a=recvonly. +TEST_F(PeerConnectionInterfaceTest, CreateSubsequentRecvOnlyOffer) { + FakeConstraints constraints; + constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, + true); + CreatePeerConnection(&constraints); + CreateAndSetRemoteOffer(kSdpStringWithStream1); + CreateAnswerAsLocalDescription(); + + // At this point we should be receiving stream 1, but not sending anything. + // A new offer should be recvonly. + SessionDescriptionInterface* offer; + DoCreateOffer(&offer, nullptr); + + const cricket::ContentInfo* video_content = + cricket::GetFirstVideoContent(offer->description()); + const cricket::VideoContentDescription* video_desc = + static_cast( + video_content->description); + ASSERT_EQ(cricket::MD_RECVONLY, video_desc->direction()); + + const cricket::ContentInfo* audio_content = + cricket::GetFirstAudioContent(offer->description()); + const cricket::AudioContentDescription* audio_desc = + static_cast( + audio_content->description); + ASSERT_EQ(cricket::MD_RECVONLY, audio_desc->direction()); +} + +// Test that if we're receiving (but not sending) a track, and the +// offerToReceiveVideo/offerToReceiveAudio constraints are explicitly set to +// false, the generated m-lines will be a=inactive. +TEST_F(PeerConnectionInterfaceTest, CreateSubsequentInactiveOffer) { + FakeConstraints constraints; + constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, + true); + CreatePeerConnection(&constraints); + CreateAndSetRemoteOffer(kSdpStringWithStream1); + CreateAnswerAsLocalDescription(); + + // At this point we should be receiving stream 1, but not sending anything. + // A new offer would be recvonly, but we'll set the "no receive" constraints + // to make it inactive. + SessionDescriptionInterface* offer; + FakeConstraints offer_constraints; + offer_constraints.AddMandatory( + webrtc::MediaConstraintsInterface::kOfferToReceiveVideo, false); + offer_constraints.AddMandatory( + webrtc::MediaConstraintsInterface::kOfferToReceiveAudio, false); + DoCreateOffer(&offer, &offer_constraints); + + const cricket::ContentInfo* video_content = + cricket::GetFirstVideoContent(offer->description()); + const cricket::VideoContentDescription* video_desc = + static_cast( + video_content->description); + ASSERT_EQ(cricket::MD_INACTIVE, video_desc->direction()); + + const cricket::ContentInfo* audio_content = + cricket::GetFirstAudioContent(offer->description()); + const cricket::AudioContentDescription* audio_desc = + static_cast( + audio_content->description); + ASSERT_EQ(cricket::MD_INACTIVE, audio_desc->direction()); +} + // Test that PeerConnection::Close changes the states to closed and all remote // tracks change state to ended. TEST_F(PeerConnectionInterfaceTest, CloseAndTestStreamsAndStates) { @@ -1644,9 +1716,9 @@ TEST_F(PeerConnectionInterfaceTest, CloseAndTestMethods) { EXPECT_TRUE(pc_->remote_description() != NULL); rtc::scoped_ptr offer; - EXPECT_TRUE(DoCreateOffer(offer.use())); + EXPECT_TRUE(DoCreateOffer(offer.use(), nullptr)); rtc::scoped_ptr answer; - EXPECT_TRUE(DoCreateAnswer(answer.use())); + EXPECT_TRUE(DoCreateAnswer(answer.use(), nullptr)); std::string sdp; ASSERT_TRUE(pc_->remote_description()->ToString(&sdp)); @@ -1750,7 +1822,7 @@ TEST_F(PeerConnectionInterfaceTest, RejectMediaContent) { EXPECT_EQ(webrtc::MediaStreamTrackInterface::kLive, remote_audio->state()); rtc::scoped_ptr local_answer; - EXPECT_TRUE(DoCreateAnswer(local_answer.accept())); + EXPECT_TRUE(DoCreateAnswer(local_answer.accept(), nullptr)); cricket::ContentInfo* video_info = local_answer->description()->GetContentByName("video"); video_info->rejected = true; @@ -1760,7 +1832,7 @@ TEST_F(PeerConnectionInterfaceTest, RejectMediaContent) { // Now create an offer where we reject both video and audio. rtc::scoped_ptr local_offer; - EXPECT_TRUE(DoCreateOffer(local_offer.accept())); + EXPECT_TRUE(DoCreateOffer(local_offer.accept(), nullptr)); video_info = local_offer->description()->GetContentByName("video"); ASSERT_TRUE(video_info != nullptr); video_info->rejected = true; @@ -1933,7 +2005,7 @@ TEST_F(PeerConnectionInterfaceTest, LocalDescriptionChanged) { // Create an offer just to ensure we have an identity before we manually // call SetLocalDescription. rtc::scoped_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(throwaway.accept())); + ASSERT_TRUE(DoCreateOffer(throwaway.accept(), nullptr)); rtc::scoped_ptr desc_1; CreateSessionDescriptionAndReference(2, 2, desc_1.accept()); @@ -1970,7 +2042,7 @@ TEST_F(PeerConnectionInterfaceTest, // Create an offer just to ensure we have an identity before we manually // call SetLocalDescription. rtc::scoped_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(throwaway.accept())); + ASSERT_TRUE(DoCreateOffer(throwaway.accept(), nullptr)); rtc::scoped_ptr desc_1; CreateSessionDescriptionAndReference(2, 2, desc_1.accept()); @@ -1999,7 +2071,7 @@ TEST_F(PeerConnectionInterfaceTest, // Create an offer just to ensure we have an identity before we manually // call SetLocalDescription. rtc::scoped_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(throwaway.accept())); + ASSERT_TRUE(DoCreateOffer(throwaway.accept(), nullptr)); rtc::scoped_ptr desc; CreateSessionDescriptionAndReference(1, 1, desc.accept()); @@ -2045,7 +2117,7 @@ TEST_F(PeerConnectionInterfaceTest, SignalSameTracksInSeparateMediaStream) { // Create an offer just to ensure we have an identity before we manually // call SetLocalDescription. rtc::scoped_ptr throwaway; - ASSERT_TRUE(DoCreateOffer(throwaway.accept())); + ASSERT_TRUE(DoCreateOffer(throwaway.accept(), nullptr)); rtc::scoped_ptr desc; CreateSessionDescriptionAndReference(1, 1, desc.accept()); @@ -2082,6 +2154,9 @@ TEST_F(PeerConnectionInterfaceTest, SignalSameTracksInSeparateMediaStream) { } // The following tests verify that session options are created correctly. +// TODO(deadbeef): Convert these tests to be more end-to-end. Instead of +// "verify options are converted correctly", should be "pass options into +// CreateOffer and verify the correct offer is produced." TEST(CreateSessionOptionsTest, GetOptionsForOfferWithInvalidAudioOption) { RTCOfferAnswerOptions rtc_options; @@ -2108,8 +2183,7 @@ TEST(CreateSessionOptionsTest, GetOptionsForOfferWithInvalidVideoOption) { } // Test that a MediaSessionOptions is created for an offer if -// OfferToReceiveAudio and OfferToReceiveVideo options are set but no -// MediaStreams are sent. +// OfferToReceiveAudio and OfferToReceiveVideo options are set. TEST(CreateSessionOptionsTest, GetMediaSessionOptionsForOfferWithAudioVideo) { RTCOfferAnswerOptions rtc_options; rtc_options.offer_to_receive_audio = 1; @@ -2123,7 +2197,7 @@ TEST(CreateSessionOptionsTest, GetMediaSessionOptionsForOfferWithAudioVideo) { } // Test that a correct MediaSessionOptions is created for an offer if -// OfferToReceiveAudio is set but no MediaStreams are sent. +// OfferToReceiveAudio is set. TEST(CreateSessionOptionsTest, GetMediaSessionOptionsForOfferWithAudio) { RTCOfferAnswerOptions rtc_options; rtc_options.offer_to_receive_audio = 1; @@ -2136,21 +2210,21 @@ TEST(CreateSessionOptionsTest, GetMediaSessionOptionsForOfferWithAudio) { } // Test that a correct MediaSessionOptions is created for an offer if -// the default OfferOptons is used or MediaStreams are sent. +// the default OfferOptions are used. TEST(CreateSessionOptionsTest, GetDefaultMediaSessionOptionsForOffer) { RTCOfferAnswerOptions rtc_options; cricket::MediaSessionOptions options; EXPECT_TRUE(ConvertRtcOptionsForOffer(rtc_options, &options)); - EXPECT_FALSE(options.has_audio()); + EXPECT_TRUE(options.has_audio()); EXPECT_FALSE(options.has_video()); - EXPECT_FALSE(options.bundle_enabled); + EXPECT_TRUE(options.bundle_enabled); EXPECT_TRUE(options.vad_enabled); EXPECT_FALSE(options.transport_options.ice_restart); } // Test that a correct MediaSessionOptions is created for an offer if -// OfferToReceiveVideo is set but no MediaStreams are sent. +// OfferToReceiveVideo is set. TEST(CreateSessionOptionsTest, GetMediaSessionOptionsForOfferWithVideo) { RTCOfferAnswerOptions rtc_options; rtc_options.offer_to_receive_audio = 0; @@ -2209,19 +2283,19 @@ TEST(CreateSessionOptionsTest, MediaConstraintsInAnswer) { EXPECT_TRUE(answer_options.has_audio()); EXPECT_TRUE(answer_options.has_video()); - RTCOfferAnswerOptions rtc_offer_optoins; + RTCOfferAnswerOptions rtc_offer_options; cricket::MediaSessionOptions offer_options; - EXPECT_TRUE(ConvertRtcOptionsForOffer(rtc_offer_optoins, &offer_options)); - EXPECT_FALSE(offer_options.has_audio()); + EXPECT_TRUE(ConvertRtcOptionsForOffer(rtc_offer_options, &offer_options)); + EXPECT_TRUE(offer_options.has_audio()); EXPECT_FALSE(offer_options.has_video()); - RTCOfferAnswerOptions updated_rtc_offer_optoins; - updated_rtc_offer_optoins.offer_to_receive_audio = 1; - updated_rtc_offer_optoins.offer_to_receive_video = 1; + RTCOfferAnswerOptions updated_rtc_offer_options; + updated_rtc_offer_options.offer_to_receive_audio = 1; + updated_rtc_offer_options.offer_to_receive_video = 1; cricket::MediaSessionOptions updated_offer_options; - EXPECT_TRUE(ConvertRtcOptionsForOffer(updated_rtc_offer_optoins, + EXPECT_TRUE(ConvertRtcOptionsForOffer(updated_rtc_offer_options, &updated_offer_options)); EXPECT_TRUE(updated_offer_options.has_audio()); EXPECT_TRUE(updated_offer_options.has_video()); @@ -2240,12 +2314,4 @@ TEST(CreateSessionOptionsTest, MediaConstraintsInAnswer) { ParseConstraintsForAnswer(&updated_answer_c, &updated_answer_options)); EXPECT_TRUE(updated_answer_options.has_audio()); EXPECT_TRUE(updated_answer_options.has_video()); - - RTCOfferAnswerOptions default_rtc_options; - EXPECT_TRUE( - ConvertRtcOptionsForOffer(default_rtc_options, &updated_offer_options)); - // By default, |has_audio| or |has_video| are false if there is no media - // track. - EXPECT_FALSE(updated_offer_options.has_audio()); - EXPECT_FALSE(updated_offer_options.has_video()); } diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index 6a0f62271e..3fa9a7d469 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc @@ -268,7 +268,6 @@ static bool IsDtlsSctp(const std::string& protocol); static bool ParseSessionDescription(const std::string& message, size_t* pos, std::string* session_id, std::string* session_version, - bool* supports_msid, TransportDescription* session_td, RtpHeaderExtensions* session_extmaps, cricket::SessionDescription* desc, @@ -280,7 +279,6 @@ static bool ParseMediaDescription( const std::string& message, const TransportDescription& session_td, const RtpHeaderExtensions& session_extmaps, - bool supports_msid, size_t* pos, cricket::SessionDescription* desc, std::vector* candidates, SdpParseError* error); @@ -898,20 +896,18 @@ bool SdpDeserialize(const std::string& message, cricket::SessionDescription* desc = new cricket::SessionDescription(); std::vector candidates; size_t current_pos = 0; - bool supports_msid = false; // Session Description if (!ParseSessionDescription(message, ¤t_pos, &session_id, - &session_version, &supports_msid, &session_td, - &session_extmaps, desc, error)) { + &session_version, &session_td, &session_extmaps, + desc, error)) { delete desc; return false; } // Media Description - if (!ParseMediaDescription(message, session_td, session_extmaps, - supports_msid, ¤t_pos, desc, &candidates, - error)) { + if (!ParseMediaDescription(message, session_td, session_extmaps, ¤t_pos, + desc, &candidates, error)) { delete desc; for (std::vector::const_iterator it = candidates.begin(); it != candidates.end(); ++it) { @@ -1379,13 +1375,7 @@ void BuildRtpContentAttributes( // RFC 3264 // a=sendrecv || a=sendonly || a=sendrecv || a=inactive - - cricket::MediaContentDirection direction = media_desc->direction(); - if (media_desc->streams().empty() && direction == cricket::MD_SENDRECV) { - direction = cricket::MD_RECVONLY; - } - - switch (direction) { + switch (media_desc->direction()) { case cricket::MD_INACTIVE: InitAttrLine(kAttributeInactive, &os); break; @@ -1798,13 +1788,14 @@ bool IsDtlsSctp(const std::string& protocol) { bool ParseSessionDescription(const std::string& message, size_t* pos, std::string* session_id, std::string* session_version, - bool* supports_msid, TransportDescription* session_td, RtpHeaderExtensions* session_extmaps, cricket::SessionDescription* desc, SdpParseError* error) { std::string line; + desc->set_msid_supported(false); + // RFC 4566 // v= (protocol version) if (!GetLineWithType(message, pos, &line, kLineTypeVersion)) { @@ -1936,7 +1927,8 @@ bool ParseSessionDescription(const std::string& message, size_t* pos, if (!GetValue(line, kAttributeMsidSemantics, &semantics, error)) { return false; } - *supports_msid = CaseInsensitiveFind(semantics, kMediaStreamSemantic); + desc->set_msid_supported( + CaseInsensitiveFind(semantics, kMediaStreamSemantic)); } else if (HasAttribute(line, kAttributeExtmap)) { RtpHeaderExtension extmap; if (!ParseExtmap(line, &extmap, error)) { @@ -2146,7 +2138,6 @@ static C* ParseContentDescription(const std::string& message, bool ParseMediaDescription(const std::string& message, const TransportDescription& session_td, const RtpHeaderExtensions& session_extmaps, - bool supports_msid, size_t* pos, cricket::SessionDescription* desc, std::vector* candidates, @@ -2241,14 +2232,6 @@ bool ParseMediaDescription(const std::string& message, } if (IsRtp(protocol)) { - // Make sure to set the media direction correctly. If the direction is not - // MD_RECVONLY or Inactive and no streams are parsed, - // a default MediaStream will be created to prepare for receiving media. - if (supports_msid && content->streams().empty() && - content->direction() == cricket::MD_SENDRECV) { - content->set_direction(cricket::MD_RECVONLY); - } - // Set the extmap. if (!session_extmaps.empty() && !content->rtp_header_extensions().empty()) { diff --git a/talk/app/webrtc/webrtcsdp_unittest.cc b/talk/app/webrtc/webrtcsdp_unittest.cc index de2ba2f479..cb6a392ab4 100644 --- a/talk/app/webrtc/webrtcsdp_unittest.cc +++ b/talk/app/webrtc/webrtcsdp_unittest.cc @@ -883,6 +883,9 @@ class WebRtcSdpTest : public testing::Test { EXPECT_TRUE(CompareCandidates(transport1.description.candidates, transport2.description.candidates)); } + + // global attributes + EXPECT_EQ(desc1.msid_supported(), desc2.msid_supported()); } bool CompareCandidates(const Candidates& cs1, const Candidates& cs2) { @@ -1967,6 +1970,7 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithRejectedAudioVideo) { // Tests that we can still handle the sdp uses mslabel and label instead of // msid for backward compatibility. TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutMsid) { + jdesc_.description()->set_msid_supported(false); JsepSessionDescription jdesc(kDummyString); std::string sdp_without_msid = kSdpFullString; Replace("msid", "xmsid", &sdp_without_msid); diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index 9618db9d80..3eb46f1d3c 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -571,9 +571,24 @@ class WebRtcSessionTest void GetOptionsForOffer( const PeerConnectionInterface::RTCOfferAnswerOptions& rtc_options, cricket::MediaSessionOptions* session_options) { - AddStreamsToOptions(session_options); ASSERT_TRUE(ConvertRtcOptionsForOffer(rtc_options, session_options)); + AddStreamsToOptions(session_options); + if (rtc_options.offer_to_receive_audio == + RTCOfferAnswerOptions::kUndefined) { + session_options->recv_audio = + session_options->HasSendMediaStream(cricket::MEDIA_TYPE_AUDIO); + } + if (rtc_options.offer_to_receive_video == + RTCOfferAnswerOptions::kUndefined) { + session_options->recv_video = + session_options->HasSendMediaStream(cricket::MEDIA_TYPE_VIDEO); + } + session_options->bundle_enabled = + session_options->bundle_enabled && + (session_options->has_audio() || session_options->has_video() || + session_options->has_data()); + if (session_->data_channel_type() == cricket::DCT_SCTP && data_channel_) { session_options->data_channel_type = cricket::DCT_SCTP; } @@ -581,11 +596,16 @@ class WebRtcSessionTest void GetOptionsForAnswer(const webrtc::MediaConstraintsInterface* constraints, cricket::MediaSessionOptions* session_options) { - AddStreamsToOptions(session_options); session_options->recv_audio = false; session_options->recv_video = false; ASSERT_TRUE(ParseConstraintsForAnswer(constraints, session_options)); + AddStreamsToOptions(session_options); + session_options->bundle_enabled = + session_options->bundle_enabled && + (session_options->has_audio() || session_options->has_video() || + session_options->has_data()); + if (session_->data_channel_type() == cricket::DCT_SCTP) { session_options->data_channel_type = cricket::DCT_SCTP; } diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc index 2cd2d46d9c..7413026092 100644 --- a/talk/session/media/mediasession.cc +++ b/talk/session/media/mediasession.cc @@ -1036,12 +1036,15 @@ static bool CreateMediaContentAnswer( answer->set_direction(MD_RECVONLY); break; case MD_RECVONLY: - answer->set_direction(MD_SENDONLY); + answer->set_direction(answer->streams().empty() ? MD_INACTIVE + : MD_SENDONLY); break; case MD_SENDRECV: - answer->set_direction(MD_SENDRECV); + answer->set_direction(answer->streams().empty() ? MD_RECVONLY + : MD_SENDRECV); break; default: + RTC_DCHECK(false && "MediaContentDescription has unexpected direction."); break; } @@ -1529,8 +1532,18 @@ bool MediaSessionDescriptionFactory::AddAudioContentForOffer( bool secure_transport = (transport_desc_factory_->secure() != SEC_DISABLED); SetMediaProtocol(secure_transport, audio.get()); - if (!options.recv_audio) { - audio->set_direction(MD_SENDONLY); + if (!audio->streams().empty()) { + if (options.recv_audio) { + audio->set_direction(MD_SENDRECV); + } else { + audio->set_direction(MD_SENDONLY); + } + } else { + if (options.recv_audio) { + audio->set_direction(MD_RECVONLY); + } else { + audio->set_direction(MD_INACTIVE); + } } desc->AddContent(CN_AUDIO, NS_JINGLE_RTP, audio.release()); @@ -1574,8 +1587,18 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( bool secure_transport = (transport_desc_factory_->secure() != SEC_DISABLED); SetMediaProtocol(secure_transport, video.get()); - if (!options.recv_video) { - video->set_direction(MD_SENDONLY); + if (!video->streams().empty()) { + if (options.recv_video) { + video->set_direction(MD_SENDRECV); + } else { + video->set_direction(MD_SENDONLY); + } + } else { + if (options.recv_video) { + video->set_direction(MD_RECVONLY); + } else { + video->set_direction(MD_INACTIVE); + } } desc->AddContent(CN_VIDEO, NS_JINGLE_RTP, video.release()); diff --git a/webrtc/p2p/base/sessiondescription.h b/webrtc/p2p/base/sessiondescription.h index 1182a67740..7880167569 100644 --- a/webrtc/p2p/base/sessiondescription.h +++ b/webrtc/p2p/base/sessiondescription.h @@ -160,10 +160,15 @@ class SessionDescription { // Remove the first group with the same semantics specified by |name|. void RemoveGroupByName(const std::string& name); + // Global attributes. + void set_msid_supported(bool supported) { msid_supported_ = supported; } + bool msid_supported() const { return msid_supported_; } + private: ContentInfos contents_; TransportInfos transport_infos_; ContentGroups content_groups_; + bool msid_supported_ = true; }; // Indicates whether a ContentDescription was an offer or an answer, as