From bda7e0b932fc89598da95496efc8650bc0e2c86c Mon Sep 17 00:00:00 2001 From: deadbeef Date: Tue, 8 Dec 2015 17:13:40 -0800 Subject: [PATCH] Fixing issue with default stream upon setting 2nd remote description. If a description is set that requires making a default stream, and one already exists, we'll now keep the existing default audio/video tracks, rather than destroying them and recreating them. Destroying them caused the blink MediaStream to go to an "ended" state, which is the root cause of the bug. BUG=webrtc:5250 Review URL: https://codereview.webrtc.org/1469833006 Cr-Commit-Position: refs/heads/master@{#10946} --- talk/app/webrtc/peerconnection.cc | 134 ++++++++---------- talk/app/webrtc/peerconnection.h | 37 ++--- .../peerconnectioninterface_unittest.cc | 22 +++ 3 files changed, 95 insertions(+), 98 deletions(-) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index 1b61c7fb27..f1ff4e6d36 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -1137,6 +1137,22 @@ void PeerConnection::SetRemoteDescription( } const cricket::SessionDescription* remote_desc = desc->description(); + const cricket::ContentInfo* audio_content = GetFirstAudioContent(remote_desc); + const cricket::ContentInfo* video_content = GetFirstVideoContent(remote_desc); + const cricket::AudioContentDescription* audio_desc = + GetFirstAudioContentDescription(remote_desc); + const cricket::VideoContentDescription* video_desc = + GetFirstVideoContentDescription(remote_desc); + const cricket::DataContentDescription* data_desc = + GetFirstDataContentDescription(remote_desc); + + // Check if the descriptions include streams, just in case the peer supports + // MSID, but doesn't indicate so with "a=msid-semantic". + if (remote_desc->msid_supported() || + (audio_desc && !audio_desc->streams().empty()) || + (video_desc && !video_desc->streams().empty())) { + remote_peer_supports_msid_ = true; + } // We wait to signal new streams until we finish processing the description, // since only at that point will new streams have all their tracks. @@ -1144,49 +1160,39 @@ void PeerConnection::SetRemoteDescription( // Find all audio rtp streams and create corresponding remote AudioTracks // and MediaStreams. - const cricket::ContentInfo* audio_content = GetFirstAudioContent(remote_desc); if (audio_content) { if (audio_content->rejected) { RemoveTracks(cricket::MEDIA_TYPE_AUDIO); } else { - const cricket::AudioContentDescription* desc = - static_cast( - audio_content->description); - UpdateRemoteStreamsList(GetActiveStreams(desc), desc->type(), + bool default_audio_track_needed = + !remote_peer_supports_msid_ && + MediaContentDirectionHasSend(audio_desc->direction()); + UpdateRemoteStreamsList(GetActiveStreams(audio_desc), + default_audio_track_needed, audio_desc->type(), new_streams); - remote_info_.default_audio_track_needed = - !remote_desc->msid_supported() && desc->streams().empty() && - MediaContentDirectionHasSend(desc->direction()); } } // Find all video rtp streams and create corresponding remote VideoTracks // and MediaStreams. - const cricket::ContentInfo* video_content = GetFirstVideoContent(remote_desc); if (video_content) { if (video_content->rejected) { RemoveTracks(cricket::MEDIA_TYPE_VIDEO); } else { - const cricket::VideoContentDescription* desc = - static_cast( - video_content->description); - UpdateRemoteStreamsList(GetActiveStreams(desc), desc->type(), + bool default_video_track_needed = + !remote_peer_supports_msid_ && + MediaContentDirectionHasSend(video_desc->direction()); + UpdateRemoteStreamsList(GetActiveStreams(video_desc), + default_video_track_needed, video_desc->type(), new_streams); - remote_info_.default_video_track_needed = - !remote_desc->msid_supported() && desc->streams().empty() && - MediaContentDirectionHasSend(desc->direction()); } } // Update the DataChannels with the information from the remote peer. - const cricket::ContentInfo* data_content = GetFirstDataContent(remote_desc); - if (data_content) { - const cricket::DataContentDescription* desc = - static_cast( - data_content->description); - if (rtc::starts_with(desc->protocol().data(), + if (data_desc) { + if (rtc::starts_with(data_desc->protocol().data(), cricket::kMediaProtocolRtpPrefix)) { - UpdateRemoteRtpDataChannels(GetActiveStreams(desc)); + UpdateRemoteRtpDataChannels(GetActiveStreams(data_desc)); } } @@ -1197,15 +1203,7 @@ void PeerConnection::SetRemoteDescription( observer_->OnAddStream(new_stream); } - // Find removed MediaStreams. - if (remote_info_.IsDefaultMediaStreamNeeded() && - remote_streams_->find(kDefaultStreamLabel) != nullptr) { - // The default media stream already exists. No need to do anything. - } else { - UpdateEndedRemoteMediaStreams(); - remote_info_.msid_supported |= remote_streams_->count() > 0; - } - MaybeCreateDefaultStream(); + UpdateEndedRemoteMediaStreams(); SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer); signaling_thread()->Post(this, MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg); @@ -1507,12 +1505,13 @@ bool PeerConnection::GetOptionsForAnswer( void PeerConnection::RemoveTracks(cricket::MediaType media_type) { UpdateLocalTracks(std::vector(), media_type); - UpdateRemoteStreamsList(std::vector(), media_type, - nullptr); + UpdateRemoteStreamsList(std::vector(), false, + media_type, nullptr); } void PeerConnection::UpdateRemoteStreamsList( const cricket::StreamParamsVec& streams, + bool default_track_needed, cricket::MediaType media_type, StreamCollection* new_streams) { TrackInfos* current_tracks = GetRemoteTracks(media_type); @@ -1524,11 +1523,14 @@ void PeerConnection::UpdateRemoteStreamsList( const TrackInfo& info = *track_it; const cricket::StreamParams* params = cricket::GetStreamBySsrc(streams, info.ssrc); - if (!params || params->id != info.track_id) { + bool track_exists = params && params->id == info.track_id; + // If this is a default track, and we still need it, don't remove it. + if ((info.stream_label == kDefaultStreamLabel && default_track_needed) || + track_exists) { + ++track_it; + } else { OnRemoteTrackRemoved(info.stream_label, info.track_id, media_type); track_it = current_tracks->erase(track_it); - } else { - ++track_it; } } @@ -1556,6 +1558,29 @@ void PeerConnection::UpdateRemoteStreamsList( OnRemoteTrackSeen(stream_label, track_id, ssrc, media_type); } } + + // Add default track if necessary. + if (default_track_needed) { + rtc::scoped_refptr default_stream = + remote_streams_->find(kDefaultStreamLabel); + if (!default_stream) { + // Create the new default MediaStream. + default_stream = + remote_stream_factory_->CreateMediaStream(kDefaultStreamLabel); + remote_streams_->AddStream(default_stream); + new_streams->AddStream(default_stream); + } + std::string default_track_id = (media_type == cricket::MEDIA_TYPE_AUDIO) + ? kDefaultAudioTrackLabel + : kDefaultVideoTrackLabel; + const TrackInfo* default_track_info = + FindTrackInfo(*current_tracks, kDefaultStreamLabel, default_track_id); + if (!default_track_info) { + current_tracks->push_back( + TrackInfo(kDefaultStreamLabel, default_track_id, 0)); + OnRemoteTrackSeen(kDefaultStreamLabel, default_track_id, 0, media_type); + } + } } void PeerConnection::OnRemoteTrackSeen(const std::string& stream_label, @@ -1618,41 +1643,6 @@ void PeerConnection::UpdateEndedRemoteMediaStreams() { } } -void PeerConnection::MaybeCreateDefaultStream() { - if (!remote_info_.IsDefaultMediaStreamNeeded()) { - return; - } - - bool default_created = false; - - rtc::scoped_refptr default_remote_stream = - remote_streams_->find(kDefaultStreamLabel); - if (default_remote_stream == nullptr) { - default_created = true; - default_remote_stream = - remote_stream_factory_->CreateMediaStream(kDefaultStreamLabel); - remote_streams_->AddStream(default_remote_stream); - } - if (remote_info_.default_audio_track_needed && - default_remote_stream->GetAudioTracks().size() == 0) { - remote_audio_tracks_.push_back( - TrackInfo(kDefaultStreamLabel, kDefaultAudioTrackLabel, 0)); - OnRemoteTrackSeen(kDefaultStreamLabel, kDefaultAudioTrackLabel, 0, - cricket::MEDIA_TYPE_AUDIO); - } - if (remote_info_.default_video_track_needed && - default_remote_stream->GetVideoTracks().size() == 0) { - remote_video_tracks_.push_back( - TrackInfo(kDefaultStreamLabel, kDefaultVideoTrackLabel, 0)); - OnRemoteTrackSeen(kDefaultStreamLabel, kDefaultVideoTrackLabel, 0, - cricket::MEDIA_TYPE_VIDEO); - } - if (default_created) { - stats_->AddStream(default_remote_stream); - observer_->OnAddStream(default_remote_stream); - } -} - void PeerConnection::EndRemoteTracks(cricket::MediaType media_type) { TrackInfos* current_tracks = GetRemoteTracks(media_type); for (TrackInfos::iterator track_it = current_tracks->begin(); diff --git a/talk/app/webrtc/peerconnection.h b/talk/app/webrtc/peerconnection.h index 2588020350..12f8d1bd4b 100644 --- a/talk/app/webrtc/peerconnection.h +++ b/talk/app/webrtc/peerconnection.h @@ -161,32 +161,16 @@ class PeerConnection : public PeerConnectionInterface, const std::string track_id, uint32_t ssrc) : stream_label(stream_label), track_id(track_id), ssrc(ssrc) {} + bool operator==(const TrackInfo& other) { + return this->stream_label == other.stream_label && + this->track_id == other.track_id && this->ssrc == other.ssrc; + } std::string stream_label; std::string track_id; uint32_t ssrc; }; typedef std::vector TrackInfos; - struct RemotePeerInfo { - RemotePeerInfo() - : msid_supported(false), - default_audio_track_needed(false), - default_video_track_needed(false) {} - // True if it has been discovered that the remote peer support MSID. - bool msid_supported; - // The remote peer indicates in the session description that audio will be - // sent but no MSID is given. - bool default_audio_track_needed; - // The remote peer indicates in the session description that video will be - // sent but no MSID is given. - bool default_video_track_needed; - - bool IsDefaultMediaStreamNeeded() { - return !msid_supported && - (default_audio_track_needed || default_video_track_needed); - } - }; - // Implements MessageHandler. void OnMessage(rtc::Message* msg) override; @@ -247,12 +231,15 @@ class PeerConnection : public PeerConnectionInterface, // Called when a media type is rejected (m-line set to port 0). void RemoveTracks(cricket::MediaType media_type); - // Makes sure a MediaStream Track is created for each StreamParam in - // |streams|. |media_type| is the type of the |streams| and can be either - // audio or video. + // Makes sure a MediaStreamTrack is created for each StreamParam in |streams|, + // and existing MediaStreamTracks are removed if there is no corresponding + // StreamParam. If |default_track_needed| is true, a default MediaStreamTrack + // is created if it doesn't exist; if false, it's removed if it exists. + // |media_type| is the type of the |streams| and can be either audio or video. // If a new MediaStream is created it is added to |new_streams|. void UpdateRemoteStreamsList( const std::vector& streams, + bool default_track_needed, cricket::MediaType media_type, StreamCollection* new_streams); @@ -276,8 +263,6 @@ class PeerConnection : public PeerConnectionInterface, // exist. void UpdateEndedRemoteMediaStreams(); - void MaybeCreateDefaultStream(); - // Set the MediaStreamTrackInterface::TrackState to |kEnded| on all remote // tracks of type |media_type|. void EndRemoteTracks(cricket::MediaType media_type); @@ -390,7 +375,7 @@ class PeerConnection : public PeerConnectionInterface, std::map> rtp_data_channels_; std::vector> sctp_data_channels_; - RemotePeerInfo remote_info_; + bool remote_peer_supports_msid_ = false; rtc::scoped_ptr remote_stream_factory_; std::vector> senders_; diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc index e3935f7410..edf75931a4 100644 --- a/talk/app/webrtc/peerconnectioninterface_unittest.cc +++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc @@ -1994,6 +1994,28 @@ TEST_F(PeerConnectionInterfaceTest, SdpWithMsidDontCreatesDefaultStream) { EXPECT_EQ(0u, observer_.remote_streams()->count()); } +// This tests that when setting a new description, the old default tracks are +// not destroyed and recreated. +// See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5250 +TEST_F(PeerConnectionInterfaceTest, DefaultTracksNotDestroyedAndRecreated) { + FakeConstraints constraints; + constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, + true); + CreatePeerConnection(&constraints); + CreateAndSetRemoteOffer(kSdpStringWithoutStreamsAudioOnly); + + ASSERT_EQ(1u, observer_.remote_streams()->count()); + MediaStreamInterface* remote_stream = observer_.remote_streams()->at(0); + ASSERT_EQ(1u, remote_stream->GetAudioTracks().size()); + + // Set the track to "disabled", then set a new description and ensure the + // track is still disabled, which ensures it hasn't been recreated. + remote_stream->GetAudioTracks()[0]->set_enabled(false); + CreateAndSetRemoteOffer(kSdpStringWithoutStreamsAudioOnly); + ASSERT_EQ(1u, remote_stream->GetAudioTracks().size()); + EXPECT_FALSE(remote_stream->GetAudioTracks()[0]->enabled()); +} + // This tests that a default MediaStream is not created if a remote session // description is updated to not have any MediaStreams. TEST_F(PeerConnectionInterfaceTest, VerifyDefaultStreamIsNotCreated) {