From b7700d370d991b4740460fe93c09ade7acc3556c Mon Sep 17 00:00:00 2001 From: Magnus Jedvert Date: Wed, 13 Jun 2018 17:26:59 +0200 Subject: [PATCH] Android: Fix VideoTrack behavior for adding/removing VideoSinks In particular: * Trying to remove a VideoSink that was not attached should be a no-op. * Trying to remove a null VideoSink should be a no-op. * Adding the same VideoSink multiple times should a no-op, and should not create duplicate native VideoSinks. * Trying to add a null VideoSink should throw an exception in the Java layer instead of crashing in native code. This CL also adds tests that verify these behaviors. Bug: webrtc:9403 Change-Id: I928b7bb7f683634e287d7fec9e26f4179f73c150 Reviewed-on: https://webrtc-review.googlesource.com/83322 Commit-Queue: Magnus Jedvert Reviewed-by: Paulina Hensman Cr-Commit-Position: refs/heads/master@{#23602} --- sdk/android/api/org/webrtc/VideoTrack.java | 17 +++- .../src/org/webrtc/PeerConnectionTest.java | 96 +++++++++++++++++++ 2 files changed, 108 insertions(+), 5 deletions(-) diff --git a/sdk/android/api/org/webrtc/VideoTrack.java b/sdk/android/api/org/webrtc/VideoTrack.java index 214078b2ca0..a5a6eb0a43c 100644 --- a/sdk/android/api/org/webrtc/VideoTrack.java +++ b/sdk/android/api/org/webrtc/VideoTrack.java @@ -32,9 +32,16 @@ public VideoTrack(long nativeTrack) { * sources produce VideoFrames. */ public void addSink(VideoSink sink) { - final long nativeSink = nativeWrapSink(sink); - sinks.put(sink, nativeSink); - nativeAddSink(nativeTrack, nativeSink); + if (sink == null) { + throw new IllegalArgumentException("The VideoSink is not allowed to be null"); + } + // We allow calling addSink() with the same sink multiple times. This is similar to the C++ + // VideoTrack::AddOrUpdateSink(). + if (!sinks.containsKey(sink)) { + final long nativeSink = nativeWrapSink(sink); + sinks.put(sink, nativeSink); + nativeAddSink(nativeTrack, nativeSink); + } } /** @@ -43,8 +50,8 @@ public void addSink(VideoSink sink) { * If the VideoSink was not attached to the track, this is a no-op. */ public void removeSink(VideoSink sink) { - final long nativeSink = sinks.remove(sink); - if (nativeSink != 0) { + final Long nativeSink = sinks.remove(sink); + if (nativeSink != null) { nativeRemoveSink(nativeTrack, nativeSink); nativeFreeSink(nativeSink); } diff --git a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java index 7e3dee1d31d..5ef5599ac8a 100644 --- a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java +++ b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java @@ -15,6 +15,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import android.support.test.InstrumentationRegistry; import android.support.test.filters.MediumTest; @@ -1370,6 +1371,101 @@ public void testRemoteStreamUpdatedWhenTracksAddedOrRemoved() throws Exception { factory.dispose(); } + @Test + @MediumTest + public void testAddingNullVideoSink() { + final PeerConnectionFactory factory = + PeerConnectionFactory.builder().createPeerConnectionFactory(); + final VideoSource videoSource = factory.createVideoSource(/* isScreencast= */ false); + final VideoTrack videoTrack = factory.createVideoTrack("video", videoSource); + try { + videoTrack.addSink(/* sink= */ null); + fail("Should have thrown an IllegalArgumentException."); + } catch (IllegalArgumentException e) { + // Expected path. + } + } + + @Test + @MediumTest + public void testRemovingNullVideoSink() { + final PeerConnectionFactory factory = + PeerConnectionFactory.builder().createPeerConnectionFactory(); + final VideoSource videoSource = factory.createVideoSource(/* isScreencast= */ false); + final VideoTrack videoTrack = factory.createVideoTrack("video", videoSource); + videoTrack.removeSink(/* sink= */ null); + } + + @Test + @MediumTest + public void testRemovingNonExistantVideoSink() { + final PeerConnectionFactory factory = + PeerConnectionFactory.builder().createPeerConnectionFactory(); + final VideoSource videoSource = factory.createVideoSource(/* isScreencast= */ false); + final VideoTrack videoTrack = factory.createVideoTrack("video", videoSource); + final VideoSink videoSink = new VideoSink() { + @Override + public void onFrame(VideoFrame frame) {} + }; + videoTrack.removeSink(videoSink); + } + + @Test + @MediumTest + public void testAddingSameVideoSinkMultipleTimes() { + final PeerConnectionFactory factory = + PeerConnectionFactory.builder().createPeerConnectionFactory(); + final VideoSource videoSource = factory.createVideoSource(/* isScreencast= */ false); + final VideoTrack videoTrack = factory.createVideoTrack("video", videoSource); + + class FrameCounter implements VideoSink { + private int count = 0; + + public int getCount() { + return count; + } + + @Override + public void onFrame(VideoFrame frame) { + count += 1; + } + } + final FrameCounter frameCounter = new FrameCounter(); + + final VideoFrame videoFrame = new VideoFrame( + JavaI420Buffer.allocate(/* width= */ 32, /* height= */ 32), /* rotation= */ 0, + /* timestampNs= */ 0); + + videoTrack.addSink(frameCounter); + videoTrack.addSink(frameCounter); + videoSource.getCapturerObserver().onFrameCaptured(videoFrame); + + // Even though we called addSink() multiple times, we should only get one frame out. + assertEquals(1, frameCounter.count); + } + + @Test + @MediumTest + public void testAddingAndRemovingVideoSink() { + final PeerConnectionFactory factory = + PeerConnectionFactory.builder().createPeerConnectionFactory(); + final VideoSource videoSource = factory.createVideoSource(/* isScreencast= */ false); + final VideoTrack videoTrack = factory.createVideoTrack("video", videoSource); + final VideoFrame videoFrame = new VideoFrame( + JavaI420Buffer.allocate(/* width= */ 32, /* height= */ 32), /* rotation= */ 0, + /* timestampNs= */ 0); + + final VideoSink failSink = new VideoSink() { + @Override + public void onFrame(VideoFrame frame) { + fail("onFrame() should not be called on removed sink"); + } + }; + videoTrack.addSink(failSink); + videoTrack.removeSink(failSink); + videoSource.getCapturerObserver().onFrameCaptured(videoFrame); + } + private static void negotiate(PeerConnection offeringPC, ObserverExpectations offeringExpectations, PeerConnection answeringPC, ObserverExpectations answeringExpectations) {