AVDACodecImages keep their backing SurfaceTexture alive

AVDACodecImages keep their backing SurfaceTexture alive

Previously, when destructing an AVDA, we had to delete the
AVDACodecImages attached to the PictureBuffer textures, and
use a fragile method of copying the SurfaceTexture buffer
and creating egl images to make sure those PictureBuffer
textures didn't become unbacked.

Now the AVDACodecImages are kept alive by the Textures
they're bound to, and they keep a ref to AVDASharedState
which contains the shared SurfaceTexture and its platform
texture. The AVDASharedState destructor cleans up the
SurfaceTexture and deletes the platform texture.

BUG=533630, 614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/3e465933721f2732fc422029529b80fc9784e812
Cr-Commit-Position: refs/heads/master@{#398293}

[watk, 2016-05-25 22:30:18]

Description was changed from

==========
AVDACodecImages keep their backing SurfaceTexture alive

TODO

BUG=533630,614090
==========

to

==========
AVDACodecImages keep their backing SurfaceTexture alive

TODO

BUG=533630,614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2016-05-26 16:59:18]

liberato@chromium.org changed reviewers:
+ liberato@chromium.org

[liberato (no reviews please), 2016-05-26 16:59:19]

very nice.  just a few thoughts.

-fl

https://codereview.chromium.org/2005103004/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/2005103004/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/texture_manager.cc:1498: if (image_previously_bound
&& level == 0)
i don't think that one needs to check if an image was previously bound, or even
(probably) the level.

AVDA can set them in the right order.

also, please add documentation to texture_manager.h::SetUnownedServiceId that
it's cleared on any SetLevelImage.  else it'll confuse somebody (probably us :)
) some day.

the texture_manager changes can probably be a separate CL.  it looks like AVDA
does the SetLevel / SetUnowned calls in the right order already.  i think that
they stand on their own as a security improvement.

https://codereview.chromium.org/2005103004/diff/1/media/gpu/android_video_dec...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2005103004/diff/1/media/gpu/android_video_dec...
media/gpu/android_video_decode_accelerator.cc:1370: // To reduce context
switching we used to create an unattached
comment is out of date.

[watk, 2016-06-06 21:14:26]

Description was changed from

==========
AVDACodecImages keep their backing SurfaceTexture alive

TODO

BUG=533630,614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
AVDACodecImages keep their backing SurfaceTexture alive

Previously, when destructing an AVDA, we had to delete the
AVDACodecImages attached to the PictureBuffer textures, and
use a fragile method of copying the SurfaceTexture buffer
and creating egl images to make sure those PictureBuffer
textures didn't become unbacked.

Now the AVDACodecImages are kept alive by the Textures
they're bound to, and they keep a ref to AVDASharedState
which contains the shared SurfaceTexture and its platform
texture. The AVDASharedState destructor cleans up the
SurfaceTexture and deletes the platform texture.

BUG=533630,614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[watk, 2016-06-06 21:14:52]

The TextureManager change is in, so PTAL

[liberato (no reviews please), 2016-06-06 21:35:55]

