Call MediaCodec.stop() on separate thread.

Call MediaCodec.stop() on separate thread.

MediaCodec.stop() call may hang in some rear cases. To avoid
application hang this call need to be done on separate thread and
possible error reported back to application.
Application may elect to continue executing and use another codec
instance for encoding/decoding or stop the call and exit.

BUG=b/24339249
R=magjed@webrtc.org

Committed: https://crrev.com/5c3da4b6e9bdc787a3c1f25c0ff0c16a4271fd26
Cr-Commit-Position: refs/heads/master@{#10467}

[AlexG, 2015-10-29 23:52:43]

glaznev@webrtc.org changed reviewers:
+ magjed@webrtc.org

[AlexG, 2015-10-29 23:52:43]

PTAL

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

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:74: private
static MediaCodecVideoDecoderErrorCallback errorCallback = null;
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?

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:75: private
static int codecErrors = 0;
Why do we need to report the number of codec errors?

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:119: public
static void setErrorCallback(MediaCodecVideoDecoderErrorCallback errorCallback)
{
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.

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:295: final
AtomicBoolean releaseDone = new AtomicBoolean(false);
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.

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:314: new
Thread(runMediaCodecRelease).start();
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?

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:319:
releaseDone.wait(MEDIA_CODEC_RELEASE_TIMEOUT_MS);
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:59]

PTAL

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:74: private
static MediaCodecVideoDecoderErrorCallback errorCallback = null;
On 2015/10/30 14:01:57, magjed_webrtc wrote:
> 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?

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.

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:75: private
static int codecErrors = 0;
On 2015/10/30 14:01:57, magjed_webrtc wrote:
> Why do we need to report the number of codec errors?

See comment above

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:119: public
static void setErrorCallback(MediaCodecVideoDecoderErrorCallback errorCallback)
{
On 2015/10/30 14:01:57, magjed_webrtc wrote:
> 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.

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.

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:295: final
AtomicBoolean releaseDone = new AtomicBoolean(false);
On 2015/10/30 14:01:57, magjed_webrtc wrote:
> 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.

Done.

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:314: new
Thread(runMediaCodecRelease).start();
On 2015/10/30 14:01:57, magjed_webrtc wrote:
> 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?

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.

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:319:
releaseDone.wait(MEDIA_CODEC_RELEASE_TIMEOUT_MS);
On 2015/10/30 14:01:57, magjed_webrtc wrote:
> 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) {
>   ...
> }

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

[AlexG, 2015-10-30 20:32:43]

The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-30 20:32:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425143005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425143005/20001

[magjed_webrtc, 2015-10-30 21:50:38]

Alright, ship it. lgtm

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1425143005/diff/1/talk/app/webrtc/java/src/org/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:319:
releaseDone.wait(MEDIA_CODEC_RELEASE_TIMEOUT_MS);
On 2015/10/30 20:07:58, AlexG wrote:
> On 2015/10/30 14:01:57, magjed_webrtc wrote:
> > 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)
{
> >   ...
> > }
> 
> Done. Thanks! I moved your ThreadUtils changes to this patch. Once you'll
submit
> it I remove it from the this CL.

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

[AlexG, 2015-10-30 21:55:17]

The CQ bit was unchecked by glaznev@webrtc.org

[AlexG, 2015-10-30 21:55:24]

The CQ bit was checked by glaznev@webrtc.org

[commit-bot: I haz the power, 2015-10-30 21:55:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425143005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425143005/20001

[AlexG, 2015-10-30 22:31:30]

Committed patchset #2 (id:20001) manually as
5c3da4b6e9bdc787a3c1f25c0ff0c16a4271fd26 (presubmit successful).

[commit-bot: I haz the power, 2015-10-30 22:31:31]

Patchset 2 (id:??) landed as
https://crrev.com/5c3da4b6e9bdc787a3c1f25c0ff0c16a4271fd26
Cr-Commit-Position: refs/heads/master@{#10467}