AVDACodecImages keep a reference to the SurfaceTexture backing them

AVDACodecImages keep a reference to the SurfaceTexture backing them

Previously an AVDACodecImage became unbacked when the
AndroidDeferredRenderingBackingStrategy was destructed. Now the images keep
a reference to the SurfaceTexture so they remain drawable. This helps when
the media pipeline is suspended, and AVDA is destructed, but video frames
should still be visible. At the moment this is visible when foregrounding
a background tab with a video element, but will help with making fullscreen
transitions look better too.

BUG=533630

[watk, 2016-02-10 02:02:19]

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

[watk, 2016-02-10 02:02:20]

Sorry about the unrelated clean up changes. I always get carried away.

[liberato (no reviews please), 2016-02-10 06:39:12]

this is a good idea.

i'll go over this in more detail in the morning, but just wanted to get a few
thoughts written down before i forget them.

thanks
-fl

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(left):

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:65:
SetImageForPicture(entry.second, nullptr);
it's late, and i might very well be missing it, but i don't see how this happens
in this CL.  it has to happen before the glDeleteTextures for the surface
texture service_id.

otherwise, there could be Texture objects that still refer to that service id
after it's deleted.  removing the codec image itself isn't needed, but calling
SetUnownedServiceId(0) on all the picture buffer textures is.

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (left):

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:876:
DCHECK(strategy_);
why remove this?

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
File content/common/gpu/media/avda_shared_state.cc (right):

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_shared_state.cc:19: // XXX: This is the wrong
context.
it's worth a comment that it's a different one that it was created on, but it's
not a bug.  they're guaranteed to be in the same share group, else nothing would
work, so this is fine.

