From ebe36efad70d70848964a7d8349e71809f69093b Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Mon, 9 Oct 2017 17:16:54 -0700 Subject: [PATCH] Update Java MediaStream when native stream's set of tracks changes. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will handle the scenario where, for example, the initial offer/answer only negotiates audio, and video is added later (to the same stream). Previously, there was absolutely no way to get a handle to the new track without hacking the SDP. Now, the stream will be updated after setRemoteDescription finishes. Bug: webrtc:5677 Change-Id: Iea31bb7744da6b82afdaf44c8f74d721298a9474 Reviewed-on: https://webrtc-review.googlesource.com/6261 Reviewed-by: Sami Kalliomäki Commit-Queue: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#20228} --- .../src/org/webrtc/PeerConnectionTest.java | 160 ++++++++++++++-- sdk/android/src/jni/classreferenceholder.cc | 1 + sdk/android/src/jni/jni_helpers.cc | 6 + sdk/android/src/jni/jni_helpers.h | 5 + .../src/jni/pc/peerconnectionobserver_jni.cc | 173 ++++++++++++++---- .../src/jni/pc/peerconnectionobserver_jni.h | 44 ++++- 6 files changed, 339 insertions(+), 50 deletions(-) diff --git a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java index eff28eccb2..d2f024d33b 100644 --- a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java +++ b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java @@ -158,7 +158,7 @@ public class PeerConnectionTest { } if (expectedIceConnectionChanges.isEmpty()) { - System.out.println(name + "Got an unexpected ice connection change " + newState); + System.out.println(name + "Got an unexpected ICE connection change " + newState); return; } @@ -167,7 +167,7 @@ public class PeerConnectionTest { @Override public synchronized void onIceConnectionReceivingChange(boolean receiving) { - System.out.println(name + "Got an ice connection receiving change " + receiving); + System.out.println(name + "Got an ICE connection receiving change " + receiving); } public synchronized void expectIceGatheringChange(IceGatheringState newState) { @@ -182,6 +182,9 @@ public class PeerConnectionTest { if (newState == IceGatheringState.GATHERING) { return; } + if (expectedIceGatheringChanges.isEmpty()) { + System.out.println(name + "Got an unexpected ICE gathering change " + newState); + } assertEquals(expectedIceGatheringChanges.removeFirst(), newState); } @@ -192,15 +195,15 @@ public class PeerConnectionTest { @Override public synchronized void onAddStream(MediaStream stream) { assertEquals(expectedAddStreamLabels.removeFirst(), stream.label()); - assertEquals(1, stream.videoTracks.size()); - assertEquals(1, stream.audioTracks.size()); - assertTrue(stream.videoTracks.get(0).id().endsWith("VideoTrack")); - assertTrue(stream.audioTracks.get(0).id().endsWith("AudioTrack")); - assertEquals("video", stream.videoTracks.get(0).kind()); - assertEquals("audio", stream.audioTracks.get(0).kind()); - VideoRenderer renderer = createVideoRenderer(this); - stream.videoTracks.get(0).addRenderer(renderer); - assertNull(renderers.put(stream, new WeakReference(renderer))); + for (AudioTrack track : stream.audioTracks) { + assertEquals("audio", track.kind()); + } + for (VideoTrack track : stream.videoTracks) { + assertEquals("video", track.kind()); + VideoRenderer renderer = createVideoRenderer(this); + track.addRenderer(renderer); + assertNull(renderers.put(stream, new WeakReference(renderer))); + } gotRemoteStreams.add(stream); } @@ -619,7 +622,7 @@ public class PeerConnectionTest { // instead of in addTracksToPC. final CameraEnumerator enumerator = new Camera1Enumerator(false /* captureToTexture */); final VideoCapturer videoCapturer = - enumerator.createCapturer(enumerator.getDeviceNames()[0], null); + enumerator.createCapturer(enumerator.getDeviceNames()[0], null /* eventsHandler */); final VideoSource videoSource = factory.createVideoSource(videoCapturer); videoCapturer.startCapture(640, 480, 30); @@ -1007,7 +1010,7 @@ public class PeerConnectionTest { // instead of in addTracksToPC. final CameraEnumerator enumerator = new Camera1Enumerator(false /* captureToTexture */); final VideoCapturer videoCapturer = - enumerator.createCapturer(enumerator.getDeviceNames()[0], null); + enumerator.createCapturer(enumerator.getDeviceNames()[0], null /* eventsHandler */); final VideoSource videoSource = factory.createVideoSource(videoCapturer); videoCapturer.startCapture(640, 480, 30); @@ -1163,6 +1166,133 @@ public class PeerConnectionTest { System.gc(); } + /** + * Test that a Java MediaStream is updated when the native stream is. + *

