When a track is added/removed directly to MediaStream notify observer->OnRenegotionNeeded
There is an inconsistency in behavior of PeerConnection.
When I remove track from PeerConnection observer->OnRenegotiationNeeded is called, however if I remove track from MediaStream then there is no notification to renegotiate.
This patch adds missing OnRenegotiationNeeded calls.
BUG=webrtc:7966
Review-Url: https://codereview.webrtc.org/2977493002
Cr-Original-Commit-Position: refs/heads/master@{#19125}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: ec390b5dfbadb9972e4fafa36226d1724c6c2204
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 1a5fd6c..f006b23 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -574,10 +574,10 @@
stream_observers_.push_back(std::unique_ptr<MediaStreamObserver>(observer));
for (const auto& track : local_stream->GetAudioTracks()) {
- OnAudioTrackAdded(track.get(), local_stream);
+ AddAudioTrack(track.get(), local_stream);
}
for (const auto& track : local_stream->GetVideoTracks()) {
- OnVideoTrackAdded(track.get(), local_stream);
+ AddVideoTrack(track.get(), local_stream);
}
stats_->AddStream(local_stream);
@@ -587,13 +587,14 @@
void PeerConnection::RemoveStream(MediaStreamInterface* local_stream) {
TRACE_EVENT0("webrtc", "PeerConnection::RemoveStream");
- for (const auto& track : local_stream->GetAudioTracks()) {
- OnAudioTrackRemoved(track.get(), local_stream);
+ if (!IsClosed()) {
+ for (const auto& track : local_stream->GetAudioTracks()) {
+ RemoveAudioTrack(track.get(), local_stream);
+ }
+ for (const auto& track : local_stream->GetVideoTracks()) {
+ RemoveVideoTrack(track.get(), local_stream);
+ }
}
- for (const auto& track : local_stream->GetVideoTracks()) {
- OnVideoTrackRemoved(track.get(), local_stream);
- }
-
local_streams_->RemoveStream(local_stream);
stream_observers_.erase(
std::remove_if(
@@ -1496,6 +1497,89 @@
}
}
+void PeerConnection::AddAudioTrack(AudioTrackInterface* track,
+ MediaStreamInterface* stream) {
+ RTC_DCHECK(!IsClosed());
+ auto sender = FindSenderForTrack(track);
+ if (sender != senders_.end()) {
+ // We already have a sender for this track, so just change the stream_id
+ // so that it's correct in the next call to CreateOffer.
+ (*sender)->internal()->set_stream_id(stream->label());
+ return;
+ }
+
+ // Normal case; we've never seen this track before.
+ rtc::scoped_refptr<RtpSenderProxyWithInternal<RtpSenderInternal>> new_sender =
+ RtpSenderProxyWithInternal<RtpSenderInternal>::Create(
+ signaling_thread(),
+ new AudioRtpSender(track, stream->label(), session_->voice_channel(),
+ stats_.get()));
+ senders_.push_back(new_sender);
+ // If the sender has already been configured in SDP, we call SetSsrc,
+ // which will connect the sender to the underlying transport. This can
+ // occur if a local session description that contains the ID of the sender
+ // is set before AddStream is called. It can also occur if the local
+ // session description is not changed and RemoveStream is called, and
+ // later AddStream is called again with the same stream.
+ const TrackInfo* track_info =
+ FindTrackInfo(local_audio_tracks_, stream->label(), track->id());
+ if (track_info) {
+ new_sender->internal()->SetSsrc(track_info->ssrc);
+ }
+}
+
+// TODO(deadbeef): Don't destroy RtpSenders here; they should be kept around
+// indefinitely, when we have unified plan SDP.
+void PeerConnection::RemoveAudioTrack(AudioTrackInterface* track,
+ MediaStreamInterface* stream) {
+ RTC_DCHECK(!IsClosed());
+ auto sender = FindSenderForTrack(track);
+ if (sender == senders_.end()) {
+ LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
+ << " doesn't exist.";
+ return;
+ }
+ (*sender)->internal()->Stop();
+ senders_.erase(sender);
+}
+
+void PeerConnection::AddVideoTrack(VideoTrackInterface* track,
+ MediaStreamInterface* stream) {
+ RTC_DCHECK(!IsClosed());
+ auto sender = FindSenderForTrack(track);
+ if (sender != senders_.end()) {
+ // We already have a sender for this track, so just change the stream_id
+ // so that it's correct in the next call to CreateOffer.
+ (*sender)->internal()->set_stream_id(stream->label());
+ return;
+ }
+
+ // Normal case; we've never seen this track before.
+ rtc::scoped_refptr<RtpSenderProxyWithInternal<RtpSenderInternal>> new_sender =
+ RtpSenderProxyWithInternal<RtpSenderInternal>::Create(
+ signaling_thread(), new VideoRtpSender(track, stream->label(),
+ session_->video_channel()));
+ senders_.push_back(new_sender);
+ const TrackInfo* track_info =
+ FindTrackInfo(local_video_tracks_, stream->label(), track->id());
+ if (track_info) {
+ new_sender->internal()->SetSsrc(track_info->ssrc);
+ }
+}
+
+void PeerConnection::RemoveVideoTrack(VideoTrackInterface* track,
+ MediaStreamInterface* stream) {
+ RTC_DCHECK(!IsClosed());
+ auto sender = FindSenderForTrack(track);
+ if (sender == senders_.end()) {
+ LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
+ << " doesn't exist.";
+ return;
+ }
+ (*sender)->internal()->Stop();
+ senders_.erase(sender);
+}
+
void PeerConnection::OnIceConnectionStateChange(
PeerConnectionInterface::IceConnectionState new_state) {
RTC_DCHECK(signaling_thread()->IsCurrent());
@@ -1563,49 +1647,17 @@
if (IsClosed()) {
return;
}
- auto sender = FindSenderForTrack(track);
- if (sender != senders_.end()) {
- // We already have a sender for this track, so just change the stream_id
- // so that it's correct in the next call to CreateOffer.
- (*sender)->internal()->set_stream_id(stream->label());
- return;
- }
-
- // Normal case; we've never seen this track before.
- rtc::scoped_refptr<RtpSenderProxyWithInternal<RtpSenderInternal>> new_sender =
- RtpSenderProxyWithInternal<RtpSenderInternal>::Create(
- signaling_thread(),
- new AudioRtpSender(track, stream->label(), session_->voice_channel(),
- stats_.get()));
- senders_.push_back(new_sender);
- // If the sender has already been configured in SDP, we call SetSsrc,
- // which will connect the sender to the underlying transport. This can
- // occur if a local session description that contains the ID of the sender
- // is set before AddStream is called. It can also occur if the local
- // session description is not changed and RemoveStream is called, and
- // later AddStream is called again with the same stream.
- const TrackInfo* track_info =
- FindTrackInfo(local_audio_tracks_, stream->label(), track->id());
- if (track_info) {
- new_sender->internal()->SetSsrc(track_info->ssrc);
- }
+ AddAudioTrack(track, stream);
+ observer_->OnRenegotiationNeeded();
}
-// TODO(deadbeef): Don't destroy RtpSenders here; they should be kept around
-// indefinitely, when we have unified plan SDP.
void PeerConnection::OnAudioTrackRemoved(AudioTrackInterface* track,
MediaStreamInterface* stream) {
if (IsClosed()) {
return;
}
- auto sender = FindSenderForTrack(track);
- if (sender == senders_.end()) {
- LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
- << " doesn't exist.";
- return;
- }
- (*sender)->internal()->Stop();
- senders_.erase(sender);
+ RemoveAudioTrack(track, stream);
+ observer_->OnRenegotiationNeeded();
}
void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track,
@@ -1613,25 +1665,8 @@
if (IsClosed()) {
return;
}
- auto sender = FindSenderForTrack(track);
- if (sender != senders_.end()) {
- // We already have a sender for this track, so just change the stream_id
- // so that it's correct in the next call to CreateOffer.
- (*sender)->internal()->set_stream_id(stream->label());
- return;
- }
-
- // Normal case; we've never seen this track before.
- rtc::scoped_refptr<RtpSenderProxyWithInternal<RtpSenderInternal>> new_sender =
- RtpSenderProxyWithInternal<RtpSenderInternal>::Create(
- signaling_thread(), new VideoRtpSender(track, stream->label(),
- session_->video_channel()));
- senders_.push_back(new_sender);
- const TrackInfo* track_info =
- FindTrackInfo(local_video_tracks_, stream->label(), track->id());
- if (track_info) {
- new_sender->internal()->SetSsrc(track_info->ssrc);
- }
+ AddVideoTrack(track, stream);
+ observer_->OnRenegotiationNeeded();
}
void PeerConnection::OnVideoTrackRemoved(VideoTrackInterface* track,
@@ -1639,14 +1674,8 @@
if (IsClosed()) {
return;
}
- auto sender = FindSenderForTrack(track);
- if (sender == senders_.end()) {
- LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
- << " doesn't exist.";
- return;
- }
- (*sender)->internal()->Stop();
- senders_.erase(sender);
+ RemoveVideoTrack(track, stream);
+ observer_->OnRenegotiationNeeded();
}
void PeerConnection::PostSetSessionDescriptionFailure(
diff --git a/pc/peerconnection.h b/pc/peerconnection.h
index 6fec348..f8b6a54 100644
--- a/pc/peerconnection.h
+++ b/pc/peerconnection.h
@@ -193,11 +193,15 @@
const std::string& track_id,
uint32_t ssrc);
void DestroyReceiver(const std::string& track_id);
- void DestroyAudioSender(MediaStreamInterface* stream,
- AudioTrackInterface* audio_track,
- uint32_t ssrc);
- void DestroyVideoSender(MediaStreamInterface* stream,
- VideoTrackInterface* video_track);
+
+ // May be called either by AddStream/RemoveStream, or when a track is
+ // added/removed from a stream previously added via AddStream.
+ void AddAudioTrack(AudioTrackInterface* track, MediaStreamInterface* stream);
+ void RemoveAudioTrack(AudioTrackInterface* track,
+ MediaStreamInterface* stream);
+ void AddVideoTrack(VideoTrackInterface* track, MediaStreamInterface* stream);
+ void RemoveVideoTrack(VideoTrackInterface* track,
+ MediaStreamInterface* stream);
// Implements IceObserver
void OnIceConnectionStateChange(IceConnectionState new_state) override;
diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc
index ce5180b..1a26a43 100644
--- a/pc/peerconnectioninterface_unittest.cc
+++ b/pc/peerconnectioninterface_unittest.cc
@@ -3736,3 +3736,37 @@
PeerConnectionInterface::RTCConfigurationType::kAggressive);
EXPECT_NE(a, h);
}
+
+// This test ensures OnRenegotiationNeeded is called when we add track with
+// MediaStream -> AddTrack in the same way it is called when we add track with
+// PeerConnection -> AddTrack.
+// The test can be removed once addStream is rewritten in terms of addTrack
+// https://bugs.chromium.org/p/webrtc/issues/detail?id=7815
+TEST_F(PeerConnectionInterfaceTest, MediaStreamAddTrackRemoveTrackRenegotiate) {
+ CreatePeerConnectionWithoutDtls();
+ rtc::scoped_refptr<MediaStreamInterface> stream(
+ pc_factory_->CreateLocalMediaStream(kStreamLabel1));
+ pc_->AddStream(stream);
+ rtc::scoped_refptr<AudioTrackInterface> audio_track(
+ pc_factory_->CreateAudioTrack("audio_track", nullptr));
+ rtc::scoped_refptr<VideoTrackInterface> video_track(
+ pc_factory_->CreateVideoTrack(
+ "video_track", pc_factory_->CreateVideoSource(
+ std::unique_ptr<cricket::VideoCapturer>(
+ new cricket::FakeVideoCapturer()))));
+ stream->AddTrack(audio_track);
+ EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout);
+ observer_.renegotiation_needed_ = false;
+
+ stream->AddTrack(video_track);
+ EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout);
+ observer_.renegotiation_needed_ = false;
+
+ stream->RemoveTrack(audio_track);
+ EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout);
+ observer_.renegotiation_needed_ = false;
+
+ stream->RemoveTrack(video_track);
+ EXPECT_TRUE_WAIT(observer_.renegotiation_needed_, kTimeout);
+ observer_.renegotiation_needed_ = false;
+}
