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}
diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc
index 1b61c7f..f1ff4e6 100644
--- a/talk/app/webrtc/peerconnection.cc
+++ b/talk/app/webrtc/peerconnection.cc
@@ -1137,6 +1137,22 @@
}
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 @@
// 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<const cricket::AudioContentDescription*>(
- 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<const cricket::VideoContentDescription*>(
- 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<const cricket::DataContentDescription*>(
- 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 @@
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 @@
void PeerConnection::RemoveTracks(cricket::MediaType media_type) {
UpdateLocalTracks(std::vector<cricket::StreamParams>(), media_type);
- UpdateRemoteStreamsList(std::vector<cricket::StreamParams>(), media_type,
- nullptr);
+ UpdateRemoteStreamsList(std::vector<cricket::StreamParams>(), 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 @@
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 @@
OnRemoteTrackSeen(stream_label, track_id, ssrc, media_type);
}
}
+
+ // Add default track if necessary.
+ if (default_track_needed) {
+ rtc::scoped_refptr<MediaStreamInterface> 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::MaybeCreateDefaultStream() {
- if (!remote_info_.IsDefaultMediaStreamNeeded()) {
- return;
- }
-
- bool default_created = false;
-
- rtc::scoped_refptr<MediaStreamInterface> 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 2588020..12f8d1b 100644
--- a/talk/app/webrtc/peerconnection.h
+++ b/talk/app/webrtc/peerconnection.h
@@ -161,32 +161,16 @@
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<TrackInfo> 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 @@
// 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<cricket::StreamParams>& streams,
+ bool default_track_needed,
cricket::MediaType media_type,
StreamCollection* new_streams);
@@ -276,8 +263,6 @@
// 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 @@
std::map<std::string, rtc::scoped_refptr<DataChannel>> rtp_data_channels_;
std::vector<rtc::scoped_refptr<DataChannel>> sctp_data_channels_;
- RemotePeerInfo remote_info_;
+ bool remote_peer_supports_msid_ = false;
rtc::scoped_ptr<RemoteMediaStreamFactory> remote_stream_factory_;
std::vector<rtc::scoped_refptr<RtpSenderInterface>> senders_;
diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc
index e3935f7..edf7593 100644
--- a/talk/app/webrtc/peerconnectioninterface_unittest.cc
+++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc
@@ -1994,6 +1994,28 @@
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) {
