Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2084143002:
Make AVDA fall back to software decoding if needed. (Closed)
|
Created:
4 years, 6 months ago by liberato (no reviews please) Modified:
4 years, 6 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mlamouri+watch-media_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
- Release hardware codecs on a separate thread, since that can
hang too if mediaserver is in a bad state.
BUG=613238
Committed: https://crrev.com/6cc255e4585bfae626a5cf00a3e4b08a428cbeb5
Cr-Commit-Position: refs/heads/master@{#401691}
Patch Set 1 #
Total comments: 1
Patch Set 2 : symbolic constants, coments. #
Total comments: 12
Patch Set 3 : removed backup thread, defer release to construction thread #
Total comments: 11
Patch Set 4 : changed _l to auto_lock #
Total comments: 19
Patch Set 5 : cl feedback. #Patch Set 6 : cl feedback. #Messages
Total messages: 26 (7 generated)
Description was changed from
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
There is still a potential GPU hang on MediaCodec::release, but it
seems to be much less frequent.
BUG=613238
==========
to
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
There is still a potential GPU hang on MediaCodec::release, but it
seems to be much less frequent.
BUG=613238
==========
liberato@chromium.org changed reviewers: + dalecurtis@chromium.org, watk@chromium.org
doesn't fix every crash, but i can refresh on my test page many more times before it breaks (including after mediaserver hangs). sometimes still does hang on MediaCodecBridge::release, which can be a separate CL. https://codereview.chromium.org/2084143002/diff/1/media/base/android/java/src... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2084143002/diff/1/media/base/android/java/src... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:142: // TODO(liberato): Should we insist on software here? wouldn't mind some opinions on this. one one hand, 'false' doesn't run the risk of refusing to decode things that we filter out with the OMX.google check. On the other hand, this will actually create a decoder, and run the risk of a hang.
https://codereview.chromium.org/2084143002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2084143002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:86: * @param requireSoftware Whether we require a software codec. requireSoftwareCodec is probably a better name. https://codereview.chromium.org/2084143002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:99: if (requireSoftware && !info.getName().startsWith("OMX.google")) continue; Does this exist on Samsung phones? The samsung one is OMX.SEC per MediaCodecUtil. https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:196: // The thread might already be started even if there are no avda instances, remove , https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: scoped_refptr<base::SingleThreadTaskRunner> BackupTaskRunner() { Instead of this, should we just stop using async codec creation when it's hung? I.e. use the main thread and let gpu hang monitor solve our problems if it becomes an issue? We'd definitely want a UMA for hung thread status then. Similar to what is done in AudioManager::AudioHangMonitor which tracks okay, hung, and recovered states. https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:276: if (!backup_thread_.IsRunning()) Collapse to ternary if not deleting. https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:282: // Called on the main thread when codec autodetection starts. There may be I think this is a bit more complicated than necessary and TimeTicks can be dangerous in the world of suspend/resume (see AudioManager::AudioHangMonitor, dunno how that works on Android though). How about instead posting a delayed task with a timeout which completes codec initialization with an error? You can then just have a simple integer in AVDATimerManager which tracks timeout counts and once x is hit force software. If the real codec initialization ever comes back you decrement the timeout count. This avoids needing the additional locking mechanism and TimeTicks based comparison mechanism.
thanks for the comments. -fl https://codereview.chromium.org/2084143002/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2084143002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:86: * @param requireSoftware Whether we require a software codec. On 2016/06/21 21:17:56, DaleCurtis wrote: > requireSoftwareCodec is probably a better name. Done. https://codereview.chromium.org/2084143002/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:99: if (requireSoftware && !info.getName().startsWith("OMX.google")) continue; On 2016/06/21 21:17:56, DaleCurtis wrote: > Does this exist on Samsung phones? The samsung one is OMX.SEC per > MediaCodecUtil. according to this: http://androidxref.com/6.0.1_r10/xref/frameworks/av/media/libstagefright/OMXC... it should be "OMX.google" is sw, "OMX.anything else" is hardware, anything else is software. i'll add the "anything else" case. https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:196: // The thread might already be started even if there are no avda instances, On 2016/06/21 21:17:56, DaleCurtis wrote: > remove , i restructured the sentence a litt.e https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:272: scoped_refptr<base::SingleThreadTaskRunner> BackupTaskRunner() { On 2016/06/21 21:17:56, DaleCurtis wrote: > Instead of this, should we just stop using async codec creation when it's hung? > I.e. use the main thread and let gpu hang monitor solve our problems if it > becomes an issue? We'd definitely want a UMA for hung thread status then. > Similar to what is done in AudioManager::AudioHangMonitor which tracks okay, > hung, and recovered states. Done, except the UMA stat. i'm still trying to figure out exactly what we want to measure. https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:276: if (!backup_thread_.IsRunning()) On 2016/06/21 21:17:56, DaleCurtis wrote: > Collapse to ternary if not deleting. Done. https://codereview.chromium.org/2084143002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:282: // Called on the main thread when codec autodetection starts. There may be On 2016/06/21 21:17:56, DaleCurtis wrote: > I think this is a bit more complicated than necessary and TimeTicks can be > dangerous in the world of suspend/resume (see AudioManager::AudioHangMonitor, > dunno how that works on Android though). > > How about instead posting a delayed task with a timeout which completes codec > initialization with an error? You can then just have a simple integer in > AVDATimerManager which tracks timeout counts and once x is hit force software. > If the real codec initialization ever comes back you decrement the timeout > count. > > This avoids needing the additional locking mechanism and TimeTicks based > comparison mechanism. keeping codec construction off the main thread is generally a good thing to do. anyway, i removed the backup thread. as i thought more about this, i'm not sure that the PostDelayedTask really buys us much. the next CL just counts the total number of pending codec constructions, and switches to main thread + software if we're trying to get more than four. it's bad in that it switches to the main thread for construction more often than it needs to, but only if lots of video elements are on the page. PS2 has the advantage of keeping things off the main thread always, and allowing a single video element to fail over all future allocations to software. as in, a single page refresh can fail over rather than needing 4 / # video elements. i doubt that matters. the UMA stat gets more complicated, since it's no longer clear exactly when we've timed out. i'll figure something out.
Description was changed from
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
There is still a potential GPU hang on MediaCodec::release, but it
seems to be much less frequent.
BUG=613238
==========
to
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
- Release hardware codecs on a separate thread, since that can
hang too if mediaserver is in a bad state.
BUG=613238
==========
https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:275: base::AutoLock _l(autodetection_info_.lock_); Chose something other than _l :) https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:284: DCHECK(autodetection_info_.outstanding_ > 0); DCHECK_GT() https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:400: // Data for determining if codec creation is hanging. Doesn't seem like this is necessary anymore? Can't you tick the count on the main thread? and count it as pending until it returns?
https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:275: base::AutoLock _l(autodetection_info_.lock_); On 2016/06/22 19:13:49, DaleCurtis wrote: > Chose something other than _l :) done. that's a habit from android native code. https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:284: DCHECK(autodetection_info_.outstanding_ > 0); On 2016/06/22 19:13:49, DaleCurtis wrote: > DCHECK_GT() Done. https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:400: // Data for determining if codec creation is hanging. On 2016/06/22 19:13:49, DaleCurtis wrote: > Doesn't seem like this is necessary anymore? Can't you tick the count on the > main thread? and count it as pending until it returns? unfortunately, not safely. we'd lose the increment if the avda instance was destroyed.
https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:400: // Data for determining if codec creation is hanging. On 2016/06/22 at 20:44:12, liberato wrote: > On 2016/06/22 19:13:49, DaleCurtis wrote: > > Doesn't seem like this is necessary anymore? Can't you tick the count on the > > main thread? and count it as pending until it returns? > > unfortunately, not safely. we'd lose the increment if the avda instance was destroyed. What do you mean? Just have a bool pending_initialize_ and decrement if it hasn't completed right?
https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:400: // Data for determining if codec creation is hanging. On 2016/06/22 20:56:49, DaleCurtis wrote: > On 2016/06/22 at 20:44:12, liberato wrote: > > On 2016/06/22 19:13:49, DaleCurtis wrote: > > > Doesn't seem like this is necessary anymore? Can't you tick the count on the > > > main thread? and count it as pending until it returns? > > > > unfortunately, not safely. we'd lose the increment if the avda instance was > destroyed. > > What do you mean? Just have a bool pending_initialize_ and decrement if it > hasn't completed right? OnCodecInitialized is weakptr'd in the callback. is there some custom callback magic that can happen if the weak ptr doesn't promote, that we could take advantage of?
https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:400: // Data for determining if codec creation is hanging. On 2016/06/22 at 21:07:58, liberato wrote: > On 2016/06/22 20:56:49, DaleCurtis wrote: > > On 2016/06/22 at 20:44:12, liberato wrote: > > > On 2016/06/22 19:13:49, DaleCurtis wrote: > > > > Doesn't seem like this is necessary anymore? Can't you tick the count on the > > > > main thread? and count it as pending until it returns? > > > > > > unfortunately, not safely. we'd lose the increment if the avda instance was > > destroyed. > > > > What do you mean? Just have a bool pending_initialize_ and decrement if it > > hasn't completed right? > > OnCodecInitialized is weakptr'd in the callback. > > is there some custom callback magic that can happen if the weak ptr doesn't promote, that we could take advantage of? Sure, but if you get to destruction with pending_initialize_ = true you can decrement the outstanding count, right?
https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:400: // Data for determining if codec creation is hanging. On 2016/06/22 21:11:50, DaleCurtis wrote: > On 2016/06/22 at 21:07:58, liberato wrote: > > On 2016/06/22 20:56:49, DaleCurtis wrote: > > > On 2016/06/22 at 20:44:12, liberato wrote: > > > > On 2016/06/22 19:13:49, DaleCurtis wrote: > > > > > Doesn't seem like this is necessary anymore? Can't you tick the count on > the > > > > > main thread? and count it as pending until it returns? > > > > > > > > unfortunately, not safely. we'd lose the increment if the avda instance > was > > > destroyed. > > > > > > What do you mean? Just have a bool pending_initialize_ and decrement if it > > > hasn't completed right? > > > > OnCodecInitialized is weakptr'd in the callback. > > > > is there some custom callback magic that can happen if the weak ptr doesn't > promote, that we could take advantage of? > > Sure, but if you get to destruction with pending_initialize_ = true you can > decrement the outstanding count, right? wouldn't we get to destruction with pending_initialize_==true in the case where it's broken, too?
lgtm, i defer to you on whether you want to go back to the backup-thread. Maybe we should just have a thread pool with max of 2 threads? https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:400: // Data for determining if codec creation is hanging. On 2016/06/22 at 21:13:28, liberato wrote: > On 2016/06/22 21:11:50, DaleCurtis wrote: > > On 2016/06/22 at 21:07:58, liberato wrote: > > > On 2016/06/22 20:56:49, DaleCurtis wrote: > > > > On 2016/06/22 at 20:44:12, liberato wrote: > > > > > On 2016/06/22 19:13:49, DaleCurtis wrote: > > > > > > Doesn't seem like this is necessary anymore? Can't you tick the count on > > the > > > > > > main thread? and count it as pending until it returns? > > > > > > > > > > unfortunately, not safely. we'd lose the increment if the avda instance > > was > > > > destroyed. > > > > > > > > What do you mean? Just have a bool pending_initialize_ and decrement if it > > > > hasn't completed right? > > > > > > OnCodecInitialized is weakptr'd in the callback. > > > > > > is there some custom callback magic that can happen if the weak ptr doesn't > > promote, that we could take advantage of? > > > > Sure, but if you get to destruction with pending_initialize_ = true you can > > decrement the outstanding count, right? > > wouldn't we get to destruction with pending_initialize_==true in the case where it's broken, too? Ah, yeah, sorry I didn't think that through. https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... media/base/android/media_codec_util.cc:70: bool require_software) { require_software_codec here and elsewhere. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1426: manager->ConstructionTaskRunner()->PostTask( Can you just use DeleteSoon() ?
two threads: for now, let's just use the main thread as you suggest. i doubt that it makes things much different in practice, and we save the complexity of the extra thread. -fl https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... media/base/android/media_codec_util.cc:70: bool require_software) { On 2016/06/22 21:20:11, DaleCurtis wrote: > require_software_codec here and elsewhere. done, sorry. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1426: manager->ConstructionTaskRunner()->PostTask( On 2016/06/22 21:20:11, DaleCurtis wrote: > Can you just use DeleteSoon() ? ooh, shiny.
https://codereview.chromium.org/2084143002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:156: // TODO(liberato): Should we insist on software here? Hmm, we could prefer software, i.e., only create a HW one if there's no software one. I don't understand why we even need to create a codec to be honest. Seems checking if there's a codec name for the mime is enough, but I'm probably missing something. https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... media/base/android/media_codec_bridge.h:177: // Return true if and only if this is a software codec. nit: s/Return/Returns/ (BTW, I wasn't sure if this was in the style guide, so I just checked and it says: "These comments should be descriptive ("Opens the file") rather than imperative ("Open the file")) https://codereview.chromium.org/2084143002/diff/60001/media/base/android/ndk_... File media/base/android/ndk_media_codec_bridge.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/ndk_... media/base/android/ndk_media_codec_bridge.cc:253: return false; Comment would be nice to say why we either can't tell the truth or don't care. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:273: // several calls to this before any call to OnAnyThread. Can you clarify what OnAnyThread is? https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:288: // Return a hint about whether autodetecting the codec type is safe or not. I feel like we're too focused on autodetection as the key concept we're worried about. To me the key concept is: is the AVDA thread hung? If it is, we shouldn't use it any more, and we should use software codecs on the main thread because they probably don't hang. Anyway, my 2c. I'm fine with how it is if you choose to leave it. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1162: // If we successfully completed after an autodetect, then reset the timer so "reset the timer" feels like a misleading way to say what we're doing :P https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1402: AVDATimerManager* manager = g_avda_timer.Pointer(); Move variable closer to usage. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1428: base::Bind(delete_codec_helper, base::Passed(&media_codec_))); We might consider incrementing outstanding_ while a destruction is pending because it might hang too. Making outstanding_ the number of tasks on the AVDA thread. i.e., the number of tasks that may never complete. It's a shame TaskRunner doesn't have a way to query the number of queued tasks.
Nice CL btw! I'm happy we can work around this.
Nice CL btw! I'm happy we can work around this.
thanks for the comments. 'Configuration' and 'Autodetection' are the same lenght, so i just stuck with autodetection. -fl https://codereview.chromium.org/2084143002/diff/60001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:156: // TODO(liberato): Should we insist on software here? On 2016/06/22 22:39:46, watk wrote: > Hmm, we could prefer software, i.e., only create a HW one if there's no software > one. > > I don't understand why we even need to create a codec to be honest. Seems > checking if there's a codec name for the mime is enough, but I'm probably > missing something. i'm not sure either. i'll leave it for now, but i agree that there's probably a way to simplify it. https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/medi... media/base/android/media_codec_bridge.h:177: // Return true if and only if this is a software codec. On 2016/06/22 22:39:46, watk wrote: > nit: s/Return/Returns/ > > (BTW, I wasn't sure if this was in the style guide, so I just checked and it > says: "These comments should be descriptive ("Opens the file") rather than > imperative ("Open the file")) didn't know that, thanks! someday, i'm going to read that style guide.... https://codereview.chromium.org/2084143002/diff/60001/media/base/android/ndk_... File media/base/android/ndk_media_codec_bridge.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/base/android/ndk_... media/base/android/ndk_media_codec_bridge.cc:253: return false; On 2016/06/22 22:39:46, watk wrote: > Comment would be nice to say why we either can't tell the truth or don't care. Done. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:273: // several calls to this before any call to OnAnyThread. On 2016/06/22 22:39:46, watk wrote: > Can you clarify what OnAnyThread is? meant "OnAsync...Complete", thanks. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:288: // Return a hint about whether autodetecting the codec type is safe or not. On 2016/06/22 22:39:46, watk wrote: > I feel like we're too focused on autodetection as the key concept we're worried > about. > > To me the key concept is: is the AVDA thread hung? If it is, we shouldn't use it > any more, and we should use software codecs on the main thread because they > probably don't hang. > > Anyway, my 2c. I'm fine with how it is if you choose to leave it. yeah, i can see that. i chose 'autodetection' because it's the action that one must avoid, but you're right - one must also avoid posting onto the codec construction thread if it's hung up. hrm. i'll pick whichever name is shorter. :) https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1162: // If we successfully completed after an autodetect, then reset the timer so On 2016/06/22 22:39:46, watk wrote: > "reset the timer" feels like a misleading way to say what we're doing :P Done. https://codereview.chromium.org/2084143002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1402: AVDATimerManager* manager = g_avda_timer.Pointer(); On 2016/06/22 22:39:46, watk wrote: > Move variable closer to usage. Done.
lgtm
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2084143002/#ps100001 (title: "cl feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2084143002/100001
Message was sent while issue was closed.
Description was changed from
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
- Release hardware codecs on a separate thread, since that can
hang too if mediaserver is in a bad state.
BUG=613238
==========
to
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
- Release hardware codecs on a separate thread, since that can
hang too if mediaserver is in a bad state.
BUG=613238
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
- Release hardware codecs on a separate thread, since that can
hang too if mediaserver is in a bad state.
BUG=613238
==========
to
==========
Make AVDA fall back to software decoding if needed.
Android MediaCodec creation will sometimes never come back. On a
N7 (2013) with Lollipop, this can happens when an attempt to allocate
a hardware codec fails, and falls back to a software codec. Once
mediaserver gets into a bad state, future attempts to create a
hardware codec will hang. As a result, no additional AVDA instances
will get codecs. Further, destroying any of them will hang the gpu
process when it tries to join the thread that handles construction.
This CL does several things:
- Don't join the construction thread if there are outstanding
MediaCodec creations on it. While this may sometimes not
shut down the thread if the AVDA is destroyed while the
creation is pending (and would return if we just waited a bit
longer), the thread will be shut down the next time an AVDA
instance is destroyed. It isn't leaked permanently.
- Fall back to requesting a software codec if we believe that
construction of a hardware codec is hung. We don't attempt
to salvage the AVDA instance(s) that have pending HW codecs,
but future AVDA instances will fall back to software. If
those HW codecs are eventually allocated, then we will start
requesting HW codecs again. For example, if we simply time
out too quickly, or if mediaserver is restarted, then we can
try for hardware codecs for future AVDA instances.
- Release hardware codecs on a separate thread, since that can
hang too if mediaserver is in a bad state.
BUG=613238
Committed: https://crrev.com/6cc255e4585bfae626a5cf00a3e4b08a428cbeb5
Cr-Commit-Position: refs/heads/master@{#401691}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6cc255e4585bfae626a5cf00a3e4b08a428cbeb5 Cr-Commit-Position: refs/heads/master@{#401691} |