[liberato (no reviews please), 2016-02-10 21:05:14]

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
File content/common/gpu/media/avda_shared_state.cc (right):

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_shared_state.cc:17:
AVDASharedState::~AVDASharedState() {
i think that you have to maintain a list of Textures in the shared state, and
SetUnownedServiceId(0) on each of them.  that can be done without a context.

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_shared_state.cc:18: if
(surface_texture_service_id_ != 0 && context_) {
it might be worthwhile to pass in the avda's context and surface during
construction, and use it here instead of |context_|.  otherwise, if we never
display a frame, it'll leak a texture since the surface texture won't be
attached.

[liberato (no reviews please), 2016-02-10 21:05:15]

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
File content/common/gpu/media/avda_shared_state.cc (right):

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_shared_state.cc:17:
AVDASharedState::~AVDASharedState() {
i think that you have to maintain a list of Textures in the shared state, and
SetUnownedServiceId(0) on each of them.  that can be done without a context.

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_shared_state.cc:18: if
(surface_texture_service_id_ != 0 && context_) {
it might be worthwhile to pass in the avda's context and surface during
construction, and use it here instead of |context_|.  otherwise, if we never
display a frame, it'll leak a texture since the surface texture won't be
attached.

[watk, 2016-02-11 00:26:51]

This stuff is getting more subtle with every change we make. I'm not sure how to
make it simpler. 

It feels weird to keep refcounts on contexts from these images. Is losing a
context a thing we have to worry about? Or can that not happen with these
contexts?

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (left):

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:876:
DCHECK(strategy_);
On 2016/02/10 06:39:12, liberato wrote:
> why remove this?

strategy_ can't be null any more because it's always set in the constructor.

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
File content/common/gpu/media/avda_shared_state.cc (right):

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_shared_state.cc:17:
AVDASharedState::~AVDASharedState() {
On 2016/02/10 21:05:15, liberato wrote:
> i think that you have to maintain a list of Textures in the shared state, and
> SetUnownedServiceId(0) on each of them.  that can be done without a context.

Discussed offline that this is not necessary because this destructor will be
invoked after the destructors of all Textures that might refer to it.

https://codereview.chromium.org/1682343002/diff/1/content/common/gpu/media/av...
content/common/gpu/media/avda_shared_state.cc:19: // XXX: This is the wrong
context.
On 2016/02/10 06:39:12, liberato wrote:
> it's worth a comment that it's a different one that it was created on, but
it's
> not a bug.  they're guaranteed to be in the same share group, else nothing
would
> work, so this is fine.

Ah, that's why everything works, thanks.

[liberato (no reviews please), 2016-02-11 15:55:42]

hopefully i'm just wrong and there's no problem.

-fl

https://codereview.chromium.org/1682343002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1682343002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:27:
glDeleteTextures(1, &service_id);
the more i think about it after our conversation offline, there might still be a
problem with relying on the strong refs to destroy the textures before the
glDeleteTextures happens.  if one binds a new glimage to replace the
AVCodecImage, then the texture could outlive the shared state while its unowned
service id is still set to |service_id|.  at least, i think so, since the
Texture doesn't directly reference the shared state.

while we won't cause that here, the renderer could also do that via glBindImage.
 it's a potential security issue.

since we're relying on the textures to be destroyed to trigger cleanup, we can't
keep a (texture)ref to them.  i don't believe that there's any notification when
a glimage changes.

maybe we could force the unowned service id back to 0 whenever the glimage is
changed, but that seems awful.

here's an alternate approach.  during AVDA::Cleanup, make a copy of the
SurfaceTexture into a teximage2D.  destroy the surface texture, unset all the
UnownedServiceIds to 0, and glDeleteTextures the surface texture's old client
texture.

each PictureBuffer texture will be an unused texture object.  bind the EGLImage
to each of the picturebuffer textures.  reveman tells me that this works --
binding an eglimage created from a 2D texture onto one that will be bound to an
external sampler.

also, one can delete the EGLImage and the newly created 2D texture immediately,
i think.  the PictureBuffer textures don't rely on it (have to check that too --
i think that's how it works).  if that's not true, then we've traded one clean
up problem for another :)

if you can think of a way to fix the cleanup story without all that work, that
would be okay too.  i'll give it more thought, but i don't see a way to do it at
the moment.

https://codereview.chromium.org/1682343002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:64:
shared_state_->SetSurfaceTextureServiceID(
good idea.

[watk, 2016-02-11 21:21:58]

https://codereview.chromium.org/1682343002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc
(right):

https://codereview.chromium.org/1682343002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:27:
glDeleteTextures(1, &service_id);
On 2016/02/11 15:55:41, liberato wrote:
> the more i think about it after our conversation offline, there might still be
a
> problem with relying on the strong refs to destroy the textures before the
> glDeleteTextures happens.  if one binds a new glimage to replace the
> AVCodecImage, then the texture could outlive the shared state while its
unowned
> service id is still set to |service_id|.  at least, i think so, since the
> Texture doesn't directly reference the shared state.
> 
> while we won't cause that here, the renderer could also do that via
glBindImage.
>  it's a potential security issue.
> 
> since we're relying on the textures to be destroyed to trigger cleanup, we
can't
> keep a (texture)ref to them.  i don't believe that there's any notification
when
> a glimage changes.
> 
> maybe we could force the unowned service id back to 0 whenever the glimage is
> changed, but that seems awful.
> 
> here's an alternate approach.  during AVDA::Cleanup, make a copy of the
> SurfaceTexture into a teximage2D.  destroy the surface texture, unset all the
> UnownedServiceIds to 0, and glDeleteTextures the surface texture's old client
> texture.
> 
> each PictureBuffer texture will be an unused texture object.  bind the
EGLImage
> to each of the picturebuffer textures.  reveman tells me that this works --
> binding an eglimage created from a 2D texture onto one that will be bound to
an
> external sampler.
> 
> also, one can delete the EGLImage and the newly created 2D texture
immediately,
> i think.  the PictureBuffer textures don't rely on it (have to check that too
--
> i think that's how it works).  if that's not true, then we've traded one clean
> up problem for another :)
> 
> if you can think of a way to fix the cleanup story without all that work, that
> would be okay too.  i'll give it more thought, but i don't see a way to do it
at
> the moment.

:( Thanks for catching this. I'll have to look into the copy approach you
suggested and pray we don't have the same cleanup problem.

[watk, 2016-02-17 21:32:49]

watk@chromium.org changed reviewers:
+ sandersd@chromium.org

[watk, 2016-02-17 21:32:49]

sandersd@: Any thoughts?

[watk, 2016-04-01 00:26:31]

Frank, Dan and I took a look at this and it seems like maybe this is still
possible if we move the responsibility for calling SetUnownedServiceId(0) to the
AVDACodecImage destructor. So if a different image is bound, the AVDACodecImage
destructor will reset the unowned id.

If the Texture is deleted, AVDACodecImage needs a way to detect that its Texture
was deleted to avoid calling SetUnownedServiceId, but other than that there seem
to be no issues.

Is this true? Or are there other subtleties that I've forgotten about? It would
be really nice to get rid of the crash prone copySurfaceTextureToPictures..

[liberato (no reviews please), 2016-04-01 05:58:08]

On 2016/04/01 00:26:31, watk wrote:
> Frank, Dan and I took a look at this and it seems like maybe this is still
> possible if we move the responsibility for calling SetUnownedServiceId(0) to
the
> AVDACodecImage destructor. So if a different image is bound, the
AVDACodecImage
> destructor will reset the unowned id.
> 
> If the Texture is deleted, AVDACodecImage needs a way to detect that its
Texture
> was deleted to avoid calling SetUnownedServiceId, but other than that there
seem
> to be no issues.
> 
> Is this true? Or are there other subtleties that I've forgotten about? It
would
> be really nice to get rid of the crash prone copySurfaceTextureToPictures..

i think it's slightly more complicated than that, but not too much.  one also
needs something to own the SurfaceTexture and the gl texture that AVDA creates
for it.

i think that you need something like this:

- shared state between AVDACodecImage that holds the ST and the service_id of
the ST texture that AVDA created.
-- this might be avda_shared_state, in fact.  not sure if it can be re-used. 
probably.
- the shared state owns the ST texture.  in its destructor, it deletes it.
- AVDACodecImage has a strong ref to the shared state
-- keeps texture alive as long as any avdacodecimage / texture needs it.

to fix the 'texture was deleted' case:
- a new call GLStreamTextureImage::NotifyDetachedFromTexture(Texture*)
- Texture::SetLevelImage variants have to call NotifyDetachedFromTexture if the
outgoing image is a stream texture
-- this is trivial, since Texture already knows that it has a
GLStreamTextureImage
- codecimage::NotifyDetached... would clear the unowned texture id from the
outgoing texture.
-- codecimage can also drop its strong ref to shared_state, if desired.
-- codecimage is inactive at this point.  attaching it to other textures does
nothing.
- codecimage currently holds a raw ptr to the texture, which probably is bad. 
Texture::Destroy should also call NotifyDetached, and NotifyDetached should
clear that raw ptr.
-- shouldn't be possible to attach a codec image to another Texture, but it's
something we ignore right now.  it could cause a crash if somebody manages to do
it.
-- this makes it safe to attach a codec image elsewhere, so that we can quit
worrying about it.

i was thinking about this recently.  once we have this, it's almost
straightforward to also include the codec itself in the shared state.  then, the
textures show the correct frames, rather than the most recent front buffer.

[watk, 2016-04-01 20:29:10]

On 2016/04/01 05:58:08, liberato wrote:
> On 2016/04/01 00:26:31, watk wrote:
> > Frank, Dan and I took a look at this and it seems like maybe this is still
> > possible if we move the responsibility for calling SetUnownedServiceId(0) to
> the
> > AVDACodecImage destructor. So if a different image is bound, the
> AVDACodecImage
> > destructor will reset the unowned id.
> > 
> > If the Texture is deleted, AVDACodecImage needs a way to detect that its
> Texture
> > was deleted to avoid calling SetUnownedServiceId, but other than that there
> seem
> > to be no issues.
> > 
> > Is this true? Or are there other subtleties that I've forgotten about? It
> would
> > be really nice to get rid of the crash prone copySurfaceTextureToPictures..
> 
> i think it's slightly more complicated than that, but not too much.  one also
> needs something to own the SurfaceTexture and the gl texture that AVDA creates
> for it.
> 
> i think that you need something like this:
> 
> - shared state between AVDACodecImage that holds the ST and the service_id of
> the ST texture that AVDA created.
> -- this might be avda_shared_state, in fact.  not sure if it can be re-used. 
> probably.
> - the shared state owns the ST texture.  in its destructor, it deletes it.
> - AVDACodecImage has a strong ref to the shared state
> -- keeps texture alive as long as any avdacodecimage / texture needs it.
> 
> to fix the 'texture was deleted' case:
> - a new call GLStreamTextureImage::NotifyDetachedFromTexture(Texture*)
> - Texture::SetLevelImage variants have to call NotifyDetachedFromTexture if
the
> outgoing image is a stream texture
> -- this is trivial, since Texture already knows that it has a
> GLStreamTextureImage
> - codecimage::NotifyDetached... would clear the unowned texture id from the
> outgoing texture.
> -- codecimage can also drop its strong ref to shared_state, if desired.
> -- codecimage is inactive at this point.  attaching it to other textures does
> nothing.
> - codecimage currently holds a raw ptr to the texture, which probably is bad. 
> Texture::Destroy should also call NotifyDetached, and NotifyDetached should
> clear that raw ptr.
> -- shouldn't be possible to attach a codec image to another Texture, but it's
> something we ignore right now.  it could cause a crash if somebody manages to
do
> it.
> -- this makes it safe to attach a codec image elsewhere, so that we can quit
> worrying about it.
> 
> i was thinking about this recently.  once we have this, it's almost
> straightforward to also include the codec itself in the shared state.  then,
the
> textures show the correct frames, rather than the most recent front buffer.

Nice. The first bit is implemented in this CL.

For the second bit, that sounds like it would work. Maybe we could avoid the
GLImage notification if AVDACI could look up the Texture it was originally
attached to via the client id (even if it's possible to attach the same image to
multiple textures we only need to reset unowned id on the original). That would
rely on the lookup returning NULL if the texture is being deleted (i.e., if
we're running inside the Texture destructor), and I don't know if that's true.
Crazy?

[liberato (no reviews please), 2016-04-01 21:49:07]

On 2016/04/01 20:29:10, watk wrote:
> On 2016/04/01 05:58:08, liberato wrote:
> > On 2016/04/01 00:26:31, watk wrote:
> > > Frank, Dan and I took a look at this and it seems like maybe this is still
> > > possible if we move the responsibility for calling SetUnownedServiceId(0)
to
> > the
> > > AVDACodecImage destructor. So if a different image is bound, the
> > AVDACodecImage
> > > destructor will reset the unowned id.
> > > 
> > > If the Texture is deleted, AVDACodecImage needs a way to detect that its
> > Texture
> > > was deleted to avoid calling SetUnownedServiceId, but other than that
there
> > seem
> > > to be no issues.
> > > 
> > > Is this true? Or are there other subtleties that I've forgotten about? It
> > would
> > > be really nice to get rid of the crash prone
copySurfaceTextureToPictures..
> > 
> > i think it's slightly more complicated than that, but not too much.  one
also
> > needs something to own the SurfaceTexture and the gl texture that AVDA
creates
> > for it.
> > 
> > i think that you need something like this:
> > 
> > - shared state between AVDACodecImage that holds the ST and the service_id
of
> > the ST texture that AVDA created.
> > -- this might be avda_shared_state, in fact.  not sure if it can be re-used.

> > probably.
> > - the shared state owns the ST texture.  in its destructor, it deletes it.
> > - AVDACodecImage has a strong ref to the shared state
> > -- keeps texture alive as long as any avdacodecimage / texture needs it.
> > 
> > to fix the 'texture was deleted' case:
> > - a new call GLStreamTextureImage::NotifyDetachedFromTexture(Texture*)
> > - Texture::SetLevelImage variants have to call NotifyDetachedFromTexture if
> the
> > outgoing image is a stream texture
> > -- this is trivial, since Texture already knows that it has a
> > GLStreamTextureImage
> > - codecimage::NotifyDetached... would clear the unowned texture id from the
> > outgoing texture.
> > -- codecimage can also drop its strong ref to shared_state, if desired.
> > -- codecimage is inactive at this point.  attaching it to other textures
does
> > nothing.
> > - codecimage currently holds a raw ptr to the texture, which probably is
bad. 
> > Texture::Destroy should also call NotifyDetached, and NotifyDetached should
> > clear that raw ptr.
> > -- shouldn't be possible to attach a codec image to another Texture, but
it's
> > something we ignore right now.  it could cause a crash if somebody manages
to
> do
> > it.
> > -- this makes it safe to attach a codec image elsewhere, so that we can quit
> > worrying about it.
> > 
> > i was thinking about this recently.  once we have this, it's almost
> > straightforward to also include the codec itself in the shared state.  then,
> the
> > textures show the correct frames, rather than the most recent front buffer.
> 
> Nice. The first bit is implemented in this CL.
> 
> For the second bit, that sounds like it would work. Maybe we could avoid the
> GLImage notification if AVDACI could look up the Texture it was originally
> attached to via the client id (even if it's possible to attach the same image
to
> multiple textures we only need to reset unowned id on the original). That
would
> rely on the lookup returning NULL if the texture is being deleted (i.e., if
> we're running inside the Texture destructor), and I don't know if that's true.
> Crazy?

i'm not sure that will work.  the client id depends on the decoder. some other
decoder (e.g., browser compositor) might have a reference to the Texture via a
mailbox, whether the renderer still has a reference to it or not / decoder got
destroyed / etc.

plus, the client id might get reused and point to some other texture.  while
nothing too bad would happen, even if it was re-used as a PictureBuffer with its
own, different unowned texture, it's still a little weird.

i think that an explicit notification that doesn't go through the decoder is
going to be a lot more robust.

[watk, 2016-04-01 22:09:26]

On 2016/04/01 21:49:07, liberato wrote:
> On 2016/04/01 20:29:10, watk wrote:
> > On 2016/04/01 05:58:08, liberato wrote:
> > > On 2016/04/01 00:26:31, watk wrote:
> > > > Frank, Dan and I took a look at this and it seems like maybe this is
still
> > > > possible if we move the responsibility for calling
SetUnownedServiceId(0)
> to
> > > the
> > > > AVDACodecImage destructor. So if a different image is bound, the
> > > AVDACodecImage
> > > > destructor will reset the unowned id.
> > > > 
> > > > If the Texture is deleted, AVDACodecImage needs a way to detect that its
> > > Texture
> > > > was deleted to avoid calling SetUnownedServiceId, but other than that
> there
> > > seem
> > > > to be no issues.
> > > > 
> > > > Is this true? Or are there other subtleties that I've forgotten about?
It
> > > would
> > > > be really nice to get rid of the crash prone
> copySurfaceTextureToPictures..
> > > 
> > > i think it's slightly more complicated than that, but not too much.  one
> also
> > > needs something to own the SurfaceTexture and the gl texture that AVDA
> creates
> > > for it.
> > > 
> > > i think that you need something like this:
> > > 
> > > - shared state between AVDACodecImage that holds the ST and the service_id
> of
> > > the ST texture that AVDA created.
> > > -- this might be avda_shared_state, in fact.  not sure if it can be
re-used.
> 
> > > probably.
> > > - the shared state owns the ST texture.  in its destructor, it deletes it.
> > > - AVDACodecImage has a strong ref to the shared state
> > > -- keeps texture alive as long as any avdacodecimage / texture needs it.
> > > 
> > > to fix the 'texture was deleted' case:
> > > - a new call GLStreamTextureImage::NotifyDetachedFromTexture(Texture*)
> > > - Texture::SetLevelImage variants have to call NotifyDetachedFromTexture
if
> > the
> > > outgoing image is a stream texture
> > > -- this is trivial, since Texture already knows that it has a
> > > GLStreamTextureImage
> > > - codecimage::NotifyDetached... would clear the unowned texture id from
the
> > > outgoing texture.
> > > -- codecimage can also drop its strong ref to shared_state, if desired.
> > > -- codecimage is inactive at this point.  attaching it to other textures
> does
> > > nothing.
> > > - codecimage currently holds a raw ptr to the texture, which probably is
> bad. 
> > > Texture::Destroy should also call NotifyDetached, and NotifyDetached
should
> > > clear that raw ptr.
> > > -- shouldn't be possible to attach a codec image to another Texture, but
> it's
> > > something we ignore right now.  it could cause a crash if somebody manages
> to
> > do
> > > it.
> > > -- this makes it safe to attach a codec image elsewhere, so that we can
quit
> > > worrying about it.
> > > 
> > > i was thinking about this recently.  once we have this, it's almost
> > > straightforward to also include the codec itself in the shared state. 
then,
> > the
> > > textures show the correct frames, rather than the most recent front
buffer.
> > 
> > Nice. The first bit is implemented in this CL.
> > 
> > For the second bit, that sounds like it would work. Maybe we could avoid the
> > GLImage notification if AVDACI could look up the Texture it was originally
> > attached to via the client id (even if it's possible to attach the same
image
> to
> > multiple textures we only need to reset unowned id on the original). That
> would
> > rely on the lookup returning NULL if the texture is being deleted (i.e., if
> > we're running inside the Texture destructor), and I don't know if that's
true.
> > Crazy?
> 
> i'm not sure that will work.  the client id depends on the decoder. some other
> decoder (e.g., browser compositor) might have a reference to the Texture via a
> mailbox, whether the renderer still has a reference to it or not / decoder got
> destroyed / etc.

Hmm, good point. I don't know how the cross context sharing works. Dan suggested
using a mailbox to refer to the texture :).

> plus, the client id might get reused and point to some other texture.  while
> nothing too bad would happen, even if it was re-used as a PictureBuffer with
its
> own, different unowned texture, it's still a little weird.
>
> i think that an explicit notification that doesn't go through the decoder is
> going to be a lot more robust.