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}

sdk/android/api/org/webrtc/VideoTrack.java

@@ -32,9 +32,16 @@ public class VideoTrack extends MediaStreamTrack {
    * 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 class VideoTrack extends MediaStreamTrack {
    * 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);
     }

sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java

@@ -15,6 +15,7 @@ import static org.junit.Assert.assertFalse;
 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 class PeerConnectionTest {
     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) {