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 <magjed@webrtc.org>
Reviewed-by: Paulina Hensman <phensman@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23602}diff --git a/sdk/android/api/org/webrtc/VideoTrack.java b/sdk/android/api/org/webrtc/VideoTrack.java
index 214078b..a5a6eb0 100644
--- a/sdk/android/api/org/webrtc/VideoTrack.java
+++ b/sdk/android/api/org/webrtc/VideoTrack.java
@@ -32,9 +32,16 @@
* 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 @@
* 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 7e3dee1..5ef5599 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 @@
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) {