Call MediaCodec.stop() on separate thread.

talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java

Index: talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java
diff --git a/talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java b/talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java
index 42af9c7fd00e8c618cfbc48fb27e49cd481f9b22..697f664ccd22dcae86fe00cb0e35c44794412e44 100644
--- a/talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java
+++ b/talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java
@@ -43,6 +43,7 @@ import org.webrtc.Logging;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import javax.microedition.khronos.egl.EGLContext;
 
@@ -66,9 +67,13 @@ public class MediaCodecVideoDecoder {
   }
 
   private static final int DEQUEUE_INPUT_TIMEOUT = 500000;  // 500 ms timeout.
+  private static final int MEDIA_CODEC_RELEASE_TIMEOUT_MS = 5000; // Timeout for codec releasing.
   // Active running decoder instance. Set in initDecode() (called from native code)
   // and reset to null in release() call.
   private static MediaCodecVideoDecoder runningInstance = null;
+  private static MediaCodecVideoDecoderErrorCallback errorCallback = null;

[magjed_webrtc, 2015/10/30 14:01:57]

We signal other MediaCodec errors with exceptions or special return values and
let the corresponding C++ class handle it. Wouldn't it be more consistent to do
this for MediaCodecVideoDecoder.release() as well?

[AlexG, 2015/10/30 20:07:59]

These are different errors. Exceptions and other media codec queue/dequeue 
timeouts are recoverable or caused by errors in video stream. They will not
affect next call - application may switch back to SW codec or stop the call and
start the next call. Hang in MediaCodec.release is different - we are getting
hanging codec instance which means that we can no longer use it in the next call
from the same app. We can use different media codec instance though, but we have
limited number of instances. This is a kind of critical error - if 2 or 3
instances are hanging app can no longer use HW codec - it need either to always
fall back to SW or force close itself with killing app process. So we need to
notify app how many codec critical errors happen so application can take
necessary actions. This is why codecErrors is also used - it reports how many
codec instances are not usable.

I change the event name to "onMediaCodecVideoDecoderCriticalError" to be
explicit that this error is critical. I also add more comments to the callback.

+  private static int codecErrors = 0;

[magjed_webrtc, 2015/10/30 14:01:57]

Why do we need to report the number of codec errors?

[AlexG, 2015/10/30 20:07:58]

See comment above

+
   private Thread mediaCodecThread;
   private MediaCodec mediaCodec;
   private ByteBuffer[] inputBuffers;
