Android MediaCodec: catch exceptions to stop() and other methods. (issue 2461523003 by braveyao@chromium.org)

[brav...@chromium.org]

Reviewers: watk
CL: https://codereview.chromium.org/2461523003/

Message:
Hi watk@, please take a look.

Description:
Android MediaCodec: catch exceptions to stop() and other methods.

In MediaCodecBridge.java, we didn't catch exception to some system methods.
One of them caused the crash in crbug/659836 for unknown reason. Try to catch
exception to all the places calling system mathods in this file.

BUG=659836

Affected files (+22, -4 lines):
M media/base/android/java/src/org/chromium/media/MediaCodecBridge.java


Index: media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
diff --git a/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java b/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
index ecc90018c2082a81a8690e084c629be3028ee3b7..089581cea862eabc44ac432ae0901d5fb7be156b 100644
--- a/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
+++ b/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
@@ -296,13 +296,23 @@ class MediaCodecBridge {

@CalledByNative
private void stop() {
- mMediaCodec.stop();
+ try {
+ mMediaCodec.stop();
+ } catch (IllegalStateException e) {
+ Log.e(TAG, "Failed to stop MediaCodec", e);
+ }
}

@TargetApi(Build.VERSION_CODES.KITKAT)
@CalledByNative
private String getName() {
- return mMediaCodec.getName();
+ String codecName = "unknown";
+ try {
+ codecName = mMediaCodec.getName();
+ } catch (IllegalStateException e) {
+ Log.e(TAG, "Cannot get codec name", e);
+ }
+ return codecName;
}

@CalledByNative
@@ -370,7 +380,11 @@ class MediaCodecBridge {

Bundle b = new Bundle();
b.putInt(MediaCodec.PARAMETER_KEY_VIDEO_BITRATE, targetBps);
- mMediaCodec.setParameters(b);
+ try {
+ mMediaCodec.setParameters(b);
+ } catch (IllegalStateException e) {
+ Log.e(TAG, "Failed to set MediaCodec parameters", e);
+ }
Log.v(TAG,
"setVideoBitrate: input " + bps + "bps@" + frameRate + ", targetBps " + targetBps);
}
@@ -380,7 +394,11 @@ class MediaCodecBridge {
private void requestKeyFrameSoon() {
Bundle b = new Bundle();
b.putInt(MediaCodec.PARAMETER_KEY_REQUEST_SYNC_FRAME, 0);
- mMediaCodec.setParameters(b);
+ try {
+ mMediaCodec.setParameters(b);
+ } catch (IllegalStateException e) {
+ Log.e(TAG, "Failed to set MediaCodec parameters", e);
+ }
}

@CalledByNative

[wa...@chromium.org]

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
File
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode303
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:303:
}
Should we be returning MEDIA_CODEC_ERROR like for the other calls?

https://codereview.chromium.org/2461523003/

[brav...@chromium.org]

Thanks for comments! Please check my response. Correct me if I'm wrong.

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
File
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode303
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:303:
}

On 2016/10/28 21:49:56, watk wrote:
> Should we be returning MEDIA_CODEC_ERROR like for the other calls?

'stop()' will be only called in AndroidVideoEncodeAccelerator.Destroy()
at present, before it kills itself. So no one can really handle the
failure here, I suppose.
Also according to Android doc, this exception means mediaCodec is
already in release state. So it's OK we do nothing.

https://codereview.chromium.org/2461523003/

[wa...@chromium.org]

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
File
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode303
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:303:
}

On 2016/10/28 22:12:57, braveyao wrote:
> On 2016/10/28 21:49:56, watk wrote:
> > Should we be returning MEDIA_CODEC_ERROR like for the other calls?
>
> 'stop()' will be only called in
AndroidVideoEncodeAccelerator.Destroy() at
> present, before it kills itself. So no one can really handle the
failure here, I
> suppose.
> Also according to Android doc, this exception means mediaCodec is
already in
> release state. So it's OK we do nothing.

ok, sg for stop().

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode386
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:386:

Log.e(TAG, "Failed to set MediaCodec parameters", e);

I feel like this should return an error? If the codec is in an illegal
state the client should stop using it and release it right? We shouldn't
try to continue.

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode400
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:400:

Log.e(TAG, "Failed to set MediaCodec parameters", e);

I feel like this should return an error too.

https://codereview.chromium.org/2461523003/

[brav...@chromium.org]

Responded. PTAL.

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
File
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode386
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:386:
Log.e(TAG, "Failed to set MediaCodec parameters", e);

On 2016/10/28 22:17:57, watk wrote:
> I feel like this should return an error? If the codec is in an illegal
state the
> client should stop using it and release it right? We shouldn't try to
continue.

Yes these two are tricky. Personally I feel it's OK to keep it as is.
First, the caller doesn't need to care the result of these two methods.
Second, if there is IllegalStateException, then the queue/dequeue
processing will definitely encounter same exception immediately and they
will reset or end the call.
WDYT?

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode400
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:400:
Log.e(TAG, "Failed to set MediaCodec parameters", e);

On 2016/10/28 22:17:57, watk wrote:
> I feel like this should return an error too.

ditto

https://codereview.chromium.org/2461523003/

[wa...@chromium.org]

lgtm

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
File
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#newcode386
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:386:
Log.e(TAG, "Failed to set MediaCodec parameters", e);

On 2016/10/28 22:34:43, braveyao wrote:
> On 2016/10/28 22:17:57, watk wrote:
> > I feel like this should return an error? If the codec is in an
illegal state
> the
> > client should stop using it and release it right? We shouldn't try
to
> continue.
>
> Yes these two are tricky. Personally I feel it's OK to keep it as is.
> First, the caller doesn't need to care the result of these two
methods.
> Second, if there is IllegalStateException, then the queue/dequeue
processing
> will definitely encounter same exception immediately and they will
reset or end
> the call.
> WDYT?

Sure, I defer to your judgement on these ones. I'm not too familiar with
the encoding path.

https://codereview.chromium.org/2461523003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2461523003/1


https://codereview.chromium.org/2461523003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Committed patchset #1 (id:1)

https://codereview.chromium.org/2461523003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Patchset 1 (id:??) landed as
https://crrev.com/19c8aad333b0a687578e3163c10fc5d29e7496f2
Cr-Commit-Position: refs/heads/master@{#428533}

https://codereview.chromium.org/2461523003/