lgtm.

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
File media/gpu/android_deferred_rendering_backing_strategy.cc (left):

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
media/gpu/android_deferred_rendering_backing_strategy.cc:338: void
AndroidDeferredRenderingBackingStrategy::CopySurfaceTextureToPictures(
yay!

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
File media/gpu/android_deferred_rendering_backing_strategy.cc (right):

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
media/gpu/android_deferred_rendering_backing_strategy.cc:53: return
gpu::GpuSurfaceLookup::GetInstance()->AcquireJavaSurface(
yay!

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
media/gpu/android_deferred_rendering_backing_strategy.cc:59: surface_texture_ =
state_provider_->CreateAttachedSurfaceTexture(&service_id);
maybe if (!surface_texture_) return early.  maybe gl::ScopedJavaSurface(...)
does the right thing, don't know.  probably shouldn't call into |shared_state_|
though.  maybe just protect that with if (surface_texture_ != nullptr).

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
media/gpu/android_deferred_rendering_backing_strategy.cc:138: surface_texture_ ?
gpu::gles2::Texture::UNBOUND
this is weird.  if we have no surface texture, and we're clearing the image,
then it'll set the state to BOUND.  does that actually happen?

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/avda_codec_im...
File media/gpu/avda_codec_image.h (right):

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/avda_codec_im...
media/gpu/avda_codec_image.h:22: class VideoCodecBridge;
was this because of the include change in shared_state?

[watk, 2016-06-07 02:22:10]

Thanks! Just played around with it on JB as well: worked fine. 

If you're still happy with the changes can you check the CQ for me when you get
in please? :)

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
File media/gpu/android_deferred_rendering_backing_strategy.cc (right):

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
media/gpu/android_deferred_rendering_backing_strategy.cc:59: surface_texture_ =
state_provider_->CreateAttachedSurfaceTexture(&service_id);
On 2016/06/06 21:35:54, liberato wrote:
> maybe if (!surface_texture_) return early.  maybe gl::ScopedJavaSurface(...)
> does the right thing, don't know.  probably shouldn't call into
|shared_state_|
> though.  maybe just protect that with if (surface_texture_ != nullptr).

I think we're okay because CreateAttachedSurfaceTexture can't fail (AFAICT).

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/android_defer...
media/gpu/android_deferred_rendering_backing_strategy.cc:138: surface_texture_ ?
gpu::gles2::Texture::UNBOUND
On 2016/06/06 21:35:54, liberato wrote:
> this is weird.  if we have no surface texture, and we're clearing the image,
> then it'll set the state to BOUND.  does that actually happen?

Ah, good point. That's weird and probably a bug. Fixed.

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/avda_codec_im...
File media/gpu/avda_codec_image.h (right):

https://codereview.chromium.org/2005103004/diff/40001/media/gpu/avda_codec_im...
media/gpu/avda_codec_image.h:22: class VideoCodecBridge;
On 2016/06/06 21:35:54, liberato wrote:
> was this because of the include change in shared_state?

Must have been :\ I just did what I had to in order to please the compiler.

[liberato (no reviews please), 2016-06-07 14:06:46]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-06-07 14:06:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2005103004/#ps60001
(title: "Comments and add a DCHECK for glGetError after glDelete")

[commit-bot: I haz the power, 2016-06-07 14:07:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005103004/60001

[commit-bot: I haz the power, 2016-06-07 14:17:16]

Description was changed from

==========
AVDACodecImages keep their backing SurfaceTexture alive

Previously, when destructing an AVDA, we had to delete the
AVDACodecImages attached to the PictureBuffer textures, and
use a fragile method of copying the SurfaceTexture buffer
and creating egl images to make sure those PictureBuffer
textures didn't become unbacked.

Now the AVDACodecImages are kept alive by the Textures
they're bound to, and they keep a ref to AVDASharedState
which contains the shared SurfaceTexture and its platform
texture. The AVDASharedState destructor cleans up the
SurfaceTexture and deletes the platform texture.

BUG=533630,614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
AVDACodecImages keep their backing SurfaceTexture alive

Previously, when destructing an AVDA, we had to delete the
AVDACodecImages attached to the PictureBuffer textures, and
use a fragile method of copying the SurfaceTexture buffer
and creating egl images to make sure those PictureBuffer
textures didn't become unbacked.

Now the AVDACodecImages are kept alive by the Textures
they're bound to, and they keep a ref to AVDASharedState
which contains the shared SurfaceTexture and its platform
texture. The AVDASharedState destructor cleans up the
SurfaceTexture and deletes the platform texture.

BUG=533630,614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-06-07 14:17:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-07 14:18:32]

Description was changed from

==========
AVDACodecImages keep their backing SurfaceTexture alive

Previously, when destructing an AVDA, we had to delete the
AVDACodecImages attached to the PictureBuffer textures, and
use a fragile method of copying the SurfaceTexture buffer
and creating egl images to make sure those PictureBuffer
textures didn't become unbacked.

Now the AVDACodecImages are kept alive by the Textures
they're bound to, and they keep a ref to AVDASharedState
which contains the shared SurfaceTexture and its platform
texture. The AVDASharedState destructor cleans up the
SurfaceTexture and deletes the platform texture.

BUG=533630,614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
AVDACodecImages keep their backing SurfaceTexture alive

Previously, when destructing an AVDA, we had to delete the
AVDACodecImages attached to the PictureBuffer textures, and
use a fragile method of copying the SurfaceTexture buffer
and creating egl images to make sure those PictureBuffer
textures didn't become unbacked.

Now the AVDACodecImages are kept alive by the Textures
they're bound to, and they keep a ref to AVDASharedState
which contains the shared SurfaceTexture and its platform
texture. The AVDASharedState destructor cleans up the
SurfaceTexture and deletes the platform texture.

BUG=533630,614090
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/3e465933721f2732fc422029529b80fc9784e812
Cr-Commit-Position: refs/heads/master@{#398293}
==========

[commit-bot: I haz the power, 2016-06-07 14:18:33]

Patchset 4 (id:??) landed as
https://crrev.com/3e465933721f2732fc422029529b80fc9784e812
Cr-Commit-Position: refs/heads/master@{#398293}