@@ -105,6 +110,17 @@ public class MediaCodecVideoDecoder {
   private MediaCodecVideoDecoder() {
   }
 
+  // MediaCodec error handler - invoked when critical error happens
+  // which prevents further use of media codec API.
+  public static interface MediaCodecVideoDecoderErrorCallback {
+    void onMediaCodecVideoDecoderError(int codecErrors);
+  }
+
+  public static void setErrorCallback(MediaCodecVideoDecoderErrorCallback errorCallback) {

[magjed_webrtc, 2015/10/30 14:01:57]

I really don't like hooking stuff up with static functions. What happens if you
have multiple decoders? And what about thread safety, this function will surely
be called from another thread than |mediaCodecThread|. If you want to use error
callbacks, I think it should be one per MediaCodec instance.

[AlexG, 2015/10/30 20:07:58]

There is similar to VideoCapturerAndroid.create() with camera events.error
callbacks. Currently there is no use case for multiple decode instances. Each
instance is created from C++ code, it can not be created from application and we
do not have WebRTC API yet to report decoder/encoder critical errors. So this
looks like a fast way to allow app to be notified on critical error. We can add
per-instance error handler later if there will be C++ API in place. App will
setup error handler at the time factory is created.

+    Logging.d(TAG, "Set error callback");
+    MediaCodecVideoDecoder.errorCallback = errorCallback;
+  }
+
   // Helper struct for findVp8Decoder() below.
   private static class DecoderProperties {
     public DecoderProperties(String codecName, int colorFormat) {
@@ -273,12 +289,48 @@ public class MediaCodecVideoDecoder {
   private void release() {
     Logging.d(TAG, "Java releaseDecoder");
     checkOnMediaCodecThread();
-    try {
-      mediaCodec.stop();
-      mediaCodec.release();
-    } catch (IllegalStateException e) {
-      Logging.e(TAG, "release failed", e);
+
+    // Run Mediacodec stop() and release() on separate thread since sometime
+    // Mediacodec.stop() may hang.
+    final AtomicBoolean releaseDone = new AtomicBoolean(false);

[magjed_webrtc, 2015/10/30 14:01:57]

I think it would be cleaner to write this with a CountDownLatch, i.e.
final CountDownLatch latch = new CountDownLatch(1);
Runnable runMediaCodecRelease = new Runnable() {
  ...
  mediaCodec.stop();
  mediaCodec.release();
  latch.countDown();
  ...
};
new Thread(runMediaCodecRelease).start();
if (!latch.await(MEDIA_CODEC_RELEASE_TIMEOUT_MS)) {
  codecErrors++;
  if (errorCallback != null) {
    Logging.e(TAG, "Invoke codec error callback. Errors: " + codecErrors);
    errorCallback.onMediaCodecVideoDecoderError(codecErrors);
  }
}
You also need to handle InterruptedException, see comment below.

[AlexG, 2015/10/30 20:07:59]

Done.

+
+    Runnable runMediaCodecRelease = new Runnable() {
+      @Override
+      public void run() {
+        try {
+          Logging.d(TAG, "Java releaseDecoder on release thread");
+          mediaCodec.stop();
+          mediaCodec.release();
+          Logging.d(TAG, "Java releaseDecoder on release thread done");
+        } catch (Exception e) {
+          Logging.e(TAG, "Media decoder release failed", e);
+        }
+        synchronized (releaseDone) {
+          releaseDone.set(true);
+          releaseDone.notifyAll();
+        }
+      }
+    };
+    new Thread(runMediaCodecRelease).start();

[magjed_webrtc, 2015/10/30 14:01:57]

We have this comment at the top of this file: "MediaCodec is thread-hostile so
this class must be operated on a single thread.". Do you think calling
MediaCodec.stop() on a separate thread can lead to other problems?

[AlexG, 2015/10/30 20:07:58]

I didn't see this requirement in Android docs - I guess this is a nice to have
feature, but may be not necessary. On few devices I have I don't see any
problems with this change, which of course does not guarantee it will not cause
problems on different devices :) But looks like only way to make async execution
of MEdiaCodec.stop is through different threads. Let's see how it will work - if
this call from another thread will fail on some devices - will revert.

+
+    if (!releaseDone.get()) {
+      synchronized(releaseDone) {
+        try {
+          releaseDone.wait(MEDIA_CODEC_RELEASE_TIMEOUT_MS);

[magjed_webrtc, 2015/10/30 14:01:57]

You should not swallow InterruptedException, i.e. catch it, log it, and do
nothing. That approach throws away important information about the fact that an
interrupt occurred, and can compromise the application's ability to cancel
activities or shut down in a timely manner.
http://www.ibm.com/developerworks/library/j-jtp05236/

I guess we don't want other threads to be able to cancel the MediaCodec release,
so you need to put this in a loop. If you use Object.wait() you need to put it
in a loop anyway, because of spurious wakeups:
"A thread can also wake up without being notified, interrupted, or timing out, a
so-called spurious wakeup. While this will rarely occur in practice,
applications must guard against it by testing for the condition that should have
caused the thread to be awakened, and continuing to wait if the condition is not
satisfied."
http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#wait(long)

If you use a CountDownLatch instead, I can add a helper function in ThreadUtils
to handle this. I would like to use such a helper function in other places as
well. I have uploaded a CL here:
https://codereview.webrtc.org/1414493006/
Then you can use:
if (!ThreadUtils.awaitUninterruptibly(latch, MEDIA_CODEC_RELEASE_TIMEOUT_MS) {
  ...
}

[AlexG, 2015/10/30 20:07:58]

Done. Thanks! I moved your ThreadUtils changes to this patch. Once you'll submit
it I remove it from the this CL.

[magjed_webrtc, 2015/10/30 21:50:37]

If you want, just submit the ThreadUtils change in this CL.

+        } catch (InterruptedException e) {
+          Logging.e(TAG,  "Wait exception.", e);
+        }
+      }
+    }
+    if (!releaseDone.get()) {
+      Logging.e(TAG, "Media decoder release timeout");
+      codecErrors++;
+      if (errorCallback != null) {
+        Logging.e(TAG, "Invoke codec error callback. Errors: " + codecErrors);
+        errorCallback.onMediaCodecVideoDecoderError(codecErrors);
+      }
     }
+
     mediaCodec = null;
     mediaCodecThread = null;
     runningInstance = null;