VideoCapturerAndroid: Fix threading issues

talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java

 /*
  * libjingle
  * Copyright 2015 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 11 matching lines @@
  * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
  * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
  * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 package org.webrtc;
 
 import android.hardware.Camera;
 import android.test.ActivityTestCase;
 import android.test.suitebuilder.annotation.SmallTest;
+import android.test.suitebuilder.annotation.MediumTest;
 import android.util.Size;
 
 import org.webrtc.CameraEnumerationAndroid.CaptureFormat;
 import org.webrtc.VideoRenderer.I420Frame;
 
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.CountDownLatch;
 
 @SuppressWarnings("deprecation")
 public class VideoCapturerAndroidTest extends ActivityTestCase {
   static class RendererCallbacks implements VideoRenderer.Callbacks {
     private int framesRendered = 0;
     private Object frameLock = 0;
 
     @Override
     public void renderFrame(I420Frame frame) {
       synchronized (frameLock) {
         ++framesRendered;
         frameLock.notify();
       }
       VideoRenderer.renderFrameDone(frame);
     }
 
     public int WaitForNextFrameToRender() throws InterruptedException {
       synchronized (frameLock) {
         frameLock.wait();
         return framesRendered;
       }
     }
   }
 
-  static class AsyncRenderer implements VideoRenderer.Callbacks {
+  static class FakeAsyncRenderer implements VideoRenderer.Callbacks {
     private final List<I420Frame> pendingFrames = new ArrayList<I420Frame>();
 
     @Override
     public void renderFrame(I420Frame frame) {
       synchronized (pendingFrames) {
         pendingFrames.add(frame);
         pendingFrames.notifyAll();
       }
     }
 
     // Wait until at least one frame have been received, before returning them.
-    public List<I420Frame> WaitForFrames() {
+    public List<I420Frame> WaitForFrames() throws InterruptedException {

[hbos, 2015/09/21 12:59:06]

nit: waitForPendingFrames()

[magjed_webrtc, 2015/09/21 15:27:08]

Done.

       synchronized (pendingFrames) {
         while (pendingFrames.isEmpty()) {
-          try {
-            pendingFrames.wait();
-          } catch (InterruptedException e) {
-            // Ignore.
-          }
+          pendingFrames.wait();
         }
         final List<I420Frame> frames = new ArrayList<I420Frame>(pendingFrames);
         pendingFrames.clear();
         return frames;
       }
     }
   }
 
   static class FakeCapturerObserver implements
       VideoCapturerAndroid.CapturerObserver {
@@ skipping 148 matching lines @@
   // to a Java video renderer using the back facing video capturer.
   // It tests both the Java and the C++ layer.
   public void testStartBackFacingVideoCapturer() throws Exception {
     if (!HaveTwoCameras()) {
       return;
     }
     startCapturerAndRender(CameraEnumerationAndroid.getNameOfBackFacingDevice());
   }
 
   @SmallTest
-  // This test that the default camera can be started and but the camera can
+  // This test that the default camera can be started and that the camera can
   // later be switched to another camera.
   // It tests both the Java and the C++ layer.
   public void testSwitchVideoCapturer() throws Exception {
     PeerConnectionFactory factory = new PeerConnectionFactory();
     VideoCapturerAndroid capturer = VideoCapturerAndroid.create("", null);
     VideoSource source =
         factory.createVideoSource(capturer, new MediaConstraints());
     VideoTrack track = factory.createVideoTrack("dummy", source);
 
-    if (HaveTwoCameras())
-      assertTrue(capturer.switchCamera(null));
-    else
-      assertFalse(capturer.switchCamera(null));
+    // Array with one element to avoid final problem in nested classes.
+    final boolean[] cameraSwitchSuccessful = new boolean[1];
+    final CountDownLatch barrier = new CountDownLatch(1);
+    capturer.switchCamera(new VideoCapturerAndroid.CameraSwitchHandler() {
+      @Override
+      public void onCameraSwitchDone(boolean isFrontCamera) {
+        cameraSwitchSuccessful[0] = true;
+        barrier.countDown();
+      }
+      @Override
+      public void onCameraSwitchError(String errorDescription) {
+        cameraSwitchSuccessful[0] = false;
+        barrier.countDown();
+      }
+    });
+    // Wait until the camera has been switched.
+    barrier.await();
 
-    // Wait until the camera have been switched.
-    capturer.runCameraThreadUntilIdle();
-
+    // Check result.
+    if (HaveTwoCameras()) {
+      assertTrue(cameraSwitchSuccessful[0]);
+    } else {
+      assertFalse(cameraSwitchSuccessful[0]);
+    }
     // Ensure that frames are received.
     RendererCallbacks callbacks = new RendererCallbacks();
     track.addRenderer(new VideoRenderer(callbacks));
     assertTrue(callbacks.WaitForNextFrameToRender() > 0);
     track.dispose();
     source.dispose();
     factory.dispose();
   }
 
   @SmallTest
@@ skipping 79 matching lines @@
     observer.WaitForNextCapturedFrame();
     capturer.stopCapture();
 
     listOftimestamps = observer.getCopyAndResetListOftimeStamps();
     assertTrue(listOftimestamps.size() >= 2);
     for (Long timeStamp : listOftimestamps) {
       capturer.returnBuffer(timeStamp);
     }
   }
 
-  @SmallTest
-  // This test that we can capture frames, stop capturing, keep the frames for rendering, and then
-  // return the frames. It tests both the Java and the C++ layer.
-  public void testCaptureAndAsyncRender() {
+  @MediumTest
+  // This test that we can capture frames, keep the frames in a local renderer, stop capturing,
+  // and then return the frames. The difference between the test testReturnBufferLate() is that we
+  // also test the JNI and C++ AndroidVideoCapturer parts.
+  public void testReturnBufferLateEndToEnd() throws InterruptedException {
     final VideoCapturerAndroid capturer = VideoCapturerAndroid.create("", null);
-    // Helper class that sets everything up, captures at least one frame, and then shuts
-    // everything down.
-    class CaptureFramesRunnable implements Runnable {
-      public List<I420Frame> frames;
+    final PeerConnectionFactory factory = new PeerConnectionFactory();
+    final VideoSource source = factory.createVideoSource(capturer, new MediaConstraints());
+    final VideoTrack track = factory.createVideoTrack("dummy", source);
+    final FakeAsyncRenderer renderer = new FakeAsyncRenderer();
+    track.addRenderer(new VideoRenderer(renderer));
+    // Wait for at least one frame.
+    final List<I420Frame> pendingFrames = renderer.WaitForFrames();
+    // Check that we have frames that have not been returned.
+    assertFalse(pendingFrames.isEmpty());
 
+    capturer.stopCapture();

[hbos, 2015/09/21 12:59:06]

waitForFrames() should happen after stopCapture() or else there is a unlikely
race in which case the fake renderer could receive more frames after
pendingFrames that are never returned.

[magjed_webrtc, 2015/09/21 15:27:08]

Thanks. Done.

+
+    // Return the frame(s), on a different thread out of spite.
+    final Thread returnThread = new Thread(new Runnable() {
       @Override
       public void run() {
-        PeerConnectionFactory factory = new PeerConnectionFactory();
-        VideoSource source = factory.createVideoSource(capturer, new MediaConstraints());
-        VideoTrack track = factory.createVideoTrack("dummy", source);
-        AsyncRenderer renderer = new AsyncRenderer();
-        track.addRenderer(new VideoRenderer(renderer));
+        for (I420Frame frame : pendingFrames) {
+          VideoRenderer.renderFrameDone(frame);
+        }
+      }
+    });
+    returnThread.start();
+    returnThread.join();
 
-        // Wait until we get at least one frame.
-        frames = renderer.WaitForFrames();
+    // Check that frames have successfully returned.
+    assertEquals(capturer.pendingFramesTimeStamps(), "[]");

[hbos, 2015/09/21 12:59:06]

nit: Checking pending frames count == 0 would be nicer.

[magjed_webrtc, 2015/09/21 15:27:08]

Done.

 
-        // Stop everything.
-        track.dispose();
-        source.dispose();
-        factory.dispose();
-      }
-    }
-
-    // Capture frames on a separate thread.
-    CaptureFramesRunnable captureFramesRunnable = new CaptureFramesRunnable();
-    Thread captureThread = new Thread(captureFramesRunnable);
-    captureThread.start();
-
-    // Wait until frames are captured, and then kill the thread.
-    try {
-      captureThread.join();
-    } catch (InterruptedException e) {
-      fail("Capture thread was interrupted");
-    }
-    captureThread = null;
-
-    // Assert that we have frames that have not been returned.
-    assertTrue(!captureFramesRunnable.frames.isEmpty());
-    // Return the frame(s).
-    for (I420Frame frame : captureFramesRunnable.frames) {
-      VideoRenderer.renderFrameDone(frame);
-    }
-    assertEquals(capturer.pendingFramesTimeStamps(), "[]");
+    // Stop everything.
+    track.dispose();
+    source.dispose();
+    factory.dispose();

[hbos, 2015/09/21 12:59:06]

It is not clear that the C++ resources should be freed after some of these have
been disposed. And the unittest does not check that this happens. It is up to
you if you want to address this or not. Shouldn't the capturer have a dispose()?
That would be a different CL and discussion though.

[magjed_webrtc, 2015/09/21 15:27:08]

The capturer is owned by |source| I think, so it does not have a separate
dispose(). I will not add a test for VideoCapturerAndroid.release() in this CL.

[hbos, 2015/09/22 09:12:46]

How about a simple has been released flag as discussed offline?

[magjed_webrtc, 2015/09/22 15:04:21]

Done.

   }
 }