+ * Specifically, test that when remote tracks are indicated as being added or + * removed from a MediaStream (via "a=ssrc" or "a=msid" in a remote + * description), the existing remote MediaStream object is updated. + *

+ * This test starts with just an audio track, adds a video track, then + * removes it. It only applies remote offers, which is sufficient to test + * this functionality and simplifies the test. This means that no media will + * actually be sent/received; we're just testing that the Java MediaStream + * object gets updated when the native object changes. + */ + @Test + @MediumTest + public void testRemoteStreamUpdatedWhenTracksAddedOrRemoved() throws Exception { + PeerConnectionFactory.Options options = new PeerConnectionFactory.Options(); + PeerConnectionFactory factory = new PeerConnectionFactory(options); + + // This test is fine with default PC constraints and no ICE servers. + MediaConstraints pcConstraints = new MediaConstraints(); + LinkedList iceServers = new LinkedList(); + + // Use OfferToReceiveAudio/Video to ensure every offer has an audio and + // video m= section. Simplifies the test because it means we don't have to + // actually apply the offer to "offeringPC"; it's just used as an SDP + // factory. + MediaConstraints offerConstraints = new MediaConstraints(); + offerConstraints.mandatory.add( + new MediaConstraints.KeyValuePair("OfferToReceiveAudio", "true")); + offerConstraints.mandatory.add( + new MediaConstraints.KeyValuePair("OfferToReceiveVideo", "true")); + + // This PeerConnection will only be used to generate offers. + ObserverExpectations offeringExpectations = new ObserverExpectations("offerer"); + PeerConnection offeringPC = + factory.createPeerConnection(iceServers, pcConstraints, offeringExpectations); + assertNotNull(offeringPC); + + ObserverExpectations expectations = new ObserverExpectations("PC under test"); + PeerConnection pcUnderTest = + factory.createPeerConnection(iceServers, pcConstraints, expectations); + assertNotNull(pcUnderTest); + + // Add offerer media stream with just an audio track. + MediaStream localStream = factory.createLocalMediaStream("stream"); + AudioTrack localAudioTrack = + factory.createAudioTrack("audio", factory.createAudioSource(new MediaConstraints())); + localStream.addTrack(localAudioTrack); + // TODO(deadbeef): Use addTrack once that's available. + offeringExpectations.expectRenegotiationNeeded(); + offeringPC.addStream(localStream); + // Create offer. + SdpObserverLatch sdpLatch = new SdpObserverLatch(); + offeringPC.createOffer(sdpLatch, offerConstraints); + assertTrue(sdpLatch.await()); + SessionDescription offerSdp = sdpLatch.getSdp(); + + // Apply remote offer to PC under test. + sdpLatch = new SdpObserverLatch(); + expectations.expectSignalingChange(SignalingState.HAVE_REMOTE_OFFER); + expectations.expectAddStream("stream"); + pcUnderTest.setRemoteDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + // Sanity check that we get one remote stream with one audio track. + MediaStream remoteStream = expectations.gotRemoteStreams.iterator().next(); + assertEquals(remoteStream.audioTracks.size(), 1); + assertEquals(remoteStream.videoTracks.size(), 0); + + // Add a video track... + final CameraEnumerator enumerator = new Camera1Enumerator(false /* captureToTexture */); + final VideoCapturer videoCapturer = + enumerator.createCapturer(enumerator.getDeviceNames()[0], null /* eventsHandler */); + final VideoSource videoSource = factory.createVideoSource(videoCapturer); + VideoTrack videoTrack = factory.createVideoTrack("video", videoSource); + offeringExpectations.expectRenegotiationNeeded(); + localStream.addTrack(videoTrack); + // ... and create an updated offer. + sdpLatch = new SdpObserverLatch(); + offeringPC.createOffer(sdpLatch, offerConstraints); + assertTrue(sdpLatch.await()); + offerSdp = sdpLatch.getSdp(); + + // Apply remote offer with new video track to PC under test. + sdpLatch = new SdpObserverLatch(); + pcUnderTest.setRemoteDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + // The remote stream should now have a video track. + assertEquals(remoteStream.audioTracks.size(), 1); + assertEquals(remoteStream.videoTracks.size(), 1); + + // Finally, create another offer with the audio track removed. + offeringExpectations.expectRenegotiationNeeded(); + localStream.removeTrack(localAudioTrack); + localAudioTrack.dispose(); + sdpLatch = new SdpObserverLatch(); + offeringPC.createOffer(sdpLatch, offerConstraints); + assertTrue(sdpLatch.await()); + offerSdp = sdpLatch.getSdp(); + + // Apply remote offer with just a video track to PC under test. + sdpLatch = new SdpObserverLatch(); + pcUnderTest.setRemoteDescription(sdpLatch, offerSdp); + assertTrue(sdpLatch.await()); + // The remote stream should no longer have an audio track. + assertEquals(remoteStream.audioTracks.size(), 0); + assertEquals(remoteStream.videoTracks.size(), 1); + + // Free the Java-land objects. Video capturer and source aren't owned by + // the PeerConnection and need to be disposed separately. + // TODO(deadbeef): Should all these events really occur on disposal? + // "Gathering complete" is especially odd since gathering never started. + // Note that this test isn't meant to test these events, but we must do + // this or otherwise it will crash. + offeringExpectations.expectIceConnectionChange(IceConnectionState.CLOSED); + offeringExpectations.expectSignalingChange(SignalingState.CLOSED); + offeringExpectations.expectIceGatheringChange(IceGatheringState.COMPLETE); + offeringPC.dispose(); + expectations.expectIceConnectionChange(IceConnectionState.CLOSED); + expectations.expectSignalingChange(SignalingState.CLOSED); + expectations.expectIceGatheringChange(IceGatheringState.COMPLETE); + pcUnderTest.dispose(); + videoCapturer.dispose(); + videoSource.dispose(); + factory.dispose(); + } + private static void negotiate(PeerConnection offeringPC, ObserverExpectations offeringExpectations, PeerConnection answeringPC, ObserverExpectations answeringExpectations) { @@ -1229,7 +1359,7 @@ public class PeerConnectionTest { // Call getStats (old implementation) before shutting down PC. expectations.expectOldStatsCallback(); - assertTrue(pc.getStats(expectations, null)); + assertTrue(pc.getStats(expectations, null /* track */)); assertTrue(expectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); // Call the new getStats implementation as well. @@ -1245,7 +1375,7 @@ public class PeerConnectionTest { // Call getStats (old implementation) after calling close(). Should still // work. expectations.expectOldStatsCallback(); - assertTrue(pc.getStats(expectations, null)); + assertTrue(pc.getStats(expectations, null /* track */)); assertTrue(expectations.waitForAllExpectationsToBeSatisfied(TIMEOUT_SECONDS)); System.out.println("FYI stats: "); diff --git a/sdk/android/src/jni/classreferenceholder.cc b/sdk/android/src/jni/classreferenceholder.cc index 1319f4cc2b..3943cf227f 100644 --- a/sdk/android/src/jni/classreferenceholder.cc +++ b/sdk/android/src/jni/classreferenceholder.cc @@ -78,6 +78,7 @@ ClassReferenceHolder::ClassReferenceHolder(JNIEnv* jni) { LoadClass(jni, "org/webrtc/MediaCodecVideoEncoder$VideoCodecType"); LoadClass(jni, "org/webrtc/MediaSource$State"); LoadClass(jni, "org/webrtc/MediaStream"); + LoadClass(jni, "org/webrtc/MediaStreamTrack"); LoadClass(jni, "org/webrtc/MediaStreamTrack$MediaType"); LoadClass(jni, "org/webrtc/MediaStreamTrack$State"); LoadClass(jni, "org/webrtc/NetworkMonitor"); diff --git a/sdk/android/src/jni/jni_helpers.cc b/sdk/android/src/jni/jni_helpers.cc index 5628bfaa2d..4c3724f44c 100644 --- a/sdk/android/src/jni/jni_helpers.cc +++ b/sdk/android/src/jni/jni_helpers.cc @@ -379,6 +379,7 @@ Iterable::Iterator::Iterator(JNIEnv* jni, jobject iterable) : jni_(jni) { jclass iterator_class = GetObjectClass(jni, iterator_); has_next_id_ = GetMethodID(jni, iterator_class, "hasNext", "()Z"); next_id_ = GetMethodID(jni, iterator_class, "next", "()Ljava/lang/Object;"); + remove_id_ = GetMethodID(jni, iterator_class, "remove", "()V"); // Start at the first element in the collection. ++(*this); @@ -414,6 +415,11 @@ Iterable::Iterator& Iterable::Iterator::operator++() { return *this; } +void Iterable::Iterator::Remove() { + jni_->CallVoidMethod(iterator_, remove_id_); + CHECK_EXCEPTION(jni_) << "error during CallVoidMethod"; +} + // Provides a way to compare the iterator with itself and with the end iterator. // Note: all other comparison results are undefined, just like for C++ input // iterators. diff --git a/sdk/android/src/jni/jni_helpers.h b/sdk/android/src/jni/jni_helpers.h index 252cfd099a..8c02e3b6df 100644 --- a/sdk/android/src/jni/jni_helpers.h +++ b/sdk/android/src/jni/jni_helpers.h @@ -184,6 +184,10 @@ class Iterable { // Advances the iterator one step. Iterator& operator++(); + // Removes the element the iterator is pointing to. Must still advance the + // iterator afterwards. + void Remove(); + // Provides a way to compare the iterator with itself and with the end // iterator. // Note: all other comparison results are undefined, just like for C++ input @@ -200,6 +204,7 @@ class Iterable { jobject value_ = nullptr; jmethodID has_next_id_ = nullptr; jmethodID next_id_ = nullptr; + jmethodID remove_id_ = nullptr; rtc::ThreadChecker thread_checker_; RTC_DISALLOW_COPY_AND_ASSIGN(Iterator); diff --git a/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc b/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc index a2f979169e..4160f5acec 100644 --- a/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc +++ b/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc @@ -10,8 +10,10 @@ #include "sdk/android/src/jni/pc/peerconnectionobserver_jni.h" +#include #include +#include "rtc_base/ptr_util.h" #include "sdk/android/src/jni/classreferenceholder.h" #include "sdk/android/src/jni/pc/java_native_conversion.h" @@ -31,6 +33,13 @@ PeerConnectionObserverJni::PeerConnectionObserverJni(JNIEnv* jni, j_media_stream_class_(jni, FindClass(jni, "org/webrtc/MediaStream")), j_media_stream_ctor_( GetMethodID(jni, *j_media_stream_class_, "", "(J)V")), + j_media_stream_track_class_( + jni, + FindClass(jni, "org/webrtc/MediaStreamTrack")), + j_track_dispose_id_( + GetMethodID(jni, *j_media_stream_track_class_, "dispose", "()V")), + j_native_track_id_( + GetFieldID(jni, *j_media_stream_track_class_, "nativeTrack", "J")), j_audio_track_class_(jni, FindClass(jni, "org/webrtc/AudioTrack")), j_audio_track_ctor_( GetMethodID(jni, *j_audio_track_class_, "", "(J)V")), @@ -133,47 +142,132 @@ void PeerConnectionObserverJni::OnAddStream( jobject j_stream = GetOrCreateJavaStream(stream); for (const auto& track : stream->GetAudioTracks()) { - jstring id = JavaStringFromStdString(jni(), track->id()); - // Java AudioTrack holds one reference. Corresponding Release() is in - // MediaStreamTrack_free, triggered by AudioTrack.dispose(). - track->AddRef(); - jobject j_track = - jni()->NewObject(*j_audio_track_class_, j_audio_track_ctor_, - reinterpret_cast(track.get()), id); - CHECK_EXCEPTION(jni()) << "error during NewObject"; - jfieldID audio_tracks_id = GetFieldID( - jni(), *j_media_stream_class_, "audioTracks", "Ljava/util/LinkedList;"); - jobject audio_tracks = GetObjectField(jni(), j_stream, audio_tracks_id); - jmethodID add = GetMethodID(jni(), GetObjectClass(jni(), audio_tracks), - "add", "(Ljava/lang/Object;)Z"); - jboolean added = jni()->CallBooleanMethod(audio_tracks, add, j_track); - CHECK_EXCEPTION(jni()) << "error during CallBooleanMethod"; - RTC_CHECK(added); + AddNativeAudioTrackToJavaStream(track, j_stream); } - for (const auto& track : stream->GetVideoTracks()) { - jstring id = JavaStringFromStdString(jni(), track->id()); - // Java VideoTrack holds one reference. Corresponding Release() is in - // MediaStreamTrack_free, triggered by VideoTrack.dispose(). - track->AddRef(); - jobject j_track = - jni()->NewObject(*j_video_track_class_, j_video_track_ctor_, - reinterpret_cast(track.get()), id); - CHECK_EXCEPTION(jni()) << "error during NewObject"; - jfieldID video_tracks_id = GetFieldID( - jni(), *j_media_stream_class_, "videoTracks", "Ljava/util/LinkedList;"); - jobject video_tracks = GetObjectField(jni(), j_stream, video_tracks_id); - jmethodID add = GetMethodID(jni(), GetObjectClass(jni(), video_tracks), - "add", "(Ljava/lang/Object;)Z"); - jboolean added = jni()->CallBooleanMethod(video_tracks, add, j_track); - CHECK_EXCEPTION(jni()) << "error during CallBooleanMethod"; - RTC_CHECK(added); + AddNativeVideoTrackToJavaStream(track, j_stream); } jmethodID m = GetMethodID(jni(), *j_observer_class_, "onAddStream", "(Lorg/webrtc/MediaStream;)V"); jni()->CallVoidMethod(*j_observer_global_, m, j_stream); CHECK_EXCEPTION(jni()) << "error during CallVoidMethod"; + + // Create an observer to update the Java stream when the native stream's set + // of tracks changes. + auto observer = rtc::MakeUnique(stream); + observer->SignalAudioTrackRemoved.connect( + this, &PeerConnectionObserverJni::OnAudioTrackRemovedFromStream); + observer->SignalVideoTrackRemoved.connect( + this, &PeerConnectionObserverJni::OnVideoTrackRemovedFromStream); + observer->SignalAudioTrackAdded.connect( + this, &PeerConnectionObserverJni::OnAudioTrackAddedToStream); + observer->SignalVideoTrackAdded.connect( + this, &PeerConnectionObserverJni::OnVideoTrackAddedToStream); + stream_observers_.push_back(std::move(observer)); +} + +void PeerConnectionObserverJni::AddNativeAudioTrackToJavaStream( + rtc::scoped_refptr track, + jobject j_stream) { + jstring id = JavaStringFromStdString(jni(), track->id()); + // Java AudioTrack holds one reference. Corresponding Release() is in + // MediaStreamTrack_free, triggered by AudioTrack.dispose(). + track->AddRef(); + jobject j_track = jni()->NewObject(*j_audio_track_class_, j_audio_track_ctor_, + reinterpret_cast(track.get()), id); + CHECK_EXCEPTION(jni()) << "error during NewObject"; + + // Now add to the audioTracks linked list. + jfieldID audio_tracks_id = GetFieldID( + jni(), *j_media_stream_class_, "audioTracks", "Ljava/util/LinkedList;"); + jobject audio_tracks = GetObjectField(jni(), j_stream, audio_tracks_id); + jmethodID add = GetMethodID(jni(), GetObjectClass(jni(), audio_tracks), "add", + "(Ljava/lang/Object;)Z"); + jboolean added = jni()->CallBooleanMethod(audio_tracks, add, j_track); + CHECK_EXCEPTION(jni()) << "error during CallBooleanMethod"; + RTC_CHECK(added); +} + +void PeerConnectionObserverJni::AddNativeVideoTrackToJavaStream( + rtc::scoped_refptr track, + jobject j_stream) { + jstring id = JavaStringFromStdString(jni(), track->id()); + // Java VideoTrack holds one reference. Corresponding Release() is in + // MediaStreamTrack_free, triggered by VideoTrack.dispose(). + track->AddRef(); + jobject j_track = jni()->NewObject(*j_video_track_class_, j_video_track_ctor_, + reinterpret_cast(track.get()), id); + CHECK_EXCEPTION(jni()) << "error during NewObject"; + + // Now add to the videoTracks linked list. + jfieldID video_tracks_id = GetFieldID( + jni(), *j_media_stream_class_, "videoTracks", "Ljava/util/LinkedList;"); + jobject video_tracks = GetObjectField(jni(), j_stream, video_tracks_id); + jmethodID add = GetMethodID(jni(), GetObjectClass(jni(), video_tracks), "add", + "(Ljava/lang/Object;)Z"); + jboolean added = jni()->CallBooleanMethod(video_tracks, add, j_track); + CHECK_EXCEPTION(jni()) << "error during CallBooleanMethod"; + RTC_CHECK(added); +} + +void PeerConnectionObserverJni::RemoveAndDisposeNativeTrackFromJavaTrackList( + MediaStreamTrackInterface* track, + jobject j_tracks) { + Iterable iterable_tracks(jni(), j_tracks); + for (auto it = iterable_tracks.begin(); it != iterable_tracks.end(); ++it) { + MediaStreamTrackInterface* native_track = + reinterpret_cast( + jni()->GetLongField(*it, j_native_track_id_)); + CHECK_EXCEPTION(jni()) << "error during GetLongField"; + if (native_track == track) { + jni()->CallVoidMethod(*it, j_track_dispose_id_); + it.Remove(); + return; + } + } + // If we reached this point, we didn't find the track, which means we're + // getting a "track removed" callback but the Java stream doesn't have a + // corresponding track, which indicates a bug somewhere. + RTC_NOTREACHED(); +} + +void PeerConnectionObserverJni::OnAudioTrackAddedToStream( + AudioTrackInterface* track, + MediaStreamInterface* stream) { + ScopedLocalRefFrame local_ref_frame(jni()); + jobject j_stream = GetOrCreateJavaStream(stream); + AddNativeAudioTrackToJavaStream(track, j_stream); +} + +void PeerConnectionObserverJni::OnVideoTrackAddedToStream( + VideoTrackInterface* track, + MediaStreamInterface* stream) { + ScopedLocalRefFrame local_ref_frame(jni()); + jobject j_stream = GetOrCreateJavaStream(stream); + AddNativeVideoTrackToJavaStream(track, j_stream); +} + +void PeerConnectionObserverJni::OnAudioTrackRemovedFromStream( + AudioTrackInterface* track, + MediaStreamInterface* stream) { + ScopedLocalRefFrame local_ref_frame(jni()); + jobject j_stream = GetOrCreateJavaStream(stream); + jfieldID audio_tracks_id = GetFieldID( + jni(), *j_media_stream_class_, "audioTracks", "Ljava/util/LinkedList;"); + jobject audio_tracks = GetObjectField(jni(), j_stream, audio_tracks_id); + RemoveAndDisposeNativeTrackFromJavaTrackList(track, audio_tracks); +} + +void PeerConnectionObserverJni::OnVideoTrackRemovedFromStream( + VideoTrackInterface* track, + MediaStreamInterface* stream) { + ScopedLocalRefFrame local_ref_frame(jni()); + jobject j_stream = GetOrCreateJavaStream(stream); + jfieldID video_tracks_id = GetFieldID( + jni(), *j_media_stream_class_, "videoTracks", "Ljava/util/LinkedList;"); + jobject video_tracks = GetObjectField(jni(), j_stream, video_tracks_id); + RemoveAndDisposeNativeTrackFromJavaTrackList(track, video_tracks); } void PeerConnectionObserverJni::OnRemoveStream( @@ -187,6 +281,7 @@ void PeerConnectionObserverJni::OnRemoveStream( "(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; @@ -250,7 +345,18 @@ void PeerConnectionObserverJni::SetConstraints( void PeerConnectionObserverJni::DisposeRemoteStream( const NativeToJavaStreamsMap::iterator& it) { + MediaStreamInterface* stream = it->first; jobject j_stream = it->second; + + // Remove the observer first, so it doesn't react to events during deletion. + stream_observers_.erase( + std::remove_if( + stream_observers_.begin(), stream_observers_.end(), + [stream](const std::unique_ptr& observer) { + return observer->stream() == stream; + }), + stream_observers_.end()); + remote_streams_.erase(it); jni()->CallVoidMethod( j_stream, GetMethodID(jni(), *j_media_stream_class_, "dispose", "()V")); @@ -277,7 +383,6 @@ jobject PeerConnectionObserverJni::GetOrCreateJavaStream( if (it != remote_streams_.end()) { return it->second; } - // Java MediaStream holds one reference. Corresponding Release() is in // MediaStream_free, triggered by MediaStream.dispose(). stream->AddRef(); diff --git a/sdk/android/src/jni/pc/peerconnectionobserver_jni.h b/sdk/android/src/jni/pc/peerconnectionobserver_jni.h index eaf7f435cf..a48828b20a 100644 --- a/sdk/android/src/jni/pc/peerconnectionobserver_jni.h +++ b/sdk/android/src/jni/pc/peerconnectionobserver_jni.h @@ -11,6 +11,7 @@ #ifndef SDK_ANDROID_SRC_JNI_PC_PEERCONNECTIONOBSERVER_JNI_H_ #define SDK_ANDROID_SRC_JNI_PC_PEERCONNECTIONOBSERVER_JNI_H_ +#include #include #include #include @@ -25,7 +26,8 @@ namespace jni { // Adapter between the C++ PeerConnectionObserver interface and the Java // PeerConnection.Observer interface. Wraps an instance of the Java interface // and dispatches C++ callbacks to Java. -class PeerConnectionObserverJni : public PeerConnectionObserver { +class PeerConnectionObserverJni : public PeerConnectionObserver, + public sigslot::has_slots<> { public: PeerConnectionObserverJni(JNIEnv* jni, jobject j_observer); virtual ~PeerConnectionObserverJni(); @@ -56,6 +58,10 @@ class PeerConnectionObserverJni : public PeerConnectionObserver { private: typedef std::map NativeToJavaStreamsMap; typedef std::map NativeToJavaRtpReceiverMap; + typedef std::map + NativeToJavaMediaTrackMap; + typedef std::map + NativeMediaStreamTrackToNativeRtpReceiver; void DisposeRemoteStream(const NativeToJavaStreamsMap::iterator& it); void DisposeRtpReceiver(const NativeToJavaRtpReceiverMap::iterator& it); @@ -70,10 +76,44 @@ class PeerConnectionObserverJni : public PeerConnectionObserver { JNIEnv* jni, const std::vector>& streams); + // The three methods below must be called from within a local ref + // frame (e.g., using ScopedLocalRefFrame), otherwise they will + // leak references. + // + // Create a Java track object to wrap |track|, and add it to |j_stream|. + void AddNativeAudioTrackToJavaStream( + rtc::scoped_refptr track, + jobject j_stream); + void AddNativeVideoTrackToJavaStream( + rtc::scoped_refptr track, + jobject j_stream); + // Remove and dispose the Java MediaStreamTrack object that wraps |track|, + // given |j_tracks| which is a linked list of tracks (either the videoTracks + // or audioTracks member of MediaStream). + // + // DCHECKs if the track isn't found. + void RemoveAndDisposeNativeTrackFromJavaTrackList( + MediaStreamTrackInterface* track, + jobject j_tracks); + + // Callbacks invoked when a native stream changes, and the Java stream needs + // to be updated; MediaStreamObserver is used to make this simpler. + void OnAudioTrackAddedToStream(AudioTrackInterface* track, + MediaStreamInterface* stream); + void OnVideoTrackAddedToStream(VideoTrackInterface* track, + MediaStreamInterface* stream); + void OnAudioTrackRemovedFromStream(AudioTrackInterface* track, + MediaStreamInterface* stream); + void OnVideoTrackRemovedFromStream(VideoTrackInterface* track, + MediaStreamInterface* stream); + const ScopedGlobalRef j_observer_global_; const ScopedGlobalRef j_observer_class_; const ScopedGlobalRef j_media_stream_class_; const jmethodID j_media_stream_ctor_; + const ScopedGlobalRef j_media_stream_track_class_; + const jmethodID j_track_dispose_id_; + const jfieldID j_native_track_id_; const ScopedGlobalRef j_audio_track_class_; const jmethodID j_audio_track_ctor_; const ScopedGlobalRef j_video_track_class_; @@ -82,11 +122,13 @@ class PeerConnectionObserverJni : public PeerConnectionObserver { const jmethodID j_data_channel_ctor_; const ScopedGlobalRef j_rtp_receiver_class_; const jmethodID j_rtp_receiver_ctor_; + // C++ -> Java remote streams. The stored jobects are global refs and must be // manually deleted upon removal. Use DisposeRemoteStream(). NativeToJavaStreamsMap remote_streams_; NativeToJavaRtpReceiverMap rtp_receivers_; std::unique_ptr constraints_; + std::vector> stream_observers_; }; } // namespace jni