Commit dcf09c7b authored by liberato@chromium.org's avatar liberato@chromium.org Committed by Commit Bot

Handle MCVD cleanup properly if texture has been destroyed.

If AbstractTexture drops its reference to the underlying texture,
then that texture might have been freed.  This happens when the
gl stub is lost.

Previously, MCVD assumed that the CodecImage attached to the texture
was valid.  However, if the AbstractTexture has dropped its ref
to the texture, and there are no other refs, then this assumption
isn't right.

This CL checks if the AbstractTexture still has a TextureBase before
referencing the CodecImage.

An alternate approach of holding a scoped_refptr to the CodecImage
in the callback would also work, but might keep the CodecImage
around longer than it should when the stub is destroyed.  This can
hold the MediaCodec open longer if the CodecImage has an unrendered
codec buffer.

Bug: 863012
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: If83a9eb0c27d6eb8e995424bdf71f7f7bc93590d
Reviewed-on: https://chromium-review.googlesource.com/1135697Reviewed-by: default avatarDale Curtis <dalecurtis@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574717}
parent 4e070c6b
......@@ -81,6 +81,12 @@ void TexturePool::OnSyncTokenReleased(AbstractTexture* texture,
void TexturePool::OnWillDestroyStub(bool have_context) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(helper_);
// TODO(liberato): Should we drop all unrendered codec buffers here? It seems
// like a good idea, just to release the resources. However, they won't block
// decoding, since decoding requires the stub too. More generally, it might
// be worthwhile to have a callback on AbstractTexture that's called when it
// transitions to not owning a texture.
// Note that we don't have to do anything with |pool_|, since AbstractTextures
// can outlive the stub that created them. They just don't have a texture.
helper_ = nullptr;
......
......@@ -172,8 +172,16 @@ void GpuVideoFrameFactory::CreateVideoFrame(
// the image. It would save a lot of hoops, but would also make the texture
// unrenderable. That's bad when the sync token isn't up to date.
auto sync_token_cb = base::BindOnce(
[](CodecImage* codec_image) { codec_image->ReleaseCodecBuffer(); },
base::Unretained(codec_image));
[](AbstractTexture* texture, CodecImage* codec_image) {
// If |texture| no longer holds the TextureBase, then |codec_image|
// might not be valid anymore if the underlying TextureBase has been
// destroyed. Note that we can't tell if it has been destroyed or not.
// However, this happens only when our stub is destroyed, causing our
// AbstractTexture to forget the texture.
if (texture->GetTextureBase())
codec_image->ReleaseCodecBuffer();
},
base::Unretained(texture.get()), base::Unretained(codec_image));
// Note that this keeps the pool around while any texture is.
auto drop_texture_ref = base::BindOnce(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment