Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629223003:
media: Ensure MediaCodecs are released before attached SurfaceTextures (Closed)
Descriptionmedia: Ensure MediaCodecs are released before attached SurfaceTextures
Previously AVDA would call SurfaceTexture#release() before calling
MediaCodec#release(), but we are seeing crashes that indicate that
MediaCodec doesn't handle this well. As a workaround, we now ensure
the MediaCodec is released first.
We don't always want to release both the surface and codec together. For
example, for a Reset() AVDA might release its codec without releasing
the surface. And then a Destroy() will want to release the surface
without the MediaCodec. To make sure they can't race, we record the
thread we last used to release a MediaCodec, so that when we release
the SurfaceTexture we can hop it through the same thread.
This change includes adding a new surface_texture member to
CodecConfig. When we create a ST the field is populated and when
tearing down AVDA it is released. UpdateSurface() ensures that either
the surface transition works, in which case we can drop the ST ref
and delete it safely, or if it fails the surface_texture field is left
unmodified.
BUG=679472
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2629223003
Cr-Commit-Position: refs/heads/master@{#444120}
Committed: https://chromium.googlesource.com/chromium/src/+/4eece651a995828d5677e6231fe7af235084dd88
Patch Set 1 #
Total comments: 6
Patch Set 2 : dale's suggestion #Patch Set 3 : more comments #
Total comments: 3
Messages
Total messages: 31 (14 generated)
Description was changed from ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now wait until the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record pending codec releases indexed by their attached SurfaceTextures, so when a SurfaceTexture is released we can wait for the codec to be released first if necessary. BUG=679472 TEST=TODO ========== to ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now wait until the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record pending codec releases indexed by their attached SurfaceTextures, so when a SurfaceTexture is released we can wait for the codec to be released first if necessary. BUG=679472 TEST=TODO CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
watk@chromium.org changed reviewers: + liberato@chromium.org
PTAL I hate this API.. but I don't want to devote effort to rethinking and refactoring it in AVDA, because MCVD won't use AVDAPictureBufferManager.
lgtm % nits. i'm going to have to revisit AVDASurfaceOwner, since i'm pretty sure it doesn't get these cases right. thanks -fl https://codereview.chromium.org/2629223003/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2629223003/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:879: picture_buffer_manager_.surface_texture().get()); this looks a lot like the surface owner api. it sends the owner in place of the surface_id, including for SurfaceTexture cases, if it wants to free the surface too. i think that i'm not handling these cases correctly in that CL, though. that's good -- we're converging :) https://codereview.chromium.org/2629223003/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:1149: picture_buffer_manager_.surface_texture().get()); it's a little weird that we ask for the surface texture after Destroy. not sure if it's a problem, but it should probably be well commented in picture_buffer_manager. else, somebody will clear it in destroy or something. probably me. :) https://codereview.chromium.org/2629223003/diff/1/media/gpu/avda_codec_alloca... File media/gpu/avda_codec_allocator.cc (right): https://codereview.chromium.org/2629223003/diff/1/media/gpu/avda_codec_alloca... media/gpu/avda_codec_allocator.cc:307: if (!surface_owners_.count(surface_id)) might want to use find rather than count + [] later to save the lookup. https://codereview.chromium.org/2629223003/diff/1/media/gpu/avda_codec_alloca... media/gpu/avda_codec_allocator.cc:325: if (surface_texture_codec_releases_.count(surface_texture.get())) { ditto find(). also might want to invert this to if(iter == end) { l#334, 335 return} just to save the indentation. https://codereview.chromium.org/2629223003/diff/1/media/gpu/avda_codec_alloca... File media/gpu/avda_codec_allocator.h (right): https://codereview.chromium.org/2629223003/diff/1/media/gpu/avda_codec_alloca... media/gpu/avda_codec_allocator.h:149: // after the codec. that it's a raw ptr is a little subtle. might deserve a comment that (a) it's not referenced, and (b) it's guaranteed not to outlive the surface texture as long as the client uses ReleaseSurfaceTexture rather than just dropping its reference. without (b) in particular, the raw ptr might become aliased to some future surface texture instance.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Can you elaborate on why it's insufficient to just always post the SurfaceTexture release during ~AVDA? We'll have already released the codec by then, it may just be pending on the codec task runner. By always trampolining through that thread we should guarantee that release has completed by the time the ST is actually released.
On 2017/01/13 20:37:14, DaleCurtis wrote: > Can you elaborate on why it's insufficient to just always post the > SurfaceTexture release during ~AVDA? We'll have already released the codec by > then, it may just be pending on the codec task runner. By always trampolining > through that thread we should guarantee that release has completed by the time > the ST is actually released. I hadn't thought about it but we can remove the ReleaseSurfaceTexture() in UpdateSurface(). In this CL it does trampoline through the allocator thread to get the ordering guarantee. If you're suggesting to remove the |surface_texture_codec_releases_| accounting in AVDACodecAllocator, the disadvantage is that we probably have to hop through both threads because we don't know which one is releasing the MediaCodec. The majority of the time it will be the same as the codec_config_->task_type_, but I'm worried about cases like: Reset(), ReleaseCodec() with HW thread, CreateNewCodec() picks SW thread because HW thread hung, Destroy(), ReleaseSurfaceTexture() posts to SW thread but codec is releasing on HW thread. We could always hop through both threads. We'll leak all ST's if the HW thread hangs. Maybe that's fine... I really don't like trying to think about what the correct behavior is when our thread hangs. We can already leak an arbitrary number of STs and codecs on the HW thread anyway with this approach, it should just be lower on average. We could hop through both threads, unless the HW thread is hung, in which case only hop through the SW thread. It can be potentially wrong in rare circumstances where the hang is a false positive.
On 2017/01/13 21:01:58, watk wrote: > On 2017/01/13 20:37:14, DaleCurtis wrote: > > Can you elaborate on why it's insufficient to just always post the > > SurfaceTexture release during ~AVDA? We'll have already released the codec by > > then, it may just be pending on the codec task runner. By always trampolining > > through that thread we should guarantee that release has completed by the time > > the ST is actually released. > > I hadn't thought about it but we can remove the ReleaseSurfaceTexture() in > UpdateSurface(). > > In this CL it does trampoline through the allocator thread to get the ordering > guarantee. > > If you're suggesting to remove the |surface_texture_codec_releases_| accounting > in AVDACodecAllocator, the disadvantage is that we probably have to hop through > both threads because we don't know which one is releasing the MediaCodec. The > majority of the time it will be the same as the codec_config_->task_type_, but > I'm worried about cases like: Reset(), ReleaseCodec() with HW thread, > CreateNewCodec() picks SW thread because HW thread hung, Destroy(), > ReleaseSurfaceTexture() posts to SW thread but codec is releasing on HW thread. > > We could always hop through both threads. We'll leak all ST's if the HW thread > hangs. Maybe that's fine... I really don't like trying to think about what the > correct behavior is when our thread hangs. We can already leak an arbitrary > number of STs and codecs on the HW thread anyway with this approach, it should > just be lower on average. > > We could hop through both threads, unless the HW thread is hung, in which case > only hop through the SW thread. It can be potentially wrong in rare > circumstances where the hang is a false positive. i don't think that we'll leak more than one with this approach, since we'll quit allocating codecs on the hw thread.
If you expose AVDACodecAllocator::TaskRunnerFor() you don't need to trampoline both. It'll just trampoline through whatever the last allocation was.
On 2017/01/13 at 21:05:34, DaleCurtis wrote: > If you expose AVDACodecAllocator::TaskRunnerFor() you don't need to trampoline both. It'll just trampoline through whatever the last allocation was. Also, +1 to Frank's comment, we'll quit allocating if things are hung.
On 2017/01/13 21:06:04, DaleCurtis wrote: > On 2017/01/13 at 21:05:34, DaleCurtis wrote: > > If you expose AVDACodecAllocator::TaskRunnerFor() you don't need to trampoline > both. It'll just trampoline through whatever the last allocation was. > > Also, +1 to Frank's comment, we'll quit allocating if things are hung. We can have an arbitrary number of codecs allocated when the thread hangs. And all those will leak. And unless I'm misunderstanding, it's not the last allocation that matters because AVDACodecAllocator is global and shared by AVDAs.
On 2017/01/13 21:11:06, watk wrote: > On 2017/01/13 21:06:04, DaleCurtis wrote: > > On 2017/01/13 at 21:05:34, DaleCurtis wrote: > > > If you expose AVDACodecAllocator::TaskRunnerFor() you don't need to > trampoline > > both. It'll just trampoline through whatever the last allocation was. > > > > Also, +1 to Frank's comment, we'll quit allocating if things are hung. > > We can have an arbitrary number of codecs allocated when the thread hangs. And > all those will leak. > > And unless I'm misunderstanding, it's not the last allocation that matters > because AVDACodecAllocator is global and shared by AVDAs. I guess you're suggesting add a TaskRunnerFor(codec), and AVDACodecAllocator remembers all codecs. That sounds good.
On 2017/01/13 22:27:15, watk wrote: > On 2017/01/13 21:11:06, watk wrote: > > On 2017/01/13 21:06:04, DaleCurtis wrote: > > > On 2017/01/13 at 21:05:34, DaleCurtis wrote: > > > > If you expose AVDACodecAllocator::TaskRunnerFor() you don't need to > > trampoline > > > both. It'll just trampoline through whatever the last allocation was. > > > > > > Also, +1 to Frank's comment, we'll quit allocating if things are hung. > > > > We can have an arbitrary number of codecs allocated when the thread hangs. And > > all those will leak. > > > > And unless I'm misunderstanding, it's not the last allocation that matters > > because AVDACodecAllocator is global and shared by AVDAs. > > I guess you're suggesting add a TaskRunnerFor(codec), and AVDACodecAllocator > remembers all codecs. That sounds good. Wait, what am I talking about? We might not have a codec when we want to release the surface.
Ok, talked to Dale offline and I get the suggestion now. We save the task_type of the last codec release in AVDA. And we just post the ST release with the same task type. Seems much simpler and correct, so thanks for the suggestion. Will do.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now wait until the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record pending codec releases indexed by their attached SurfaceTextures, so when a SurfaceTexture is released we can wait for the codec to be released first if necessary. BUG=679472 TEST=TODO CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now ensure the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record the thread we last used to release a MediaCodec, so that when we release the SurfaceTexture we can hop it through the same thread. BUG=679472 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now ensure the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record the thread we last used to release a MediaCodec, so that when we release the SurfaceTexture we can hop it through the same thread. BUG=679472 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now ensure the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record the thread we last used to release a MediaCodec, so that when we release the SurfaceTexture we can hop it through the same thread. This change includes adding a new surface_texture member to CodecConfig. When we create a ST the field is populated and when tearing down AVDA it is released. UpdateSurface() ensures that either the surface transition works, in which case we can drop the ST ref and delete it safely, or if it fails the surface_texture field is left unmodified. BUG=679472 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
alrighty! PTAL https://codereview.chromium.org/2629223003/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2629223003/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:1149: picture_buffer_manager_.surface_texture().get()); On 2017/01/13 17:22:04, liberato wrote: > it's a little weird that we ask for the surface texture after Destroy. not sure > if it's a problem, but it should probably be well commented in > picture_buffer_manager. else, somebody will clear it in destroy or something. > probably me. :) Agreed. I fixed this in the latest PS. Now Destroy() does clear it :)
lgtm, thanks! we could limit to KK if you want it looks like, but I'm fine with doing it everywhere for coverage since it seems harmless.
The CQ bit was checked by watk@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.
lgtm % question. https://codereview.chromium.org/2629223003/diff/80001/media/gpu/avda_codec_al... File media/gpu/avda_codec_allocator.h (right): https://codereview.chromium.org/2629223003/diff/80001/media/gpu/avda_codec_al... media/gpu/avda_codec_allocator.h:59: scoped_refptr<gl::SurfaceTexture> surface_texture; is this used?
Thanks! https://codereview.chromium.org/2629223003/diff/80001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2629223003/diff/80001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1154: if (codec_config_->surface_texture) { This is where surface_texture is used. We store a strong ref to it in the codec config so that it lives independently of the picture buffer manager, the codec, and the glimages. https://codereview.chromium.org/2629223003/diff/80001/media/gpu/avda_codec_al... File media/gpu/avda_codec_allocator.h (right): https://codereview.chromium.org/2629223003/diff/80001/media/gpu/avda_codec_al... media/gpu/avda_codec_allocator.h:59: scoped_refptr<gl::SurfaceTexture> surface_texture; On 2017/01/17 18:58:24, liberato wrote: > is this used? Yes, I put a comment on its usage. The point of it is to keep a strong ref to the ST that is independent of anything else using it, so we don't have to worry about ~ST doing the release before we release the codec, and we don't have to give PBM a weird guarantee about how long it holds onto the ST.
The CQ bit was checked by watk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484679774180550,
"parent_rev": "1107a295dcee2f4ee3f6c24f836c6adfa7b3830d", "commit_rev":
"4eece651a995828d5677e6231fe7af235084dd88"}
Message was sent while issue was closed.
Description was changed from ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now ensure the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record the thread we last used to release a MediaCodec, so that when we release the SurfaceTexture we can hop it through the same thread. This change includes adding a new surface_texture member to CodecConfig. When we create a ST the field is populated and when tearing down AVDA it is released. UpdateSurface() ensures that either the surface transition works, in which case we can drop the ST ref and delete it safely, or if it fails the surface_texture field is left unmodified. BUG=679472 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media: Ensure MediaCodecs are released before attached SurfaceTextures Previously AVDA would call SurfaceTexture#release() before calling MediaCodec#release(), but we are seeing crashes that indicate that MediaCodec doesn't handle this well. As a workaround, we now ensure the MediaCodec is released first. We don't always want to release both the surface and codec together. For example, for a Reset() AVDA might release its codec without releasing the surface. And then a Destroy() will want to release the surface without the MediaCodec. To make sure they can't race, we record the thread we last used to release a MediaCodec, so that when we release the SurfaceTexture we can hop it through the same thread. This change includes adding a new surface_texture member to CodecConfig. When we create a ST the field is populated and when tearing down AVDA it is released. UpdateSurface() ensures that either the surface transition works, in which case we can drop the ST ref and delete it safely, or if it fails the surface_texture field is left unmodified. BUG=679472 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2629223003 Cr-Commit-Position: refs/heads/master@{#444120} Committed: https://chromium.googlesource.com/chromium/src/+/4eece651a995828d5677e6231fe7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4eece651a995828d5677e6231fe7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
