Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2461523003:
Android MediaCodec: catch exceptions to stop() and other methods. (Closed)
|
Created:
4 years, 1 month ago by braveyao Modified:
4 years, 1 month ago Reviewers:
watk CC:
chromium-reviews, feature-media-reviews_chromium.org, avayvod+watch_chromium.org, mlamouri+watch-media_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid 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
Committed: https://crrev.com/19c8aad333b0a687578e3163c10fc5d29e7496f2
Cr-Commit-Position: refs/heads/master@{#428533}
Patch Set 1 #
Total comments: 8
Messages
Total messages: 16 (7 generated)
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@chromium.org changed reviewers: + watk@chromium.org
Hi watk@, please take a look.
https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:303: } Should we be returning MEDIA_CODEC_ERROR like for the other calls?
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... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... 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/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... 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... 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... 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.
Responded. PTAL. https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... 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... 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
lgtm https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/2461523003/diff/1/media/base/android/java/src... 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.
The CQ bit was checked by braveyao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/19c8aad333b0a687578e3163c10fc5d29e7496f2 Cr-Commit-Position: refs/heads/master@{#428533} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/19c8aad333b0a687578e3163c10fc5d29e7496f2 Cr-Commit-Position: refs/heads/master@{#428533} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
