Call MediaCodec.stop() on separate thread.

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

 /*
  * libjingle
  * Copyright 2014 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 25 matching lines @@
 import android.opengl.GLES11Ext;
 import android.opengl.GLES20;
 import android.os.Build;
 import android.view.Surface;
 
 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;
 
 // Java-side of peerconnection_jni.cc:MediaCodecVideoDecoder.
 // This class is an implementation detail of the Java PeerConnection API.
 // MediaCodec is thread-hostile so this class must be operated on a single
 // thread.
 public class MediaCodecVideoDecoder {
   // This class is constructed, operated, and destroyed by its C++ incarnation,
   // so the class and its methods have non-public visibility.  The API this
   // class exposes aims to mimic the webrtc::VideoDecoder API as closely as
   // possibly to minimize the amount of translation work necessary.
 
   private static final String TAG = "MediaCodecVideoDecoder";
 
   // Tracks webrtc::VideoCodecType.
   public enum VideoCodecType {
     VIDEO_CODEC_VP8,
     VIDEO_CODEC_VP9,
     VIDEO_CODEC_H264
   }
 
   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;
   private ByteBuffer[] outputBuffers;
   private static final String VP8_MIME_TYPE = "video/x-vnd.on2.vp8";
   private static final String H264_MIME_TYPE = "video/avc";
   // List of supported HW VP8 decoders.
   private static final String[] supportedVp8HwCodecPrefixes =
     {"OMX.qcom.", "OMX.Nvidia.", "OMX.Exynos.", "OMX.Intel." };
   // List of supported HW H.264 decoders.
@@ skipping 16 matching lines @@
   private int sliceHeight;
   private boolean useSurface;
   private int textureID = 0;
   private SurfaceTexture surfaceTexture = null;
   private Surface surface = null;
   private EglBase eglBase;
 
   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) {
       this.codecName = codecName;
       this.colorFormat = colorFormat;
     }
     public final String codecName; // OpenMax component name for VP8 codec.
     public final int colorFormat;  // Color format supported by codec.
   }
 
@@ skipping 148 matching lines @@
       return true;
     } catch (IllegalStateException e) {
       Logging.e(TAG, "initDecode failed", e);
       return false;
     }
   }
 
   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;
     if (useSurface) {
       surface.release();
       surface = null;
       Logging.d(TAG, "Delete video decoder TextureID " + textureID);
       GLES20.glDeleteTextures(1, new int[] {textureID}, 0);
       textureID = 0;
       eglBase.release();
@@ skipping 117 matching lines @@
   // MediaCodec.CodecException upon codec error.
   private void returnDecodedByteBuffer(int index)
       throws IllegalStateException, MediaCodec.CodecException {
     checkOnMediaCodecThread();
     if (useSurface) {
       throw new IllegalStateException("returnDecodedByteBuffer() called for surface decoding.");
     }
     mediaCodec.releaseOutputBuffer(index, false /* render */);
   }
 }