From 66f438f8c37165db669e15314cdb7bbfa0841f4c Mon Sep 17 00:00:00 2001 From: magjed Date: Thu, 23 Jul 2015 06:02:37 -0700 Subject: [PATCH] Revert of Fixing scenario where track is rejected and later un-rejected. (patchset #5 id:80001 of https://codereview.webrtc.org/1231613002/) Reason for revert: I think this causes WebRtcBrowserTest.CallAndModifyStream to fail on Android. See https://code.google.com/p/webrtc/issues/detail?id=4857 for more info. Original issue's description: > 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 > > Committed: https://crrev.com/be37888b6d5d269dbd5385569dba15c0d70594f2 > Cr-Commit-Position: refs/heads/master@{#9600} TBR=pthatcher@webrtc.org,juberti@webrtc.org,deadbeef@webrtc.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:2136 Review URL: https://codereview.webrtc.org/1247443005 Cr-Commit-Position: refs/heads/master@{#9622} --- 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, 19 insertions(+), 110 deletions(-) diff --git a/talk/app/webrtc/mediastreamhandler.cc b/talk/app/webrtc/mediastreamhandler.cc index 08be35ec86..f68699d629 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()) { - Start(); + OnEnabledChanged(); track->AddSink(sink_adapter_.get()); } @@ -101,10 +101,6 @@ 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; @@ -136,19 +132,13 @@ RemoteAudioTrackHandler::RemoteAudioTrackHandler( audio_track_(track), provider_(provider) { track->GetSource()->RegisterAudioObserver(this); - Start(); + OnEnabledChanged(); } 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); } @@ -176,7 +166,10 @@ LocalVideoTrackHandler::LocalVideoTrackHandler( : TrackHandler(track, ssrc), local_video_track_(track), provider_(provider) { - Start(); + VideoSourceInterface* source = local_video_track_->GetSource(); + if (source) + provider_->SetCaptureDevice(ssrc, source->GetVideoCapturer()); + OnEnabledChanged(); } LocalVideoTrackHandler::~LocalVideoTrackHandler() { @@ -185,13 +178,6 @@ 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); @@ -213,18 +199,14 @@ RemoteVideoTrackHandler::RemoteVideoTrackHandler( : TrackHandler(track, ssrc), remote_video_track_(track), provider_(provider) { - Start(); + OnEnabledChanged(); + provider_->SetVideoPlayout(ssrc, true, + remote_video_track_->GetSource()->FrameInput()); } 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. @@ -319,12 +301,6 @@ 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, @@ -351,12 +327,6 @@ 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) @@ -426,12 +396,6 @@ 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); @@ -474,12 +438,6 @@ 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 f79eff367a..801648dc04 100644 --- a/talk/app/webrtc/mediastreamhandler.h +++ b/talk/app/webrtc/mediastreamhandler.h @@ -51,8 +51,6 @@ 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; @@ -103,7 +101,7 @@ class LocalAudioTrackHandler : public TrackHandler { uint32 ssrc, AudioProviderInterface* provider); virtual ~LocalAudioTrackHandler(); - void Start() override; + void Stop() override; protected: @@ -129,7 +127,6 @@ class RemoteAudioTrackHandler : public AudioSourceInterface::AudioObserver, uint32 ssrc, AudioProviderInterface* provider); virtual ~RemoteAudioTrackHandler(); - void Start() override; void Stop() override; protected: @@ -153,7 +150,6 @@ class LocalVideoTrackHandler : public TrackHandler { uint32 ssrc, VideoProviderInterface* provider); virtual ~LocalVideoTrackHandler(); - void Start() override; void Stop() override; protected: @@ -174,7 +170,6 @@ class RemoteVideoTrackHandler : public TrackHandler { uint32 ssrc, VideoProviderInterface* provider); virtual ~RemoteVideoTrackHandler(); - void Start() override; void Stop() override; protected: @@ -197,7 +192,6 @@ 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; @@ -220,7 +214,6 @@ 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 { @@ -231,7 +224,6 @@ 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 @@ -264,10 +256,6 @@ 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. @@ -285,10 +273,6 @@ 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 d126ac5967..1f5f14fd7b 100644 --- a/talk/app/webrtc/mediastreamsignaling.cc +++ b/talk/app/webrtc/mediastreamsignaling.cc @@ -525,13 +525,7 @@ void MediaStreamSignaling::OnLocalDescriptionChanged( GetFirstAudioContent(desc->description()); if (audio_content) { if (audio_content->rejected) { - 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); + RejectRemoteTracks(cricket::MEDIA_TYPE_AUDIO); } const cricket::AudioContentDescription* audio_desc = static_cast( @@ -543,13 +537,7 @@ void MediaStreamSignaling::OnLocalDescriptionChanged( GetFirstVideoContent(desc->description()); if (video_content) { if (video_content->rejected) { - 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); + RejectRemoteTracks(cricket::MEDIA_TYPE_VIDEO); } const cricket::VideoContentDescription* video_desc = static_cast( @@ -571,13 +559,11 @@ void MediaStreamSignaling::OnLocalDescriptionChanged( } void MediaStreamSignaling::OnAudioChannelClose() { - SetRemoteTracksState(cricket::MEDIA_TYPE_AUDIO, - MediaStreamTrackInterface::kEnded); + RejectRemoteTracks(cricket::MEDIA_TYPE_AUDIO); } void MediaStreamSignaling::OnVideoChannelClose() { - SetRemoteTracksState(cricket::MEDIA_TYPE_VIDEO, - MediaStreamTrackInterface::kEnded); + RejectRemoteTracks(cricket::MEDIA_TYPE_VIDEO); } void MediaStreamSignaling::OnDataChannelClose() { @@ -692,9 +678,7 @@ void MediaStreamSignaling::OnRemoteTrackRemoved( } } -void MediaStreamSignaling::SetRemoteTracksState( - cricket::MediaType media_type, - MediaStreamTrackInterface::TrackState state) { +void MediaStreamSignaling::RejectRemoteTracks(cricket::MediaType media_type) { TrackInfos* current_tracks = GetRemoteTracks(media_type); for (TrackInfos::iterator track_it = current_tracks->begin(); track_it != current_tracks->end(); ++track_it) { @@ -705,7 +689,7 @@ void MediaStreamSignaling::SetRemoteTracksState( // 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(state); + track->set_state(webrtc::MediaStreamTrackInterface::kEnded); } } if (media_type == cricket::MEDIA_TYPE_VIDEO) { @@ -713,7 +697,7 @@ void MediaStreamSignaling::SetRemoteTracksState( // 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(state); + track->set_state(webrtc::MediaStreamTrackInterface::kEnded); } } } diff --git a/talk/app/webrtc/mediastreamsignaling.h b/talk/app/webrtc/mediastreamsignaling.h index 9109d09f36..87eede6305 100644 --- a/talk/app/webrtc/mediastreamsignaling.h +++ b/talk/app/webrtc/mediastreamsignaling.h @@ -323,10 +323,9 @@ class MediaStreamSignaling : public sigslot::has_slots<> { const std::string& track_id, cricket::MediaType media_type); - // Set the MediaStreamTrackInterface::TrackState to |state| on all remote + // Set the MediaStreamTrackInterface::TrackState to |kEnded| on all remote // tracks of type |media_type|. - void SetRemoteTracksState(cricket::MediaType media_type, - MediaStreamTrackInterface::TrackState state); + void RejectRemoteTracks(cricket::MediaType media_type); // 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 fa49c77e12..dba77da219 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -615,14 +615,6 @@ 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); } @@ -646,14 +638,6 @@ 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); }