Restoring behavior where PeerConnection tracks changes to MediaStreams.
If a MediaStream is added to a PeerConnection, and later a track
is added to the MediaStream, a new RtpSender will now be created for
that track, and it will appear in subsequent offers.
Similarly, removed tracks will remove RtpSenders.
BUG=webrtc:5265
Review URL: https://codereview.webrtc.org/1507973003
Cr-Commit-Position: refs/heads/master@{#11040}
diff --git a/talk/app/webrtc/mediastreamobserver.cc b/talk/app/webrtc/mediastreamobserver.cc
index a856164..2650b9a 100644
--- a/talk/app/webrtc/mediastreamobserver.cc
+++ b/talk/app/webrtc/mediastreamobserver.cc
@@ -27,4 +27,75 @@
#include "talk/app/webrtc/mediastreamobserver.h"
-// This file is currently stubbed so that Chromium's build files can be updated.
+#include <algorithm>
+
+namespace webrtc {
+
+MediaStreamObserver::MediaStreamObserver(MediaStreamInterface* stream)
+ : stream_(stream),
+ cached_audio_tracks_(stream->GetAudioTracks()),
+ cached_video_tracks_(stream->GetVideoTracks()) {
+ stream_->RegisterObserver(this);
+}
+
+MediaStreamObserver::~MediaStreamObserver() {
+ stream_->UnregisterObserver(this);
+}
+
+void MediaStreamObserver::OnChanged() {
+ AudioTrackVector new_audio_tracks = stream_->GetAudioTracks();
+ VideoTrackVector new_video_tracks = stream_->GetVideoTracks();
+
+ // Find removed audio tracks.
+ for (const auto& cached_track : cached_audio_tracks_) {
+ auto it = std::find_if(
+ new_audio_tracks.begin(), new_audio_tracks.end(),
+ [cached_track](const AudioTrackVector::value_type& new_track) {
+ return new_track->id().compare(cached_track->id()) == 0;
+ });
+ if (it == new_audio_tracks.end()) {
+ SignalAudioTrackRemoved(cached_track.get(), stream_);
+ }
+ }
+
+ // Find added audio tracks.
+ for (const auto& new_track : new_audio_tracks) {
+ auto it = std::find_if(
+ cached_audio_tracks_.begin(), cached_audio_tracks_.end(),
+ [new_track](const AudioTrackVector::value_type& cached_track) {
+ return new_track->id().compare(cached_track->id()) == 0;
+ });
+ if (it == cached_audio_tracks_.end()) {
+ SignalAudioTrackAdded(new_track.get(), stream_);
+ }
+ }
+
+ // Find removed video tracks.
+ for (const auto& cached_track : cached_video_tracks_) {
+ auto it = std::find_if(
+ new_video_tracks.begin(), new_video_tracks.end(),
+ [cached_track](const VideoTrackVector::value_type& new_track) {
+ return new_track->id().compare(cached_track->id()) == 0;
+ });
+ if (it == new_video_tracks.end()) {
+ SignalVideoTrackRemoved(cached_track.get(), stream_);
+ }
+ }
+
+ // Find added video tracks.
+ for (const auto& new_track : new_video_tracks) {
+ auto it = std::find_if(
+ cached_video_tracks_.begin(), cached_video_tracks_.end(),
+ [new_track](const VideoTrackVector::value_type& cached_track) {
+ return new_track->id().compare(cached_track->id()) == 0;
+ });
+ if (it == cached_video_tracks_.end()) {
+ SignalVideoTrackAdded(new_track.get(), stream_);
+ }
+ }
+
+ cached_audio_tracks_ = new_audio_tracks;
+ cached_video_tracks_ = new_video_tracks;
+}
+
+} // namespace webrtc
diff --git a/talk/app/webrtc/mediastreamobserver.h b/talk/app/webrtc/mediastreamobserver.h
index d6128f3..1dd6c4c 100644
--- a/talk/app/webrtc/mediastreamobserver.h
+++ b/talk/app/webrtc/mediastreamobserver.h
@@ -25,9 +25,41 @@
* ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
-// This file is currently stubbed so that Chromium's build files can be updated.
-
#ifndef TALK_APP_WEBRTC_MEDIASTREAMOBSERVER_H_
#define TALK_APP_WEBRTC_MEDIASTREAMOBSERVER_H_
+#include "talk/app/webrtc/mediastreaminterface.h"
+#include "webrtc/base/scoped_ref_ptr.h"
+#include "webrtc/base/sigslot.h"
+
+namespace webrtc {
+
+// Helper class which will listen for changes to a stream and emit the
+// corresponding signals.
+class MediaStreamObserver : public ObserverInterface {
+ public:
+ explicit MediaStreamObserver(MediaStreamInterface* stream);
+ ~MediaStreamObserver();
+
+ const MediaStreamInterface* stream() const { return stream_; }
+
+ void OnChanged() override;
+
+ sigslot::signal2<AudioTrackInterface*, MediaStreamInterface*>
+ SignalAudioTrackAdded;
+ sigslot::signal2<AudioTrackInterface*, MediaStreamInterface*>
+ SignalAudioTrackRemoved;
+ sigslot::signal2<VideoTrackInterface*, MediaStreamInterface*>
+ SignalVideoTrackAdded;
+ sigslot::signal2<VideoTrackInterface*, MediaStreamInterface*>
+ SignalVideoTrackRemoved;
+
+ private:
+ rtc::scoped_refptr<MediaStreamInterface> stream_;
+ AudioTrackVector cached_audio_tracks_;
+ VideoTrackVector cached_video_tracks_;
+};
+
+} // namespace webrtc
+
#endif // TALK_APP_WEBRTC_MEDIASTREAMOBSERVER_H_
diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc
index 6cca166..3a38248 100644
--- a/talk/app/webrtc/peerconnection.cc
+++ b/talk/app/webrtc/peerconnection.cc
@@ -27,6 +27,7 @@
#include "talk/app/webrtc/peerconnection.h"
+#include <algorithm>
#include <vector>
#include <cctype> // for isdigit
@@ -36,6 +37,7 @@
#include "talk/app/webrtc/jsepsessiondescription.h"
#include "talk/app/webrtc/mediaconstraintsinterface.h"
#include "talk/app/webrtc/mediastream.h"
+#include "talk/app/webrtc/mediastreamobserver.h"
#include "talk/app/webrtc/mediastreamproxy.h"
#include "talk/app/webrtc/mediastreamtrackproxy.h"
#include "talk/app/webrtc/remoteaudiosource.h"
@@ -740,48 +742,22 @@
}
local_streams_->AddStream(local_stream);
+ MediaStreamObserver* observer = new MediaStreamObserver(local_stream);
+ observer->SignalAudioTrackAdded.connect(this,
+ &PeerConnection::OnAudioTrackAdded);
+ observer->SignalAudioTrackRemoved.connect(
+ this, &PeerConnection::OnAudioTrackRemoved);
+ observer->SignalVideoTrackAdded.connect(this,
+ &PeerConnection::OnVideoTrackAdded);
+ observer->SignalVideoTrackRemoved.connect(
+ this, &PeerConnection::OnVideoTrackRemoved);
+ stream_observers_.push_back(rtc::scoped_ptr<MediaStreamObserver>(observer));
for (const auto& track : local_stream->GetAudioTracks()) {
- auto sender = FindSenderForTrack(track.get());
- if (sender == senders_.end()) {
- // Normal case; we've never seen this track before.
- AudioRtpSender* new_sender = new AudioRtpSender(
- track.get(), local_stream->label(), session_.get(), 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_, local_stream->label(), track->id());
- if (track_info) {
- new_sender->SetSsrc(track_info->ssrc);
- }
- } else {
- // 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)->set_stream_id(local_stream->label());
- }
+ OnAudioTrackAdded(track.get(), local_stream);
}
for (const auto& track : local_stream->GetVideoTracks()) {
- auto sender = FindSenderForTrack(track.get());
- if (sender == senders_.end()) {
- // Normal case; we've never seen this track before.
- VideoRtpSender* new_sender = new VideoRtpSender(
- track.get(), local_stream->label(), session_.get());
- senders_.push_back(new_sender);
- const TrackInfo* track_info = FindTrackInfo(
- local_video_tracks_, local_stream->label(), track->id());
- if (track_info) {
- new_sender->SetSsrc(track_info->ssrc);
- }
- } else {
- // 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)->set_stream_id(local_stream->label());
- }
+ OnVideoTrackAdded(track.get(), local_stream);
}
stats_->AddStream(local_stream);
@@ -789,32 +765,24 @@
return true;
}
-// TODO(deadbeef): Don't destroy RtpSenders here; they should be kept around
-// indefinitely, when we have unified plan SDP.
void PeerConnection::RemoveStream(MediaStreamInterface* local_stream) {
TRACE_EVENT0("webrtc", "PeerConnection::RemoveStream");
for (const auto& track : local_stream->GetAudioTracks()) {
- auto sender = FindSenderForTrack(track.get());
- if (sender == senders_.end()) {
- LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
- << " doesn't exist.";
- continue;
- }
- (*sender)->Stop();
- senders_.erase(sender);
+ OnAudioTrackRemoved(track.get(), local_stream);
}
for (const auto& track : local_stream->GetVideoTracks()) {
- auto sender = FindSenderForTrack(track.get());
- if (sender == senders_.end()) {
- LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
- << " doesn't exist.";
- continue;
- }
- (*sender)->Stop();
- senders_.erase(sender);
+ OnVideoTrackRemoved(track.get(), local_stream);
}
local_streams_->RemoveStream(local_stream);
+ stream_observers_.erase(
+ std::remove_if(
+ stream_observers_.begin(), stream_observers_.end(),
+ [local_stream](const rtc::scoped_ptr<MediaStreamObserver>& observer) {
+ return observer->stream()->label().compare(local_stream->label()) ==
+ 0;
+ }),
+ stream_observers_.end());
if (IsClosed()) {
return;
@@ -1432,6 +1400,80 @@
observer_->OnStateChange(PeerConnectionObserver::kSignalingState);
}
+void PeerConnection::OnAudioTrackAdded(AudioTrackInterface* track,
+ MediaStreamInterface* stream) {
+ 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)->set_stream_id(stream->label());
+ return;
+ }
+
+ // Normal case; we've never seen this track before.
+ AudioRtpSender* new_sender =
+ new AudioRtpSender(track, stream->label(), session_.get(), 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->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::OnAudioTrackRemoved(AudioTrackInterface* track,
+ MediaStreamInterface* stream) {
+ auto sender = FindSenderForTrack(track);
+ if (sender == senders_.end()) {
+ LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
+ << " doesn't exist.";
+ return;
+ }
+ (*sender)->Stop();
+ senders_.erase(sender);
+}
+
+void PeerConnection::OnVideoTrackAdded(VideoTrackInterface* track,
+ MediaStreamInterface* stream) {
+ 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)->set_stream_id(stream->label());
+ return;
+ }
+
+ // Normal case; we've never seen this track before.
+ VideoRtpSender* new_sender =
+ new VideoRtpSender(track, stream->label(), session_.get());
+ senders_.push_back(new_sender);
+ const TrackInfo* track_info =
+ FindTrackInfo(local_video_tracks_, stream->label(), track->id());
+ if (track_info) {
+ new_sender->SetSsrc(track_info->ssrc);
+ }
+}
+
+void PeerConnection::OnVideoTrackRemoved(VideoTrackInterface* track,
+ MediaStreamInterface* stream) {
+ auto sender = FindSenderForTrack(track);
+ if (sender == senders_.end()) {
+ LOG(LS_WARNING) << "RtpSender for track with id " << track->id()
+ << " doesn't exist.";
+ return;
+ }
+ (*sender)->Stop();
+ senders_.erase(sender);
+}
+
void PeerConnection::PostSetSessionDescriptionFailure(
SetSessionDescriptionObserver* observer,
const std::string& error) {
diff --git a/talk/app/webrtc/peerconnection.h b/talk/app/webrtc/peerconnection.h
index 9cb3635..f6c4fc3 100644
--- a/talk/app/webrtc/peerconnection.h
+++ b/talk/app/webrtc/peerconnection.h
@@ -42,6 +42,7 @@
namespace webrtc {
+class MediaStreamObserver;
class RemoteMediaStreamFactory;
typedef std::vector<PortAllocatorFactoryInterface::StunConfiguration>
@@ -201,6 +202,16 @@
void OnSessionStateChange(WebRtcSession* session, WebRtcSession::State state);
void ChangeSignalingState(SignalingState signaling_state);
+ // Signals from MediaStreamObserver.
+ void OnAudioTrackAdded(AudioTrackInterface* track,
+ MediaStreamInterface* stream);
+ void OnAudioTrackRemoved(AudioTrackInterface* track,
+ MediaStreamInterface* stream);
+ void OnVideoTrackAdded(VideoTrackInterface* track,
+ MediaStreamInterface* stream);
+ void OnVideoTrackRemoved(VideoTrackInterface* track,
+ MediaStreamInterface* stream);
+
rtc::Thread* signaling_thread() const {
return factory_->signaling_thread();
}
@@ -364,6 +375,8 @@
// Streams created as a result of SetRemoteDescription.
rtc::scoped_refptr<StreamCollection> remote_streams_;
+ std::vector<rtc::scoped_ptr<MediaStreamObserver>> stream_observers_;
+
// These lists store track info seen in local/remote descriptions.
TrackInfos remote_audio_tracks_;
TrackInfos remote_video_tracks_;
diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc
index edf7593..5e23931 100644
--- a/talk/app/webrtc/peerconnectioninterface_unittest.cc
+++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc
@@ -1156,6 +1156,48 @@
EXPECT_NE(audio_ssrc, video_ssrc);
}
+// Test that it's possible to call AddTrack on a MediaStream after adding
+// the stream to a PeerConnection.
+// TODO(deadbeef): Remove this test once this behavior is no longer supported.
+TEST_F(PeerConnectionInterfaceTest, AddTrackAfterAddStream) {
+ CreatePeerConnection();
+ // Create audio stream and add to PeerConnection.
+ AddVoiceStream(kStreamLabel1);
+ MediaStreamInterface* stream = pc_->local_streams()->at(0);
+
+ // Add video track to the audio-only stream.
+ scoped_refptr<VideoTrackInterface> video_track(
+ pc_factory_->CreateVideoTrack("video_label", nullptr));
+ stream->AddTrack(video_track.get());
+
+ scoped_ptr<SessionDescriptionInterface> offer;
+ ASSERT_TRUE(DoCreateOffer(offer.use(), nullptr));
+
+ const cricket::MediaContentDescription* video_desc =
+ cricket::GetFirstVideoContentDescription(offer->description());
+ EXPECT_TRUE(video_desc != nullptr);
+}
+
+// Test that it's possible to call RemoveTrack on a MediaStream after adding
+// the stream to a PeerConnection.
+// TODO(deadbeef): Remove this test once this behavior is no longer supported.
+TEST_F(PeerConnectionInterfaceTest, RemoveTrackAfterAddStream) {
+ CreatePeerConnection();
+ // Create audio/video stream and add to PeerConnection.
+ AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label");
+ MediaStreamInterface* stream = pc_->local_streams()->at(0);
+
+ // Remove the video track.
+ stream->RemoveTrack(stream->GetVideoTracks()[0]);
+
+ scoped_ptr<SessionDescriptionInterface> offer;
+ ASSERT_TRUE(DoCreateOffer(offer.use(), nullptr));
+
+ const cricket::MediaContentDescription* video_desc =
+ cricket::GetFirstVideoContentDescription(offer->description());
+ EXPECT_TRUE(video_desc == nullptr);
+}
+
// Test that we can specify a certain track that we want statistics about.
TEST_F(PeerConnectionInterfaceTest, GetStatsForSpecificTrack) {
InitiateCall();
diff --git a/talk/libjingle.gyp b/talk/libjingle.gyp
index e8035fe..e3480ca 100755
--- a/talk/libjingle.gyp
+++ b/talk/libjingle.gyp
@@ -746,6 +746,8 @@
'app/webrtc/mediastream.cc',
'app/webrtc/mediastream.h',
'app/webrtc/mediastreaminterface.h',
+ 'app/webrtc/mediastreamobserver.cc',
+ 'app/webrtc/mediastreamobserver.h',
'app/webrtc/mediastreamprovider.h',
'app/webrtc/mediastreamproxy.h',
'app/webrtc/mediastreamtrack.h',
