From be37888b6d5d269dbd5385569dba15c0d70594f2 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 17 Jul 2015 10:30:44 -0700 Subject: [PATCH] Fixing scenario where track is rejected and later un-rejected. Added `RestartLocalTracks` and `RestartRemoteTracks` methods to `MediaStreamHandlerContainer` which will redo the track handlers' initial setup; most importantly, this will re-connect the renderer/capturer/etc. to a channel which was destroyed and then re-created. Also added `AcceptRemoteTracks` method to MediaStreamSignaling, which does the inverse of `RejectRemoteTracks`. Effectively this will notify sinks that the track is live again, after previously being set to `kEnded` when it was rejected. BUG=webrtc:2136 Review URL: https://codereview.webrtc.org/1231613002 Cr-Commit-Position: refs/heads/master@{#9600} --- talk/app/webrtc/mediastreamhandler.cc | 60 +++++++++++++++++++++---- talk/app/webrtc/mediastreamhandler.h | 18 +++++++- talk/app/webrtc/mediastreamsignaling.cc | 30 ++++++++++--- talk/app/webrtc/mediastreamsignaling.h | 5 ++- talk/app/webrtc/peerconnection.cc | 16 +++++++ 5 files changed, 110 insertions(+), 19 deletions(-) diff --git a/talk/app/webrtc/mediastreamhandler.cc b/talk/app/webrtc/mediastreamhandler.cc index f68699d629..08be35ec86 100644 --- a/talk/app/webrtc/mediastreamhandler.cc +++ b/talk/app/webrtc/mediastreamhandler.cc @@ -90,7 +90,7 @@ LocalAudioTrackHandler::LocalAudioTrackHandler( audio_track_(track), provider_(provider), sink_adapter_(new LocalAudioSinkAdapter()) { - OnEnabledChanged(); + Start(); track->AddSink(sink_adapter_.get()); } @@ -101,6 +101,10 @@ void LocalAudioTrackHandler::OnStateChanged() { // TODO(perkj): What should happen when the state change? } +void LocalAudioTrackHandler::Start() { + OnEnabledChanged(); +} + void LocalAudioTrackHandler::Stop() { audio_track_->RemoveSink(sink_adapter_.get()); cricket::AudioOptions options; @@ -132,13 +136,19 @@ RemoteAudioTrackHandler::RemoteAudioTrackHandler( audio_track_(track), provider_(provider) { track->GetSource()->RegisterAudioObserver(this); - OnEnabledChanged(); + Start(); } RemoteAudioTrackHandler::~RemoteAudioTrackHandler() { audio_track_->GetSource()->UnregisterAudioObserver(this); } +void RemoteAudioTrackHandler::Start() { + // TODO(deadbeef) - Should we remember the audio playout volume last set, + // and set it again in case the audio channel was destroyed and recreated? + OnEnabledChanged(); +} + void RemoteAudioTrackHandler::Stop() { provider_->SetAudioPlayout(ssrc(), false, NULL); } @@ -166,10 +176,7 @@ LocalVideoTrackHandler::LocalVideoTrackHandler( : TrackHandler(track, ssrc), local_video_track_(track), provider_(provider) { - VideoSourceInterface* source = local_video_track_->GetSource(); - if (source) - provider_->SetCaptureDevice(ssrc, source->GetVideoCapturer()); - OnEnabledChanged(); + Start(); } LocalVideoTrackHandler::~LocalVideoTrackHandler() { @@ -178,6 +185,13 @@ LocalVideoTrackHandler::~LocalVideoTrackHandler() { void LocalVideoTrackHandler::OnStateChanged() { } +void LocalVideoTrackHandler::Start() { + VideoSourceInterface* source = local_video_track_->GetSource(); + if (source) + provider_->SetCaptureDevice(ssrc(), source->GetVideoCapturer()); + OnEnabledChanged(); +} + void LocalVideoTrackHandler::Stop() { provider_->SetCaptureDevice(ssrc(), NULL); provider_->SetVideoSend(ssrc(), false, NULL); @@ -199,14 +213,18 @@ RemoteVideoTrackHandler::RemoteVideoTrackHandler( : TrackHandler(track, ssrc), remote_video_track_(track), provider_(provider) { - OnEnabledChanged(); - provider_->SetVideoPlayout(ssrc, true, - remote_video_track_->GetSource()->FrameInput()); + Start(); } RemoteVideoTrackHandler::~RemoteVideoTrackHandler() { } +void RemoteVideoTrackHandler::Start() { + OnEnabledChanged(); + provider_->SetVideoPlayout(ssrc(), true, + remote_video_track_->GetSource()->FrameInput()); +} + void RemoteVideoTrackHandler::Stop() { // Since cricket::VideoRenderer is not reference counted // we need to remove the renderer before we are deleted. @@ -301,6 +319,12 @@ void LocalMediaStreamHandler::AddVideoTrack(VideoTrackInterface* video_track, track_handlers_.push_back(handler); } +void LocalMediaStreamHandler::RestartAllTracks() { + for (auto it : track_handlers_) { + it->Start(); + } +} + RemoteMediaStreamHandler::RemoteMediaStreamHandler( MediaStreamInterface* stream, AudioProviderInterface* audio_provider, @@ -327,6 +351,12 @@ void RemoteMediaStreamHandler::AddVideoTrack(VideoTrackInterface* video_track, track_handlers_.push_back(handler); } +void RemoteMediaStreamHandler::RestartAllTracks() { + for (auto it : track_handlers_) { + it->Start(); + } +} + MediaStreamHandlerContainer::MediaStreamHandlerContainer( AudioProviderInterface* audio_provider, VideoProviderInterface* video_provider) @@ -396,6 +426,12 @@ void MediaStreamHandlerContainer::RemoveRemoteTrack( handler->RemoveTrack(track); } +void MediaStreamHandlerContainer::RestartAllRemoteTracks() { + for (auto it : remote_streams_handlers_) { + it->RestartAllTracks(); + } +} + void MediaStreamHandlerContainer::RemoveLocalStream( MediaStreamInterface* stream) { DeleteStreamHandler(&local_streams_handlers_, stream); @@ -438,6 +474,12 @@ void MediaStreamHandlerContainer::RemoveLocalTrack( handler->RemoveTrack(track); } +void MediaStreamHandlerContainer::RestartAllLocalTracks() { + for (auto it : local_streams_handlers_) { + it->RestartAllTracks(); + } +} + MediaStreamHandler* MediaStreamHandlerContainer::CreateRemoteStreamHandler( MediaStreamInterface* stream) { ASSERT(!FindStreamHandler(remote_streams_handlers_, stream)); diff --git a/talk/app/webrtc/mediastreamhandler.h b/talk/app/webrtc/mediastreamhandler.h index 801648dc04..f79eff367a 100644 --- a/talk/app/webrtc/mediastreamhandler.h +++ b/talk/app/webrtc/mediastreamhandler.h @@ -51,6 +51,8 @@ class TrackHandler : public ObserverInterface { TrackHandler(MediaStreamTrackInterface* track, uint32 ssrc); virtual ~TrackHandler(); virtual void OnChanged(); + // Associate |track_| with |ssrc_|. Can be called multiple times. + virtual void Start() = 0; // Stop using |track_| on this PeerConnection. virtual void Stop() = 0; @@ -101,7 +103,7 @@ class LocalAudioTrackHandler : public TrackHandler { uint32 ssrc, AudioProviderInterface* provider); virtual ~LocalAudioTrackHandler(); - + void Start() override; void Stop() override; protected: @@ -127,6 +129,7 @@ class RemoteAudioTrackHandler : public AudioSourceInterface::AudioObserver, uint32 ssrc, AudioProviderInterface* provider); virtual ~RemoteAudioTrackHandler(); + void Start() override; void Stop() override; protected: @@ -150,6 +153,7 @@ class LocalVideoTrackHandler : public TrackHandler { uint32 ssrc, VideoProviderInterface* provider); virtual ~LocalVideoTrackHandler(); + void Start() override; void Stop() override; protected: @@ -170,6 +174,7 @@ class RemoteVideoTrackHandler : public TrackHandler { uint32 ssrc, VideoProviderInterface* provider); virtual ~RemoteVideoTrackHandler(); + void Start() override; void Stop() override; protected: @@ -192,6 +197,7 @@ class MediaStreamHandler : public ObserverInterface { virtual void AddAudioTrack(AudioTrackInterface* audio_track, uint32 ssrc) = 0; virtual void AddVideoTrack(VideoTrackInterface* video_track, uint32 ssrc) = 0; + virtual void RestartAllTracks() = 0; virtual void RemoveTrack(MediaStreamTrackInterface* track); void OnChanged() override; @@ -214,6 +220,7 @@ class LocalMediaStreamHandler : public MediaStreamHandler { void AddAudioTrack(AudioTrackInterface* audio_track, uint32 ssrc) override; void AddVideoTrack(VideoTrackInterface* video_track, uint32 ssrc) override; + void RestartAllTracks() override; }; class RemoteMediaStreamHandler : public MediaStreamHandler { @@ -224,6 +231,7 @@ class RemoteMediaStreamHandler : public MediaStreamHandler { ~RemoteMediaStreamHandler(); void AddAudioTrack(AudioTrackInterface* audio_track, uint32 ssrc) override; void AddVideoTrack(VideoTrackInterface* video_track, uint32 ssrc) override; + void RestartAllTracks() override; }; // Container for MediaStreamHandlers of currently known local and remote @@ -256,6 +264,10 @@ class MediaStreamHandlerContainer { void RemoveRemoteTrack(MediaStreamInterface* stream, MediaStreamTrackInterface* track); + // Make all remote track handlers reassociate their corresponding tracks + // with their corresponding SSRCs. + void RestartAllRemoteTracks(); + // Remove all TrackHandlers for tracks in |stream| and make sure // the audio_provider and video_provider is notified that the tracks has been // removed. @@ -273,6 +285,10 @@ class MediaStreamHandlerContainer { void RemoveLocalTrack(MediaStreamInterface* stream, MediaStreamTrackInterface* track); + // Make all local track handlers reassociate their corresponding tracks + // with their corresponding SSRCs. + void RestartAllLocalTracks(); + private: typedef std::list StreamHandlerList; MediaStreamHandler* FindStreamHandler(const StreamHandlerList& handlers, diff --git a/talk/app/webrtc/mediastreamsignaling.cc b/talk/app/webrtc/mediastreamsignaling.cc index 1f5f14fd7b..d126ac5967 100644 --- a/talk/app/webrtc/mediastreamsignaling.cc +++ b/talk/app/webrtc/mediastreamsignaling.cc @@ -525,7 +525,13 @@ void MediaStreamSignaling::OnLocalDescriptionChanged( GetFirstAudioContent(desc->description()); if (audio_content) { if (audio_content->rejected) { - RejectRemoteTracks(cricket::MEDIA_TYPE_AUDIO); + SetRemoteTracksState(cricket::MEDIA_TYPE_AUDIO, + MediaStreamTrackInterface::kEnded); + } else { + // This is needed in case the local description caused the track to be + // rejected, then later accepted, without being destroyed. + SetRemoteTracksState(cricket::MEDIA_TYPE_AUDIO, + MediaStreamTrackInterface::kLive); } const cricket::AudioContentDescription* audio_desc = static_cast( @@ -537,7 +543,13 @@ void MediaStreamSignaling::OnLocalDescriptionChanged( GetFirstVideoContent(desc->description()); if (video_content) { if (video_content->rejected) { - RejectRemoteTracks(cricket::MEDIA_TYPE_VIDEO); + SetRemoteTracksState(cricket::MEDIA_TYPE_VIDEO, + MediaStreamTrackInterface::kEnded); + } else { + // This is needed in case the local description caused the track to be + // rejected, then later accepted, without being destroyed. + SetRemoteTracksState(cricket::MEDIA_TYPE_VIDEO, + MediaStreamTrackInterface::kLive); } const cricket::VideoContentDescription* video_desc = static_cast( @@ -559,11 +571,13 @@ void MediaStreamSignaling::OnLocalDescriptionChanged( } void MediaStreamSignaling::OnAudioChannelClose() { - RejectRemoteTracks(cricket::MEDIA_TYPE_AUDIO); + SetRemoteTracksState(cricket::MEDIA_TYPE_AUDIO, + MediaStreamTrackInterface::kEnded); } void MediaStreamSignaling::OnVideoChannelClose() { - RejectRemoteTracks(cricket::MEDIA_TYPE_VIDEO); + SetRemoteTracksState(cricket::MEDIA_TYPE_VIDEO, + MediaStreamTrackInterface::kEnded); } void MediaStreamSignaling::OnDataChannelClose() { @@ -678,7 +692,9 @@ void MediaStreamSignaling::OnRemoteTrackRemoved( } } -void MediaStreamSignaling::RejectRemoteTracks(cricket::MediaType media_type) { +void MediaStreamSignaling::SetRemoteTracksState( + cricket::MediaType media_type, + MediaStreamTrackInterface::TrackState state) { TrackInfos* current_tracks = GetRemoteTracks(media_type); for (TrackInfos::iterator track_it = current_tracks->begin(); track_it != current_tracks->end(); ++track_it) { @@ -689,7 +705,7 @@ void MediaStreamSignaling::RejectRemoteTracks(cricket::MediaType media_type) { // There's no guarantee the track is still available, e.g. the track may // have been removed from the stream by javascript. if (track) { - track->set_state(webrtc::MediaStreamTrackInterface::kEnded); + track->set_state(state); } } if (media_type == cricket::MEDIA_TYPE_VIDEO) { @@ -697,7 +713,7 @@ void MediaStreamSignaling::RejectRemoteTracks(cricket::MediaType media_type) { // There's no guarantee the track is still available, e.g. the track may // have been removed from the stream by javascript. if (track) { - track->set_state(webrtc::MediaStreamTrackInterface::kEnded); + track->set_state(state); } } } diff --git a/talk/app/webrtc/mediastreamsignaling.h b/talk/app/webrtc/mediastreamsignaling.h index 87eede6305..9109d09f36 100644 --- a/talk/app/webrtc/mediastreamsignaling.h +++ b/talk/app/webrtc/mediastreamsignaling.h @@ -323,9 +323,10 @@ class MediaStreamSignaling : public sigslot::has_slots<> { const std::string& track_id, cricket::MediaType media_type); - // Set the MediaStreamTrackInterface::TrackState to |kEnded| on all remote + // Set the MediaStreamTrackInterface::TrackState to |state| on all remote // tracks of type |media_type|. - void RejectRemoteTracks(cricket::MediaType media_type); + void SetRemoteTracksState(cricket::MediaType media_type, + MediaStreamTrackInterface::TrackState state); // Finds remote MediaStreams without any tracks and removes them from // |remote_streams_| and notifies the observer that the MediaStream no longer diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index dba77da219..fa49c77e12 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -615,6 +615,14 @@ void PeerConnection::SetLocalDescription( PostSetSessionDescriptionFailure(observer, error); return; } + + // This is necessary because an audio/video channel may have been previously + // destroyed by removing it from the remote description, which would NOT + // destroy the local handler. So upon receiving a new local description, + // we may need to tell that local track handler to connect the capturer + // to the now re-created audio/video channel. + stream_handler_container_->RestartAllLocalTracks(); + SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer); signaling_thread()->Post(this, MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg); } @@ -638,6 +646,14 @@ void PeerConnection::SetRemoteDescription( PostSetSessionDescriptionFailure(observer, error); return; } + + // This is necessary because an audio/video channel may have been previously + // destroyed by removing it from the local description, which would NOT + // destroy the remote handler. So upon receiving a new remote description, + // we may need to tell that remote track handler to connect the renderer + // to the now re-created audio/video channel. + stream_handler_container_->RestartAllRemoteTracks(); + SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer); signaling_thread()->Post(this, MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg); }