Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1920093003:
media: Handle output SurfaceView destruction in AVDA (Closed)
|
Created:
4 years, 7 months ago by watk Modified:
4 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, piman+watch_chromium.org, posciak+watch_chromium.org, no sievers Base URL:
https://chromium.googlesource.com/chromium/src.git@svteardown Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Handle output SurfaceView destruction in AVDA
Previously AVDA didn't get a callback when its output surface was being
destroyed. Now, with AVDASurfaceTracker, AVDA registers for a callback
in which it releases its MediaCodec to avoid putting MediaCodec into
an invalid state that can causes crashes in some cases.
This also disables AVDA VP8 for api levels 16 and 17, because on those
platform versions we have to fully flush the MediaCodec before
releasing it, and that's something we don't want to handle in the
surface destruction callback.
BUG=598408, 533630
Committed: https://crrev.com/c9a797dd54d4a176eb758189e35942ee45e1dc1f
Cr-Commit-Position: refs/heads/master@{#393392}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Comments + drain and flush logic #
Total comments: 6
Patch Set 3 : Rebase #Patch Set 4 : WIP drain decoder #
Total comments: 3
Patch Set 5 : rebase #Patch Set 6 : Remove draining stuff #Patch Set 7 : Dale's comments and disable VP8 on platforms where we do surface teardown #
Total comments: 1
Messages
Total messages: 28 (7 generated)
watk@chromium.org changed reviewers: + liberato@chromium.org
liberato PTAL. sievers FYI.
Note: this is based on https://codereview.chromium.org/1878103002/ which hasn't landed.
https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:428: on_destroying_surface_cb_ = all of the callback code might be better in the deferred strategy. as in, it can register for the destruction callback, and just notify avda to change state. this might fit better when the strategy chooses when / if to get a surface. STATE_DESTROYED can stay in avda, but it can expose a "SurfaceWasDestroyed" or something. that call would end up moving into MediaCodecLoop, i think. https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:1034: if (state_ == SURFACE_DESTROYED) please add a comment that this will release the media codec and why we want that. https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:1314: strategy_->CodecChanged(media_codec_.get()); should this also clear |drain_type_|, for completeness? i realize that it doesn't really matter, but it seems like a good idea.
https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:428: on_destroying_surface_cb_ = On 2016/04/27 16:50:04, liberato wrote: > all of the callback code might be better in the deferred strategy. as in, it > can register for the destruction callback, and just notify avda to change state. > > this might fit better when the strategy chooses when / if to get a surface. > > STATE_DESTROYED can stay in avda, but it can expose a "SurfaceWasDestroyed" or > something. that call would end up moving into MediaCodecLoop, i think. I'm not sure if I agree. I think this is simpler in terms of less plumbing, and conceptually in my mind. The reason we really care about surface destruction is because we need to release the MediaCodec, and that's AVDAs job. https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:1034: if (state_ == SURFACE_DESTROYED) On 2016/04/27 16:50:04, liberato wrote: > please add a comment that this will release the media codec and why we want > that. Done. https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:1314: strategy_->CodecChanged(media_codec_.get()); On 2016/04/27 16:50:04, liberato wrote: > should this also clear |drain_type_|, for completeness? > > i realize that it doesn't really matter, but it seems like a good idea. Good call, I think this was broken if a drain was ongoing (especially DRAIN_FOR_DESTROY). Added logic to complete the drain. I also made flush return early if the surface is lost.
lgtm https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:428: on_destroying_surface_cb_ = On 2016/04/27 18:54:04, watk wrote: > On 2016/04/27 16:50:04, liberato wrote: > > all of the callback code might be better in the deferred strategy. as in, it > > can register for the destruction callback, and just notify avda to change > state. > > > > this might fit better when the strategy chooses when / if to get a surface. > > > > STATE_DESTROYED can stay in avda, but it can expose a "SurfaceWasDestroyed" or > > something. that call would end up moving into MediaCodecLoop, i think. > > I'm not sure if I agree. I think this is simpler in terms of less plumbing, and > conceptually in my mind. The reason we really care about surface destruction is > because we need to release the MediaCodec, and that's AVDAs job. what brought it up is that AVDA didn't even record the surface_id before. it was all contained in the strategy. now, avda peeks at the surface, and assumes what the strategy is going to do with it. granted, right now, the strategy has no choice since WMPI also assumes it, but that's another issue. :) to be clear, i'm not suggesting moving anything beside the callback registration / unregistration itself. it's not a big deal at this point, but will probably happen anyway when the strategy starts worrying about when to allocate the surface. https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:1314: strategy_->CodecChanged(media_codec_.get()); On 2016/04/27 18:54:04, watk wrote: > On 2016/04/27 16:50:04, liberato wrote: > > should this also clear |drain_type_|, for completeness? > > > > i realize that it doesn't really matter, but it seems like a good idea. > > Good call, I think this was broken if a drain was ongoing (especially > DRAIN_FOR_DESTROY). Added logic to complete the drain. I also made flush return > early if the surface is lost. actually, i didn't think about it that much. good point on OnDrainCompleted.
watk@chromium.org changed reviewers: + dalecurtis@chromium.org
If you get a minute, could you give this a quick look please Dale? https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:428: on_destroying_surface_cb_ = On 2016/04/27 21:00:37, liberato wrote: > On 2016/04/27 18:54:04, watk wrote: > > On 2016/04/27 16:50:04, liberato wrote: > > > all of the callback code might be better in the deferred strategy. as in, > it > > > can register for the destruction callback, and just notify avda to change > > state. > > > > > > this might fit better when the strategy chooses when / if to get a surface. > > > > > > STATE_DESTROYED can stay in avda, but it can expose a "SurfaceWasDestroyed" > or > > > something. that call would end up moving into MediaCodecLoop, i think. > > > > I'm not sure if I agree. I think this is simpler in terms of less plumbing, > and > > conceptually in my mind. The reason we really care about surface destruction > is > > because we need to release the MediaCodec, and that's AVDAs job. > > what brought it up is that AVDA didn't even record the surface_id before. it > was all contained in the strategy. now, avda peeks at the surface, and assumes > what the strategy is going to do with it. granted, right now, the strategy has > no choice since WMPI also assumes it, but that's another issue. :) > > to be clear, i'm not suggesting moving anything beside the callback registration > / unregistration itself. it's not a big deal at this point, but will probably > happen anyway when the strategy starts worrying about when to allocate the > surface. Gotcha, well since you're not too worried I'll defer that change until it's more clear to me how it will look.
https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:953: if (state_ == SURFACE_DESTROYED) { No unnecessary parens. https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1251: } Clear surface destruction cb? https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1314: // If we're currently asynchronously configuring a codec, it will be destroyed Hmm, this may have bad interactions where we need to complete drain first right? I.e. should we release all output buffers and flush codec synchronously here? https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1322: // If we're draining, signal completion now because the drain can no longer Line above. https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1324: if (drain_type_ != DRAIN_TYPE_NONE) { No unnecessary parens.
https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1314: // If we're currently asynchronously configuring a codec, it will be destroyed On 2016/04/27 21:20:35, DaleCurtis wrote: > Hmm, this may have bad interactions where we need to complete drain first right? > I.e. should we release all output buffers and flush codec synchronously here? Oh, good point. :( This is going to be painful.
Patchset #3 (id:40001) has been deleted
Just posting my WIP before I leave for the day. I haven't compiled it yet, but I wanted to get some feedback on how crazy it is.
https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1320: media_codec_->DequeueInputBuffer(kTimeout, &input_buf_index); input buffers might become available after we decode output buffers. i don't think that one can do this only once. it might be better to start a drain normally via StartCodecDrain, which queues an EOS bitstream bfufer, then drive DoIOTask() here until done / timeout. not sure off-hand if you would need a new drain type, or if you can use DESTROY.
https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1320: media_codec_->DequeueInputBuffer(kTimeout, &input_buf_index); On 2016/05/04 14:19:56, liberato wrote: > input buffers might become available after we decode output buffers. i don't > think that one can do this only once. Do we care about input buffers? We only need to queue one eos. > it might be better to start a drain normally via StartCodecDrain, which queues > an EOS bitstream bfufer, then drive DoIOTask() here until done / timeout. not > sure off-hand if you would need a new drain type, or if you can use DESTROY. I can try that. I was hoping this would be simple enough to offset the code duplication. DoIOTask has conditions that we have to audit and make sure we don't break going forward.
https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1320: media_codec_->DequeueInputBuffer(kTimeout, &input_buf_index); On 2016/05/04 18:18:46, watk wrote: > On 2016/05/04 14:19:56, liberato wrote: > > input buffers might become available after we decode output buffers. i don't > > think that one can do this only once. > > Do we care about input buffers? We only need to queue one eos. > > > > it might be better to start a drain normally via StartCodecDrain, which queues > > an EOS bitstream bfufer, then drive DoIOTask() here until done / timeout. not > > sure off-hand if you would need a new drain type, or if you can use DESTROY. > > I can try that. I was hoping this would be simple enough to offset the code > duplication. DoIOTask has conditions that we have to audit and make sure we > don't break going forward. my concern is how does one know that an input buffer will be available right now? maybe they're all full, pending avda releasing an output buffer to decode the next input buffer into.
On 2016/05/04 18:30:00, liberato wrote: > https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... > File media/gpu/android_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/1920093003/diff/80001/media/gpu/android_video... > media/gpu/android_video_decode_accelerator.cc:1320: > media_codec_->DequeueInputBuffer(kTimeout, &input_buf_index); > On 2016/05/04 18:18:46, watk wrote: > > On 2016/05/04 14:19:56, liberato wrote: > > > input buffers might become available after we decode output buffers. i > don't > > > think that one can do this only once. > > > > Do we care about input buffers? We only need to queue one eos. > > > > > > > it might be better to start a drain normally via StartCodecDrain, which > queues > > > an EOS bitstream bfufer, then drive DoIOTask() here until done / timeout. > not > > > sure off-hand if you would need a new drain type, or if you can use DESTROY. > > > > I can try that. I was hoping this would be simple enough to offset the code > > duplication. DoIOTask has conditions that we have to audit and make sure we > > don't break going forward. > > my concern is how does one know that an input buffer will be available right > now? maybe they're all full, pending avda releasing an output buffer to decode > the next input buffer into. Oh, you're right (though hard to imagine this happening in practice.) Thanks for the explanation.
I started trying to do this, but it felt crazy. What if I just disallow the SurfaceView, VP8, JellyBean combo and use SurfaceTexture instead? Though that makes me wonder if the VP8 flush() hanging bug might also occur if the surface is lost and we can't flush the decoder with an EOS. Which would mean we would have to either blacklist SurfaceView for all VP8, or plumb OnDestroyingSurface for all Android versions.
Alright! I think it's finally ready
https://codereview.chromium.org/1920093003/diff/140001/media/gpu/android_vide... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1920093003/diff/140001/media/gpu/android_vide... media/gpu/android_video_decode_accelerator.cc:1461: if (base::android::BuildInfo::GetInstance()->sdk_int() >= 18 && should this be > rather than >= ? 18 is JB i think.
On 2016/05/11 22:09:00, liberato wrote: > https://codereview.chromium.org/1920093003/diff/140001/media/gpu/android_vide... > File media/gpu/android_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/1920093003/diff/140001/media/gpu/android_vide... > media/gpu/android_video_decode_accelerator.cc:1461: if > (base::android::BuildInfo::GetInstance()->sdk_int() >= 18 && > should this be > rather than >= ? 18 is JB i think. True, 18 is JB MR2, this matches when we send the DestroyingSurface callback, and I chose 18 for that because I'd only seen crashes for 17 I believe. 18 is when they added the CTS tests too.
Description was changed from ========== media: Handle output SurfaceView destruction in AVDA Previously AVDA didn't get a callback when its output surface was being destroyed. Now, with AVDASurfaceTracker, AVDA registers for a callback in which it releases its MediaCodec to avoid putting MediaCodec into an invalid state that can causes crashes in some cases. BUG=598408,533630 ========== to ========== media: Handle output SurfaceView destruction in AVDA Previously AVDA didn't get a callback when its output surface was being destroyed. Now, with AVDASurfaceTracker, AVDA registers for a callback in which it releases its MediaCodec to avoid putting MediaCodec into an invalid state that can causes crashes in some cases. This also disables AVDA VP8 for api levels 16 and 17, because on those platform versions we have to fully flush the MediaCodec before releasing it, and that's something we don't want to handle in the surface destruction callback. BUG=598408,533630 ==========
lgtm
The CQ bit was checked by watk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920093003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920093003/140001
Message was sent while issue was closed.
Description was changed from ========== media: Handle output SurfaceView destruction in AVDA Previously AVDA didn't get a callback when its output surface was being destroyed. Now, with AVDASurfaceTracker, AVDA registers for a callback in which it releases its MediaCodec to avoid putting MediaCodec into an invalid state that can causes crashes in some cases. This also disables AVDA VP8 for api levels 16 and 17, because on those platform versions we have to fully flush the MediaCodec before releasing it, and that's something we don't want to handle in the surface destruction callback. BUG=598408,533630 ========== to ========== media: Handle output SurfaceView destruction in AVDA Previously AVDA didn't get a callback when its output surface was being destroyed. Now, with AVDASurfaceTracker, AVDA registers for a callback in which it releases its MediaCodec to avoid putting MediaCodec into an invalid state that can causes crashes in some cases. This also disables AVDA VP8 for api levels 16 and 17, because on those platform versions we have to fully flush the MediaCodec before releasing it, and that's something we don't want to handle in the surface destruction callback. BUG=598408,533630 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== media: Handle output SurfaceView destruction in AVDA Previously AVDA didn't get a callback when its output surface was being destroyed. Now, with AVDASurfaceTracker, AVDA registers for a callback in which it releases its MediaCodec to avoid putting MediaCodec into an invalid state that can causes crashes in some cases. This also disables AVDA VP8 for api levels 16 and 17, because on those platform versions we have to fully flush the MediaCodec before releasing it, and that's something we don't want to handle in the surface destruction callback. BUG=598408,533630 ========== to ========== media: Handle output SurfaceView destruction in AVDA Previously AVDA didn't get a callback when its output surface was being destroyed. Now, with AVDASurfaceTracker, AVDA registers for a callback in which it releases its MediaCodec to avoid putting MediaCodec into an invalid state that can causes crashes in some cases. This also disables AVDA VP8 for api levels 16 and 17, because on those platform versions we have to fully flush the MediaCodec before releasing it, and that's something we don't want to handle in the surface destruction callback. BUG=598408,533630 Committed: https://crrev.com/c9a797dd54d4a176eb758189e35942ee45e1dc1f Cr-Commit-Position: refs/heads/master@{#393392} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c9a797dd54d4a176eb758189e35942ee45e1dc1f Cr-Commit-Position: refs/heads/master@{#393392} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
