Fix potential synchronization issues with framelisteners in EglRenderer.

Previously, a frame queued before calling addFrameListener could be
passed to the listener. Also fixes an issue where listener could still
be called after removeFrameListener call returned.

BUG=webrtc:6470

Review-Url: https://codereview.webrtc.org/2529313002
Cr-Commit-Position: refs/heads/master@{#15275}

webrtc/api/android/java/src/org/webrtc/EglRenderer.java

@@ -76,7 +76,6 @@ public class EglRenderer implements VideoRenderer.Callbacks {
   private final Object handlerLock = new Object();
   private Handler renderThreadHandler;
 
-  private final Object frameListenerLock = new Object();
   private final ArrayList<ScaleAndFrameListener> frameListeners = new ArrayList<>();
 
   // Variables for fps reduction.
@@ -364,27 +363,37 @@ public class EglRenderer implements VideoRenderer.Callbacks {
    * @param scale    The scale of the Bitmap passed to the callback, or 0 if no Bitmap is
    *                 required.
    */
-  public void addFrameListener(FrameListener listener, float scale) {
-    synchronized (frameListenerLock) {
-      frameListeners.add(new ScaleAndFrameListener(scale, listener));
-    }
+  public void addFrameListener(final FrameListener listener, final float scale) {
+    postToRenderThread(new Runnable() {
+      @Override
+      public void run() {
+        frameListeners.add(new ScaleAndFrameListener(scale, listener));
+      }
+    });
   }
 
   /**
    * Remove any pending callback that was added with addFrameListener. If the callback is not in
-   * the queue, nothing happens.
+   * the queue, nothing happens. It is ensured that callback won't be called after this method
+   * returns.
    *
    * @param runnable The callback to remove.
    */
-  public void removeFrameListener(FrameListener listener) {
-    synchronized (frameListenerLock) {
-      final Iterator<ScaleAndFrameListener> iter = frameListeners.iterator();
-      while (iter.hasNext()) {
-        if (iter.next().listener == listener) {
-          iter.remove();
+  public void removeFrameListener(final FrameListener listener) {
+    final CountDownLatch latch = new CountDownLatch(1);
+    postToRenderThread(new Runnable() {
+      @Override
+      public void run() {
+        latch.countDown();
+        final Iterator<ScaleAndFrameListener> iter = frameListeners.iterator();
+        while (iter.hasNext()) {
+          if (iter.next().listener == listener) {
+            iter.remove();
+          }
         }
       }
-    }
+    });
+    ThreadUtils.awaitUninterruptibly(latch);
   }
 
   // VideoRenderer.Callbacks interface.
@@ -581,12 +590,10 @@ public class EglRenderer implements VideoRenderer.Callbacks {
     // Make temporary copy of callback list to avoid ConcurrentModificationException, in case
     // callbacks call addFramelistener or removeFrameListener.
     final ArrayList<ScaleAndFrameListener> tmpList;
-    synchronized (frameListenerLock) {
-      if (frameListeners.isEmpty())
-        return;
-      tmpList = new ArrayList<>(frameListeners);
-      frameListeners.clear();
-    }
+    if (frameListeners.isEmpty())
+      return;
+    tmpList = new ArrayList<>(frameListeners);
+    frameListeners.clear();
 
     final float[] bitmapMatrix = RendererCommon.multiplyMatrices(
         RendererCommon.multiplyMatrices(texMatrix,

webrtc/api/androidtests/src/org/webrtc/EglRendererTest.java

@@ -29,6 +29,7 @@ public class EglRendererTest extends InstrumentationTestCase {
   final static int SURFACE_WAIT_MS = 1000;
   final static int TEST_FRAME_WIDTH = 4;
   final static int TEST_FRAME_HEIGHT = 4;
+  final static int REMOVE_FRAME_LISTENER_RACY_NUM_TESTS = 10;
   // Some arbitrary frames.
   final static ByteBuffer[][] TEST_FRAMES = {
       {
@@ -263,4 +264,29 @@ public class EglRendererTest extends InstrumentationTestCase {
       checkBitmap(testFrameListener.resetAndGetBitmap(), scale);
     }
   }
+
+  /**
+   * Checks that the frame listener will not be called with a frame that was delivered before the
+   * frame listener was added.
+   */
+  @SmallTest
+  public void testFrameListenerNotCalledWithOldFrames() throws Exception {
+    feedFrame(0);
+    eglRenderer.addFrameListener(testFrameListener, 0f);
+    // Check the old frame does not trigger frame listener.
+    assertFalse(testFrameListener.waitForBitmap(RENDER_WAIT_MS));
+  }
+
+  /** Checks that the frame listener will not be called after it is removed. */
+  @SmallTest
+  public void testRemoveFrameListenerNotRacy() throws Exception {
+    for (int i = 0; i < REMOVE_FRAME_LISTENER_RACY_NUM_TESTS; i++) {
+      feedFrame(0);
+      eglRenderer.addFrameListener(testFrameListener, 0f);
+      eglRenderer.removeFrameListener(testFrameListener);
+      feedFrame(1);
+    }
+    // Check the frame listener hasn't triggered.
+    assertFalse(testFrameListener.waitForBitmap(RENDER_WAIT_MS));
+  }
 }