From 98cde26c7875497273f6bdde4b4dcb3e719386f2 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Tue, 31 May 2016 13:02:21 -0700 Subject: [PATCH] Use scoped_refptr for On(Add|Remove)Stream and OnDataChannel. This will make it much less likely for application developers to not realize the object is reference counted. It also fixes a bug in the Java PeerConnection binding, by allowing a reference to be transferred in the OnRemoveStream call via std::move. BUG=webrtc:5128 R=pthatcher@webrtc.org, tkchin@webrtc.org Review URL: https://codereview.webrtc.org/1972793003 . Cr-Commit-Position: refs/heads/master@{#12976} --- .../src/org/webrtc/PeerConnectionTest.java | 63 +++++++++++++++++-- webrtc/api/java/jni/peerconnection_jni.cc | 15 +++-- webrtc/api/peerconnection.cc | 25 ++++++-- webrtc/api/peerconnection_unittest.cc | 9 ++- webrtc/api/peerconnectionfactory_unittest.cc | 8 ++- webrtc/api/peerconnectioninterface.h | 29 ++++++--- .../api/peerconnectioninterface_unittest.cc | 8 ++- webrtc/api/test/peerconnectiontestwrapper.cc | 5 +- webrtc/api/test/peerconnectiontestwrapper.h | 9 ++- webrtc/base/scoped_ref_ptr.h | 6 ++ .../peerconnection/client/conductor.cc | 15 ++--- .../peerconnection/client/conductor.h | 9 ++- 12 files changed, 150 insertions(+), 51 deletions(-) diff --git a/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java b/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java index 809603a3b4..14ca30b23f 100644 --- a/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java +++ b/webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java @@ -746,9 +746,7 @@ public class PeerConnectionTest extends ActivityTestCase { offeringExpectations.dataChannel.close(); getMetrics(); - // Free the Java-land objects, collect them, and sleep a bit to make sure we - // don't get late-arrival crashes after the Java-land objects have been - // freed. + // Free the Java-land objects and collect them. shutdownPC(offeringPC, offeringExpectations); offeringPC = null; shutdownPC(answeringPC, answeringExpectations); @@ -959,14 +957,67 @@ public class PeerConnectionTest extends ActivityTestCase { MediaStream aRMS = answeringExpectations.gotRemoteStreams.iterator().next(); assertEquals(aRMS.videoTracks.get(0).state(), MediaStreamTrack.State.ENDED); - // Free the Java-land objects, collect them, and sleep a bit to make sure we - // don't get late-arrival crashes after the Java-land objects have been - // freed. + // Finally, remove the audio track as well, which should completely remove + // the remote stream. This used to trigger an assert. + // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=5128 + AudioTrack offererAudioTrack = oLMS.get().audioTracks.get(0); + oLMS.get().removeTrack(offererAudioTrack); + + // Create offer. + sdpLatch = new SdpObserverLatch(); + offeringPC.createOffer(sdpLatch, new MediaConstraints()); + assertTrue(sdpLatch.await()); + offerSdp = sdpLatch.getSdp(); + assertEquals(offerSdp.type, SessionDescription.Type.OFFER); + assertFalse(offerSdp.description.isEmpty()); + + // Set local description for offerer. + sdpLatch = new SdpObserverLatch(); + offeringExpectations.expectSignalingChange(SignalingState.HAVE_LOCAL_OFFER); + offeringPC.setLocalDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Set remote description for answerer. + sdpLatch = new SdpObserverLatch(); + answeringExpectations.expectSignalingChange(SignalingState.HAVE_REMOTE_OFFER); + answeringExpectations.expectRemoveStream("offeredMediaStream"); + answeringPC.setRemoteDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Create answer. + sdpLatch = new SdpObserverLatch(); + answeringPC.createAnswer(sdpLatch, new MediaConstraints()); + assertTrue(sdpLatch.await()); + answerSdp = sdpLatch.getSdp(); + assertEquals(answerSdp.type, SessionDescription.Type.ANSWER); + assertFalse(answerSdp.description.isEmpty()); + + // Set local description for answerer. + sdpLatch = new SdpObserverLatch(); + answeringExpectations.expectSignalingChange(SignalingState.STABLE); + answeringPC.setLocalDescription(sdpLatch, answerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Set remote description for offerer. + sdpLatch = new SdpObserverLatch(); + offeringExpectations.expectSignalingChange(SignalingState.STABLE); + offeringPC.setRemoteDescription(sdpLatch, answerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + // Make sure the stream was really removed. + assertTrue(answeringExpectations.gotRemoteStreams.isEmpty()); + + // Free the Java-land objects and collect them. shutdownPC(offeringPC, offeringExpectations); offeringPC = null; shutdownPC(answeringPC, answeringExpectations); answeringPC = null; offererVideoTrack.dispose(); + offererAudioTrack.dispose(); videoSource.dispose(); factory.dispose(); System.gc(); diff --git a/webrtc/api/java/jni/peerconnection_jni.cc b/webrtc/api/java/jni/peerconnection_jni.cc index a07580544f..88631719ed 100644 --- a/webrtc/api/java/jni/peerconnection_jni.cc +++ b/webrtc/api/java/jni/peerconnection_jni.cc @@ -256,14 +256,14 @@ class PCOJava : public PeerConnectionObserver { CHECK_EXCEPTION(jni()) << "error during CallVoidMethod"; } - void OnAddStream(MediaStreamInterface* stream) override { + void OnAddStream(rtc::scoped_refptr stream) override { ScopedLocalRefFrame local_ref_frame(jni()); // Java MediaStream holds one reference. Corresponding Release() is in // MediaStream_free, triggered by MediaStream.dispose(). stream->AddRef(); jobject j_stream = jni()->NewObject(*j_media_stream_class_, j_media_stream_ctor_, - reinterpret_cast(stream)); + reinterpret_cast(stream.get())); CHECK_EXCEPTION(jni()) << "error during NewObject"; for (const auto& track : stream->GetAudioTracks()) { @@ -319,7 +319,8 @@ class PCOJava : public PeerConnectionObserver { CHECK_EXCEPTION(jni()) << "error during CallVoidMethod"; } - void OnRemoveStream(MediaStreamInterface* stream) override { + void OnRemoveStream( + rtc::scoped_refptr stream) override { ScopedLocalRefFrame local_ref_frame(jni()); NativeToJavaStreamsMap::iterator it = remote_streams_.find(stream); RTC_CHECK(it != remote_streams_.end()) << "unexpected stream: " << std::hex @@ -329,13 +330,17 @@ class PCOJava : public PeerConnectionObserver { "(Lorg/webrtc/MediaStream;)V"); jni()->CallVoidMethod(*j_observer_global_, m, j_stream); CHECK_EXCEPTION(jni()) << "error during CallVoidMethod"; + // Release the refptr reference so that DisposeRemoteStream can assert + // it removes the final reference. + stream = nullptr; DisposeRemoteStream(it); } - void OnDataChannel(DataChannelInterface* channel) override { + void OnDataChannel( + rtc::scoped_refptr channel) override { ScopedLocalRefFrame local_ref_frame(jni()); jobject j_channel = jni()->NewObject( - *j_data_channel_class_, j_data_channel_ctor_, (jlong)channel); + *j_data_channel_class_, j_data_channel_ctor_, (jlong)channel.get()); CHECK_EXCEPTION(jni()) << "error during NewObject"; jmethodID m = GetMethodID(jni(), *j_observer_class_, "onDataChannel", diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index 581159f13a..5a0d32a5ff 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -1152,7 +1152,11 @@ void PeerConnection::SetRemoteDescription( for (size_t i = 0; i < new_streams->count(); ++i) { MediaStreamInterface* new_stream = new_streams->at(i); stats_->AddStream(new_stream); + // Call both the raw pointer and scoped_refptr versions of the method + // for compatibility. observer_->OnAddStream(new_stream); + observer_->OnAddStream( + rtc::scoped_refptr(new_stream)); } UpdateEndedRemoteMediaStreams(); @@ -1715,9 +1719,12 @@ void PeerConnection::UpdateEndedRemoteMediaStreams() { } } - for (const auto& stream : streams_to_remove) { + for (auto& stream : streams_to_remove) { remote_streams_->RemoveStream(stream); - observer_->OnRemoveStream(stream); + // Call both the raw pointer and scoped_refptr versions of the method + // for compatibility. + observer_->OnRemoveStream(stream.get()); + observer_->OnRemoveStream(std::move(stream)); } } @@ -1888,8 +1895,11 @@ void PeerConnection::CreateRemoteRtpDataChannel(const std::string& label, return; } channel->SetReceiveSsrc(remote_ssrc); - observer_->OnDataChannel( - DataChannelProxy::Create(signaling_thread(), channel)); + auto proxy_channel = DataChannelProxy::Create(signaling_thread(), channel); + // Call both the raw pointer and scoped_refptr versions of the method + // for compatibility. + observer_->OnDataChannel(proxy_channel.get()); + observer_->OnDataChannel(std::move(proxy_channel)); } rtc::scoped_refptr PeerConnection::InternalCreateDataChannel( @@ -2019,8 +2029,11 @@ void PeerConnection::OnDataChannelOpenMessage( return; } - observer_->OnDataChannel( - DataChannelProxy::Create(signaling_thread(), channel)); + auto proxy_channel = DataChannelProxy::Create(signaling_thread(), channel); + // Call both the raw pointer and scoped_refptr versions of the method + // for compatibility. + observer_->OnDataChannel(proxy_channel.get()); + observer_->OnDataChannel(std::move(proxy_channel)); } RtpSenderInterface* PeerConnection::FindSenderById(const std::string& id) { diff --git a/webrtc/api/peerconnection_unittest.cc b/webrtc/api/peerconnection_unittest.cc index d6506703f0..4efaa740bf 100644 --- a/webrtc/api/peerconnection_unittest.cc +++ b/webrtc/api/peerconnection_unittest.cc @@ -244,7 +244,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, webrtc::PeerConnectionInterface::SignalingState new_state) override { EXPECT_EQ(pc()->signaling_state(), new_state); } - void OnAddStream(MediaStreamInterface* media_stream) override { + void OnAddStream( + rtc::scoped_refptr media_stream) override { media_stream->RegisterObserver(this); for (size_t i = 0; i < media_stream->GetVideoTracks().size(); ++i) { const std::string id = media_stream->GetVideoTracks()[i]->id(); @@ -254,7 +255,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, media_stream->GetVideoTracks()[i])); } } - void OnRemoveStream(MediaStreamInterface* media_stream) override {} + void OnRemoveStream( + rtc::scoped_refptr media_stream) override {} void OnRenegotiationNeeded() override {} void OnIceConnectionChange( webrtc::PeerConnectionInterface::IceConnectionState new_state) override { @@ -428,7 +430,8 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver, PeerConnectionInterface::RTCOfferAnswerOptions::kUndefined; } - void OnDataChannel(DataChannelInterface* data_channel) override { + void OnDataChannel( + rtc::scoped_refptr data_channel) override { LOG(INFO) << id_ << "OnDataChannel"; data_channel_ = data_channel; data_observer_.reset(new MockDataChannelObserver(data_channel)); diff --git a/webrtc/api/peerconnectionfactory_unittest.cc b/webrtc/api/peerconnectionfactory_unittest.cc index de21e806d5..05bc6c8659 100644 --- a/webrtc/api/peerconnectionfactory_unittest.cc +++ b/webrtc/api/peerconnectionfactory_unittest.cc @@ -69,9 +69,11 @@ class NullPeerConnectionObserver : public PeerConnectionObserver { virtual void OnSignalingMessage(const std::string& msg) {} virtual void OnSignalingChange( PeerConnectionInterface::SignalingState new_state) {} - virtual void OnAddStream(MediaStreamInterface* stream) {} - virtual void OnRemoveStream(MediaStreamInterface* stream) {} - virtual void OnDataChannel(DataChannelInterface* data_channel) {} + virtual void OnAddStream(rtc::scoped_refptr stream) {} + virtual void OnRemoveStream(rtc::scoped_refptr stream) { + } + virtual void OnDataChannel( + rtc::scoped_refptr data_channel) {} virtual void OnRenegotiationNeeded() {} virtual void OnIceConnectionChange( PeerConnectionInterface::IceConnectionState new_state) {} diff --git a/webrtc/api/peerconnectioninterface.h b/webrtc/api/peerconnectioninterface.h index 4fa9bf2408..4fdbd935f9 100644 --- a/webrtc/api/peerconnectioninterface.h +++ b/webrtc/api/peerconnectioninterface.h @@ -503,27 +503,40 @@ class PeerConnectionObserver { virtual void OnSignalingChange( PeerConnectionInterface::SignalingState new_state) = 0; + // TODO(deadbeef): Once all subclasses override the scoped_refptr versions + // of the below three methods, make them pure virtual and remove the raw + // pointer version. + // Triggered when media is received on a new stream from remote peer. - virtual void OnAddStream(MediaStreamInterface* stream) = 0; + virtual void OnAddStream(rtc::scoped_refptr stream) {} + // Deprecated; please use the version that uses a scoped_refptr. + virtual void OnAddStream(MediaStreamInterface* stream) {} // Triggered when a remote peer close a stream. - virtual void OnRemoveStream(MediaStreamInterface* stream) = 0; + virtual void OnRemoveStream(rtc::scoped_refptr stream) { + } + // Deprecated; please use the version that uses a scoped_refptr. + virtual void OnRemoveStream(MediaStreamInterface* stream) {} - // Triggered when a remote peer open a data channel. - virtual void OnDataChannel(DataChannelInterface* data_channel) = 0; + // Triggered when a remote peer opens a data channel. + virtual void OnDataChannel( + rtc::scoped_refptr data_channel){}; + // Deprecated; please use the version that uses a scoped_refptr. + virtual void OnDataChannel(DataChannelInterface* data_channel) {} - // Triggered when renegotiation is needed, for example the ICE has restarted. + // Triggered when renegotiation is needed. For example, an ICE restart + // has begun. virtual void OnRenegotiationNeeded() = 0; - // Called any time the IceConnectionState changes + // Called any time the IceConnectionState changes. virtual void OnIceConnectionChange( PeerConnectionInterface::IceConnectionState new_state) = 0; - // Called any time the IceGatheringState changes + // Called any time the IceGatheringState changes. virtual void OnIceGatheringChange( PeerConnectionInterface::IceGatheringState new_state) = 0; - // New Ice candidate have been found. + // A new ICE candidate has been gathered. virtual void OnIceCandidate(const IceCandidateInterface* candidate) = 0; // Ice candidates have been removed. diff --git a/webrtc/api/peerconnectioninterface_unittest.cc b/webrtc/api/peerconnectioninterface_unittest.cc index d2f0ad1c56..0ec7478452 100644 --- a/webrtc/api/peerconnectioninterface_unittest.cc +++ b/webrtc/api/peerconnectioninterface_unittest.cc @@ -473,16 +473,18 @@ class MockPeerConnectionObserver : public PeerConnectionObserver { return remote_streams_->find(label); } StreamCollectionInterface* remote_streams() const { return remote_streams_; } - void OnAddStream(MediaStreamInterface* stream) override { + void OnAddStream(rtc::scoped_refptr stream) override { last_added_stream_ = stream; remote_streams_->AddStream(stream); } - void OnRemoveStream(MediaStreamInterface* stream) override { + void OnRemoveStream( + rtc::scoped_refptr stream) override { last_removed_stream_ = stream; remote_streams_->RemoveStream(stream); } void OnRenegotiationNeeded() override { renegotiation_needed_ = true; } - void OnDataChannel(DataChannelInterface* data_channel) override { + void OnDataChannel( + rtc::scoped_refptr data_channel) override { last_datachannel_ = data_channel; } diff --git a/webrtc/api/test/peerconnectiontestwrapper.cc b/webrtc/api/test/peerconnectiontestwrapper.cc index ed8e031afc..bc49be5459 100644 --- a/webrtc/api/test/peerconnectiontestwrapper.cc +++ b/webrtc/api/test/peerconnectiontestwrapper.cc @@ -96,7 +96,8 @@ PeerConnectionTestWrapper::CreateDataChannel( return peer_connection_->CreateDataChannel(label, &init); } -void PeerConnectionTestWrapper::OnAddStream(MediaStreamInterface* stream) { +void PeerConnectionTestWrapper::OnAddStream( + rtc::scoped_refptr stream) { LOG(LS_INFO) << "PeerConnectionTestWrapper " << name_ << ": OnAddStream"; // TODO(ronghuawu): support multiple streams. @@ -116,7 +117,7 @@ void PeerConnectionTestWrapper::OnIceCandidate( } void PeerConnectionTestWrapper::OnDataChannel( - webrtc::DataChannelInterface* data_channel) { + rtc::scoped_refptr data_channel) { SignalOnDataChannel(data_channel); } diff --git a/webrtc/api/test/peerconnectiontestwrapper.h b/webrtc/api/test/peerconnectiontestwrapper.h index 3272366c76..0fa0f7ceec 100644 --- a/webrtc/api/test/peerconnectiontestwrapper.h +++ b/webrtc/api/test/peerconnectiontestwrapper.h @@ -43,9 +43,12 @@ class PeerConnectionTestWrapper webrtc::PeerConnectionInterface::SignalingState new_state) {} virtual void OnStateChange( webrtc::PeerConnectionObserver::StateType state_changed) {} - virtual void OnAddStream(webrtc::MediaStreamInterface* stream); - virtual void OnRemoveStream(webrtc::MediaStreamInterface* stream) {} - virtual void OnDataChannel(webrtc::DataChannelInterface* data_channel); + virtual void OnAddStream( + rtc::scoped_refptr stream); + virtual void OnRemoveStream( + rtc::scoped_refptr stream) {} + virtual void OnDataChannel( + rtc::scoped_refptr data_channel); virtual void OnRenegotiationNeeded() {} virtual void OnIceConnectionChange( webrtc::PeerConnectionInterface::IceConnectionState new_state) {} diff --git a/webrtc/base/scoped_ref_ptr.h b/webrtc/base/scoped_ref_ptr.h index a71c20ae32..ceee445216 100644 --- a/webrtc/base/scoped_ref_ptr.h +++ b/webrtc/base/scoped_ref_ptr.h @@ -89,6 +89,12 @@ class scoped_refptr { ptr_->AddRef(); } + // Move constructors. + scoped_refptr(scoped_refptr&& r) : ptr_(r.release()) {} + + template + scoped_refptr(scoped_refptr&& r) : ptr_(r.release()) {} + ~scoped_refptr() { if (ptr_) ptr_->Release(); diff --git a/webrtc/examples/peerconnection/client/conductor.cc b/webrtc/examples/peerconnection/client/conductor.cc index 8ec6ed9c10..1b656512a0 100644 --- a/webrtc/examples/peerconnection/client/conductor.cc +++ b/webrtc/examples/peerconnection/client/conductor.cc @@ -156,19 +156,16 @@ void Conductor::EnsureStreamingUI() { // // Called when a remote stream is added -void Conductor::OnAddStream(webrtc::MediaStreamInterface* stream) { +void Conductor::OnAddStream( + rtc::scoped_refptr stream) { LOG(INFO) << __FUNCTION__ << " " << stream->label(); - - stream->AddRef(); - main_wnd_->QueueUIThreadCallback(NEW_STREAM_ADDED, - stream); + main_wnd_->QueueUIThreadCallback(NEW_STREAM_ADDED, stream.release()); } -void Conductor::OnRemoveStream(webrtc::MediaStreamInterface* stream) { +void Conductor::OnRemoveStream( + rtc::scoped_refptr stream) { LOG(INFO) << __FUNCTION__ << " " << stream->label(); - stream->AddRef(); - main_wnd_->QueueUIThreadCallback(STREAM_REMOVED, - stream); + main_wnd_->QueueUIThreadCallback(STREAM_REMOVED, stream.release()); } void Conductor::OnIceCandidate(const webrtc::IceCandidateInterface* candidate) { diff --git a/webrtc/examples/peerconnection/client/conductor.h b/webrtc/examples/peerconnection/client/conductor.h index 02351b78be..74e5f6c5a2 100644 --- a/webrtc/examples/peerconnection/client/conductor.h +++ b/webrtc/examples/peerconnection/client/conductor.h @@ -66,9 +66,12 @@ class Conductor void OnSignalingChange( webrtc::PeerConnectionInterface::SignalingState new_state) override{}; - void OnAddStream(webrtc::MediaStreamInterface* stream) override; - void OnRemoveStream(webrtc::MediaStreamInterface* stream) override; - void OnDataChannel(webrtc::DataChannelInterface* channel) override {} + void OnAddStream( + rtc::scoped_refptr stream) override; + void OnRemoveStream( + rtc::scoped_refptr stream) override; + void OnDataChannel( + rtc::scoped_refptr channel) override {} void OnRenegotiationNeeded() override {} void OnIceConnectionChange( webrtc::PeerConnectionInterface::IceConnectionState new_state) override{};