Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2508053002:
media: Do a TimedWait() for video surface teardown in AVDA (Closed)
|
Created:
4 years, 1 month ago by watk Modified:
4 years, 1 month ago CC:
DaleCurtis, apacible+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Do a TimedWait() for video surface teardown in AVDA
In OnSurfaceDestroyed() we have to release the MediaCodec synchronously
to avoid crashing or hanging. Previously we did the release
asynchronously because we post MediaCodec release to another thread.
Now that SurfaceDestroyed messages are enabled on all Android API
levels, and we don't have deferred error reporting this change
makes the teardown more robust by waiting for MediaCodecs to be
released before returning from OnSurfaceDestroyed().
* AVDACodecAllocator now tracks the allocation of surfaces to AVDAs.
* AVDACodecAllocator now allocates and releases codecs. When a codec
with an attached surface is released, it tracks the completion of
the async release task so that it can wait on it.
* AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are
routed through AVDACodecAllocator.
BUG=662599
TEST=new tests, manual fullscreen transitions
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
Committed: https://crrev.com/8b00de4de08282a2a667fb6a04c753f0964cc67e
Cr-Commit-Position: refs/heads/master@{#434038}
Patch Set 1 #
Total comments: 8
Patch Set 2 : cleanup/couple more tests #
Total comments: 3
Patch Set 3 : bugs #
Total comments: 5
Patch Set 4 : addressed comments + added some comments #
Total comments: 4
Patch Set 5 : fix comments #Patch Set 6 : change timeout to 2 #
Messages
Total messages: 51 (33 generated)
Description was changed from ========== media: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously, but previously we didn't do sync surface teardown in many cases. Now that it's enabled on all Android API levels this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). BUG= ========== to ========== media: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously, but previously we didn't do sync surface teardown in many cases. Now that it's enabled on all Android API levels this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). BUG= 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: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously, but previously we didn't do sync surface teardown in many cases. Now that it's enabled on all Android API levels this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). BUG= 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: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously, but previously we didn't do sync surface teardown in many cases. Now that it's enabled on all Android API levels this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG= 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
I'm sending this for an early review because branch cut is tomorrow and this should probably go into 56.. I'm still working on testing, and I haven't done a proper self-review so there might be silly mistakes, but PTAL. I'm also open to the idea of scrapping this to be honest. It's not clear that it solves a practical problem; it might just be theoretical. I much prefer solving all the problems, but considering we're going to rework this for DialogSurfaces anyway, I haven't convinced myself that this is worth doing.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
I think some of this is a good cleanup either way. The problem is that the MediaCodec destruction might not happen until after the surface is destroyed right? And on <=KK platforms we hit a CHECK in this case? Did your enable CL make the last dev cut? We can check to see if there are any crashes already, JB has a lot of users on dev/beta for whatever reason :o
i haven't done an in-depth review of it for correctness. tl;dr: i like the refactor. i think that it's worth the effort. i'm less sure that we need to get it into 56, since it's complicated. as for DialogSurface: i think that it'll still be applicable. we're still going to get (sync) surfaceDestroyed there, and it'll be nice to handle it correctly. the exact steps we take might change a little. plus, CVV and DS will almost certainly co-exist until a finch experiment tell us if DS is stable. it's nice to have the logic well-encapsulated rather than what it's like in ToT, so that we can support CVV and DS. -fl https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator.cc (left): https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:404: DCHECK(g_avda_manager.Get().AllocateSurface(config_.surface_id, this)); where did this go? https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:1163: g_avda_manager.Get().StopTimer(this); hrm, that seems like an improvement. https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:1463: AVDACodecAllocator::Get().DeallocateSurface(this, config_.surface_id); one of these has to be "no surface", i think. works okay, but might be worth a comment. good catch, btw. https://codereview.chromium.org/2508053002/diff/1/media/gpu/avda_codec_alloca... File media/gpu/avda_codec_allocator.cc (right): https://codereview.chromium.org/2508053002/diff/1/media/gpu/avda_codec_alloca... media/gpu/avda_codec_allocator.cc:224: // 2) No AVDA owns the surface, but a MediaCodec is currently being configured is this the case where async construction is pending while the avda is destroyed? as in, own surface => start async => destroy => deallocate surface => OnSurfaceDestroyed happens (here) =(some time in the future)=> async completes https://codereview.chromium.org/2508053002/diff/1/media/gpu/avda_codec_alloca... File media/gpu/avda_codec_allocator.h (right): https://codereview.chromium.org/2508053002/diff/1/media/gpu/avda_codec_alloca... media/gpu/avda_codec_allocator.h:93: class AVDACodecAllocatorClient { +1
Thanks! Re looking for dev crashes: good idea. I'll try that. It's in 56.0.2919.3 which is the latest dev. I'll send an update when this is cleaned up. https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator.cc (left): https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:404: DCHECK(g_avda_manager.Get().AllocateSurface(config_.surface_id, this)); On 2016/11/17 07:28:19, liberato wrote: > where did this go? Ah, I added a unit test that mostly obseletes this. (And dcheng@ didn't like this DCHECK in Dale's last CL). https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2508053002/diff/1/media/gpu/android_video_dec... media/gpu/android_video_decode_accelerator.cc:1463: AVDACodecAllocator::Get().DeallocateSurface(this, config_.surface_id); On 2016/11/17 07:28:19, liberato wrote: > one of these has to be "no surface", i think. works okay, but might be worth a > comment. > > good catch, btw. sg will do https://codereview.chromium.org/2508053002/diff/1/media/gpu/avda_codec_alloca... File media/gpu/avda_codec_allocator.cc (right): https://codereview.chromium.org/2508053002/diff/1/media/gpu/avda_codec_alloca... media/gpu/avda_codec_allocator.cc:224: // 2) No AVDA owns the surface, but a MediaCodec is currently being configured On 2016/11/17 07:28:19, liberato wrote: > is this the case where async construction is pending while the avda is > destroyed? as in, own surface => start async => destroy => deallocate surface > => OnSurfaceDestroyed happens (here) =(some time in the future)=> async > completes Yeah exactly. Whether the AVDA is alive or not the result should be the same: the codec will be released (on the main thread) when the OnCodecConfigured task is dropped because the avda weak ptr is invalidated. So we'll do two scary things in that case: release on main thread, and release a MC with destroyed surface. But I'm assuming they're safe because we haven't seen any reports of that failing I don't think. And it kinda makes sense that an unused MC hasn't had a chance to get into any crashy state. I initially wrote a much more ambitious change that also tracked the allocations and waited for them too. But it got too complicated and I realized it probably doesn't matter.. I guess we'll see.
The CQ bit was checked by watk@chromium.org to run a CQ dry run
Description was changed from ========== media: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously, but previously we didn't do sync surface teardown in many cases. Now that it's enabled on all Android API levels this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG= 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: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously, but previously we didn't do sync surface teardown in many cases. Now that it's enabled on all Android API levels this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG=662599 TEST=new tests, manual fullscreen transitions 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 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== media: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously, but previously we didn't do sync surface teardown in many cases. Now that it's enabled on all Android API levels this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG=662599 TEST=new tests, manual fullscreen transitions 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: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously because we post MediaCodec release to another thread. Now that SurfaceDestroyed messages are enabled on all Android API levels, and we don't have deferred error reporting this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG=662599 TEST=new tests, manual fullscreen transitions 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Patchset #3 (id:40001) has been deleted
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...
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Alright, this is ready now. PTAL. https://codereview.chromium.org/2508053002/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2508053002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1151: AVDACodecAllocator::Get().DeallocateSurface(this, config_.surface_id); This was a bug. A waiter could be promoted here before the surface is released. https://codereview.chromium.org/2508053002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1457: AVDACodecAllocator::Get().DeallocateSurface(this, config_.surface_id); Turns out this was completely wrong. We shouldn't deallocate until the surface is released. https://codereview.chromium.org/2508053002/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1462: if (!AVDACodecAllocator::Get().AllocateSurface(this, config_.surface_id)) { This needs a deallocate in case we fail some step below.. error prone api.
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.
ping
lgtm https://codereview.chromium.org/2508053002/diff/100001/media/gpu/android_vide... File media/gpu/android_video_decode_accelerator.cc (left): https://codereview.chromium.org/2508053002/diff/100001/media/gpu/android_vide... media/gpu/android_video_decode_accelerator.cc:1008: ReleaseMediaCodec(); You dropped this. If media_codec_ will always be null here add a dcheck instead. https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... File media/gpu/avda_codec_allocator.cc (right): https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... media/gpu/avda_codec_allocator.cc:236: if (surface_owners_.count(surface_id)) { count will find all instances, so you probably want find even though this should always be 1. https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... media/gpu/avda_codec_allocator.cc:252: // The codec is being released so we have to wait for it here. It's a Sad face. Wish there was a way to avoid this. https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... File media/gpu/avda_codec_allocator.h (right): https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... media/gpu/avda_codec_allocator.h:118: static AVDACodecAllocator& Get(); Typically return pointers in this case.
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...
Thanks! Comments addressed minus the count() one because these are std::maps. liberato@ do you want to look as well? https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... File media/gpu/avda_codec_allocator.cc (right): https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... media/gpu/avda_codec_allocator.cc:118: 31); // PRESUBMIT_IGNORE_UMA_MAX This was already removed and deprecated but made it back in somehow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
On 2016/11/22 02:03:26, watk wrote: > Thanks! Comments addressed minus the count() one because these are std::maps. > > liberato@ do you want to look as well? > > https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... > File media/gpu/avda_codec_allocator.cc (right): > > https://codereview.chromium.org/2508053002/diff/100001/media/gpu/avda_codec_a... > media/gpu/avda_codec_allocator.cc:118: 31); // PRESUBMIT_IGNORE_UMA_MAX > This was already removed and deprecated but made it back in somehow. sorry for the delay -- forgot about this. yeah, i'll look now.
lgtm % nits. also a nice refactor even without the bug fix. -fl https://codereview.chromium.org/2508053002/diff/120001/media/gpu/android_vide... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2508053002/diff/120001/media/gpu/android_vide... media/gpu/android_video_decode_accelerator.cc:1512: AVDACodecAllocator::Instance()->DeallocateSurface(this, config_.surface_id); on success, isn't this now equal to |pending_surface_id| @1498? i think that one must save the old value. https://codereview.chromium.org/2508053002/diff/120001/media/gpu/avda_codec_a... File media/gpu/avda_codec_allocator.cc (right): https://codereview.chromium.org/2508053002/diff/120001/media/gpu/avda_codec_a... media/gpu/avda_codec_allocator.cc:261: released.TimedWait(base::TimeDelta::FromSeconds(4)); even 4 seems high
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...
Thanks, I changed the timeout to 2 seconds. cqing https://codereview.chromium.org/2508053002/diff/120001/media/gpu/android_vide... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2508053002/diff/120001/media/gpu/android_vide... media/gpu/android_video_decode_accelerator.cc:1512: AVDACodecAllocator::Instance()->DeallocateSurface(this, config_.surface_id); On 2016/11/22 18:49:22, liberato wrote: > on success, isn't this now equal to |pending_surface_id| @1498? i think that > one must save the old value. Oh jeez, good catch! Wish I could unit test this :( https://codereview.chromium.org/2508053002/diff/120001/media/gpu/avda_codec_a... File media/gpu/avda_codec_allocator.cc (right): https://codereview.chromium.org/2508053002/diff/120001/media/gpu/avda_codec_a... media/gpu/avda_codec_allocator.cc:261: released.TimedWait(base::TimeDelta::FromSeconds(4)); On 2016/11/22 18:49:22, liberato wrote: > even 4 seems high True, let me reduce it. 2?
The CQ bit was checked by watk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/2508053002/#ps160001 (title: "change timeout to 2")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
watk@chromium.org changed reviewers: + jbauman@chromium.org
jbauman@chromium.org: Please review changes in gpu_child_thread.cc.
content/gpu lgtm
The CQ bit was checked by liberato@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": 160001, "attempt_start_ts": 1479857573693650,
"parent_rev": "c63a0214a7350107aaffa55ef51cad8f23e19421", "commit_rev":
"cd1ba803cf16a9f95a4afde083fa18141787f1cc"}
Message was sent while issue was closed.
Description was changed from ========== media: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously because we post MediaCodec release to another thread. Now that SurfaceDestroyed messages are enabled on all Android API levels, and we don't have deferred error reporting this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG=662599 TEST=new tests, manual fullscreen transitions 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: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously because we post MediaCodec release to another thread. Now that SurfaceDestroyed messages are enabled on all Android API levels, and we don't have deferred error reporting this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG=662599 TEST=new tests, manual fullscreen transitions 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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== media: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously because we post MediaCodec release to another thread. Now that SurfaceDestroyed messages are enabled on all Android API levels, and we don't have deferred error reporting this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG=662599 TEST=new tests, manual fullscreen transitions 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: Do a TimedWait() for video surface teardown in AVDA In OnSurfaceDestroyed() we have to release the MediaCodec synchronously to avoid crashing or hanging. Previously we did the release asynchronously because we post MediaCodec release to another thread. Now that SurfaceDestroyed messages are enabled on all Android API levels, and we don't have deferred error reporting this change makes the teardown more robust by waiting for MediaCodecs to be released before returning from OnSurfaceDestroyed(). * AVDACodecAllocator now tracks the allocation of surfaces to AVDAs. * AVDACodecAllocator now allocates and releases codecs. When a codec with an attached surface is released, it tracks the completion of the async release task so that it can wait on it. * AVDASurfaceTracker is gone. Now OnSurfaceDestroyed messages are routed through AVDACodecAllocator. BUG=662599 TEST=new tests, manual fullscreen transitions 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 Committed: https://crrev.com/8b00de4de08282a2a667fb6a04c753f0964cc67e Cr-Commit-Position: refs/heads/master@{#434038} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8b00de4de08282a2a667fb6a04c753f0964cc67e Cr-Commit-Position: refs/heads/master@{#434038} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
