Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

From API level 21 many of MediaCodec's methods can throw CodecException
without warning. MediaCodecBridge now catches it (and its superclass,
IllegalStateException) and forwards it to callers as MEDIA_CODEC_ERROR.

There is probably opportunity to improve the MediaCodecBridge interface
such that fewer of its methods can fail, but this is simple for now and
fixes an existing crash.

BUG=585978
Committed: https://crrev.com/5906d7d07155203356125d21f34a349442cea5be
Cr-Commit-Position: refs/heads/master@{#379374}

[watk, 2016-03-03 22:08:51]

Patchset #1 (id:1) has been deleted

[watk, 2016-03-03 22:18:57]

watk@chromium.org changed reviewers:
+ qinmin@chromium.org, timav@chromium.org

[watk, 2016-03-03 22:18:58]

watk@chromium.org changed required reviewers:
+ timav@chromium.org

[watk, 2016-03-03 22:18:58]

PTAL. I'd like to merge this back to M50 to fix a crash we're seeing in AVDA.

timav@, please do a full review.
qinmin@, I want to make sure you're happy with the approach too, but no need to
do a full review.

[watk, 2016-03-03 22:21:37]

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

https://codereview.chromium.org/1764813002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:148:
private static class GetOutputFormatResult {
I added this so that we didn't have to have two wrapper classes for the status
and return value for both getOutputSize and getSamplingRate.

https://codereview.chromium.org/1764813002/diff/20001/media/filters/android/m...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/1764813002/diff/20001/media/filters/android/m...
media/filters/android/media_codec_audio_decoder.cc:516: CHECK_EQ(status,
MEDIA_CODEC_OK);
I'd be happy to fix these CHECKs now if there's a clear way to do so. I left
them as TODOs because it wasn't obvious how to handle them.

[watk, 2016-03-03 22:52:45]

watk@chromium.org changed required reviewers:
+ qinmin@chromium.org
- timav@chromium.org

[watk, 2016-03-03 22:52:45]

Swapping roles because Tima is OOO :)

qinmin@ please do a full review.

[qinmin, 2016-03-03 23:15:24]

https://codereview.chromium.org/1764813002/diff/20001/media/base/android/audi...
File media/base/android/audio_decoder_job.cc (right):

https://codereview.chromium.org/1764813002/diff/20001/media/base/android/audi...
media/base/android/audio_decoder_job.cc:203: if (status != MEDIA_CODEC_OK ||
output_sampling_rate_ != old_sampling_rate)
if the return status is not MEDIA_CODEC_OK, i think we should abort playback

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

https://codereview.chromium.org/1764813002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:323: if
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP
we should never throw the exception for all builds, just catch it and log the
error. Throw the exception will cause app to crash.

[watk, 2016-03-04 01:32:56]

Description was changed from

==========
Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

From API level 21 many of MediaCodec's methods can throw CodecException
without warning. MediaCodecBridge now catches it and forwards it to
callers as MEDIA_CODEC_ERROR. IllegalStateException is still thrown
in cases where it was previously, because it should only happen if
we're using the API incorrectly.

There is probably opportunity to improve the MediaCodecBridge interface
such that fewer of its methods can fail, but this is simple for now and
fixes an existing crash.

BUG=585978
==========

to

==========
Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

From API level 21 many of MediaCodec's methods can throw CodecException
without warning. MediaCodecBridge now catches it (and its superclass,
IllegalStateException) and forwards it to callers as MEDIA_CODEC_ERROR.

There is probably opportunity to improve the MediaCodecBridge interface
such that fewer of its methods can fail, but this is simple for now and
fixes an existing crash.

BUG=585978
==========

[watk, 2016-03-04 01:33:32]

https://codereview.chromium.org/1764813002/diff/20001/media/base/android/audi...
File media/base/android/audio_decoder_job.cc (right):

https://codereview.chromium.org/1764813002/diff/20001/media/base/android/audi...
media/base/android/audio_decoder_job.cc:203: if (status != MEDIA_CODEC_OK ||
output_sampling_rate_ != old_sampling_rate)
On 2016/03/03 23:15:23, qinmin wrote:
> if the return status is not MEDIA_CODEC_OK, i think we should abort playback

Makes sense. I'm not quite sure how to do that just yet, so I've turned this
into a CHECK and a TODO.

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

https://codereview.chromium.org/1764813002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:323: if
(Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP
On 2016/03/03 23:15:24, qinmin wrote:
> we should never throw the exception for all builds, just catch it and log the
> error. Throw the exception will cause app to crash.

Fair enough. It always catches IllegalStateException now.

[qinmin, 2016-03-04 05:04:13]

lgtm

[Tima Vaisburd, 2016-03-04 16:58:05]

lgtm

[watk, 2016-03-04 19:52:57]

watk@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[watk, 2016-03-04 19:52:57]

dalecurtis@chromium.org: Please review changes in content/

[DaleCurtis, 2016-03-04 20:33:34]

lgtm

https://codereview.chromium.org/1764813002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1764813002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_encode_accelerator.cc:339:
RETURN_ON_FAILURE(status != media::MEDIA_CODEC_ERROR,
Use == OK for readability.

[watk, 2016-03-04 20:37:18]

The CQ bit was checked by watk@chromium.org

[watk, 2016-03-04 20:37:18]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
qinmin@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1764813002/#ps80001
(title: "dale's comment")

[commit-bot: I haz the power, 2016-03-04 20:38:26]

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

[commit-bot: I haz the power, 2016-03-04 21:54:35]

Description was changed from

==========
Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

From API level 21 many of MediaCodec's methods can throw CodecException
without warning. MediaCodecBridge now catches it (and its superclass,
IllegalStateException) and forwards it to callers as MEDIA_CODEC_ERROR.

There is probably opportunity to improve the MediaCodecBridge interface
such that fewer of its methods can fail, but this is simple for now and
fixes an existing crash.

BUG=585978
==========

to

==========
Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

From API level 21 many of MediaCodec's methods can throw CodecException
without warning. MediaCodecBridge now catches it (and its superclass,
IllegalStateException) and forwards it to callers as MEDIA_CODEC_ERROR.

There is probably opportunity to improve the MediaCodecBridge interface
such that fewer of its methods can fail, but this is simple for now and
fixes an existing crash.

BUG=585978
==========

[commit-bot: I haz the power, 2016-03-04 21:54:36]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-04 21:55:22]

Description was changed from

==========
Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

From API level 21 many of MediaCodec's methods can throw CodecException
without warning. MediaCodecBridge now catches it (and its superclass,
IllegalStateException) and forwards it to callers as MEDIA_CODEC_ERROR.

There is probably opportunity to improve the MediaCodecBridge interface
such that fewer of its methods can fail, but this is simple for now and
fixes an existing crash.

BUG=585978
==========

to

==========
Catch CodecException in MediaCodecBridge and return MEDIA_CODEC_ERROR

From API level 21 many of MediaCodec's methods can throw CodecException
without warning. MediaCodecBridge now catches it (and its superclass,
IllegalStateException) and forwards it to callers as MEDIA_CODEC_ERROR.

There is probably opportunity to improve the MediaCodecBridge interface
such that fewer of its methods can fail, but this is simple for now and
fixes an existing crash.

BUG=585978

Committed: https://crrev.com/5906d7d07155203356125d21f34a349442cea5be
Cr-Commit-Position: refs/heads/master@{#379374}
==========

[commit-bot: I haz the power, 2016-03-04 21:55:24]

Patchset 4 (id:??) landed as
https://crrev.com/5906d7d07155203356125d21f34a349442cea5be
Cr-Commit-Position: refs/heads/master@{#379374}