916118 - Simplify the deallocation protocol of TextureClient/TextureHost

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

We support the following 3 combinations in TextureFlags:

1) TEXTURE_DEALLOCATE_HOST: deallocate on the host. no synchronization needed.
2) TEXTURE_DEALLOCATE_CLIENT: deallocate on the client, need the client to wait for the host to notify that it is safe to deallocate.
3) none of the two bits are set: Equivalent to 2) but the data is externally own and layers should not try to do the deallocation (just notify the owner that the data is safe to destroy)


3) was meant to be used (at least) for the b2g h264 path where buffers are owned by the GonkNativeWindow.

It turns out that we implemented 2) and 3) the same way, so we don't really need a distinction between 2) and 3) in the texture flags.

So we can make it simpler by just having TEXTURE_DEALLOCATE_CLIENT for 2) and 3) and when the bit is not set we deallocate on the host (default behavior).

This should help avoid confusion and bugs caused by flags not properly set. For instance, if the texture flags is incidentally 0, we deallocate on the host side rather than leaking.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Remove the flag TEXTURE_DEALLOCATE_HOST

Comment 2

[Nicolas Silva [:nical]]

try push https://tbpl.mozilla.org/?tree=Try&rev=d1c4b2b665ec

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Did we need to do this simplification now?  From the comment 0, everything was working correctly, though it may have been confusing.  This is a lot of changes to just avoid confusion on the last day of the train?  Can this wait?

Comment 4

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Did we need to do this simplification now?  From the comment 0, everything
> was working correctly, though it may have been confusing.  This is a lot of
> changes to just avoid confusion on the last day of the train?  Can this wait?

It definitely can wait. I did this because it took half an hour and I needed to rest my brain on an easy task.

Comment 5

[Nick Cameron [:nrc]]

Comment on attachment 804471 [details] [diff] [review]
Remove the flag TEXTURE_DEALLOCATE_HOST

Review of attachment 804471 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this took so long, it slipped off my TODO list and I didn't notice till I got the bugzilla reminder. r=me with the following nits fixed

::: gfx/layers/CompositorTypes.h
@@ +60,5 @@
>  // from the previous texture.
>  const TextureFlags TEXTURE_COPY_PREVIOUS      = 1 << 24;
>  // Who is responsible for deallocating the shared data.
> +// if TEXTURE_DEALLOCATE_CLIENT is set, the shared data is deallocated on the
> +// client side and requires some extra synchronizaion to ensure reace-free

*race

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +104,3 @@
>        // GLContext's SurfaceStream handles ownership itself,
>        // and doesn't require layers to do any deallocation.
> +      flags |= TEXTURE_DEALLOCATE_CLIENT;

Is this really deallocate client? Do we need a flag for nobody has to dellocate?

::: gfx/layers/client/ImageClient.cpp
@@ +164,5 @@
>      }
>  
>      bool bufferCreated = false;
>      if (!mFrontBuffer) {
> +      mFrontBuffer = CreateBufferTextureClient(gfx::FORMAT_YUV, TEXTURE_FLAGS_DEFAULT);

No need for flags param

@@ +225,5 @@
>      if (!mFrontBuffer) {
>        gfxASurface::gfxImageFormat format
>          = gfxPlatform::GetPlatform()->OptimalFormatForContent(surface->GetContentType());
>        mFrontBuffer = CreateBufferTextureClient(gfx::ImageFormatToSurfaceFormat(format),
> +                                               TEXTURE_FLAGS_DEFAULT);

No need for flags param

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +127,1 @@
>      // to be synchronous.

move the comment too

::: gfx/layers/opengl/TextureClientOGL.cpp
@@ +21,5 @@
>    , mHandle(0)
>    , mInverted(false)
>  {
> +  MOZ_ASSERT(aFlags & TEXTURE_DEALLOCATE_CLIENT,
> +             "SharedTextureClientOGL is alwas owned externally");

*always

Comment 6

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #5)
> Comment on attachment 804471 [details] [diff] [review]
> Remove the flag TEXTURE_DEALLOCATE_HOST
> 
> Review of attachment 804471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry this took so long, it slipped off my TODO list and I didn't notice
> till I got the bugzilla reminder. r=me with the following nits fixed
> 

No problem, this is at the bottom of my priorities

Comment 7

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #5)
> Comment on attachment 804471 [details] [diff] [review]
> Remove the flag TEXTURE_DEALLOCATE_HOST
> ::: gfx/layers/client/ClientCanvasLayer.cpp
> @@ +104,3 @@
> >        // GLContext's SurfaceStream handles ownership itself,
> >        // and doesn't require layers to do any deallocation.
> > +      flags |= TEXTURE_DEALLOCATE_CLIENT;
> 
> Is this really deallocate client? Do we need a flag for nobody has to
> dellocate?

Yes, the way client-side deallocation works is that when we destroy the texture client, we make it drop its shared data on the form of a TextureClientData object. The latter will be kept in a map until a reply from the compositor is received ensuring that it is safe to deallocate things. the TextureClientData is then asked to deallocate the data. For SharedTextureClientOGL the corresponding textureClientData just doesn't do anything because the shared texture is owned externally, but it's rather bad because right now the external system has no way to know when it can reuse or delete the texture. We need to implement it properly at some point.

For gralloc, the texture client knows whether it is owned externally or have to notify an external system (GonkNativeWindow), so it will give a different TextureClientData depending on the case. My attempt at handling this at the TextureClient/Host level ended up just making things confusing (because of the potentially contradicting flags combinations) while the gralloc implem implemented this logic at the lower level.

So now the flag just informs the system about how much synchronization must be done to safely deallocate or reuse buffers, and the texture client implementation is responsible for deallocating or doing what the potential external system expects.

Comment 8

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bafb8bf3b279

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX asserts/crashes and reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e766f9da9b33

https://tbpl.mozilla.org/php/getParsedLog.php?id=28412456&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28415552&tree=Mozilla-Inbound

Comment 10

[Ryan VanderMeulen [:RyanVM]]

And some B2G reftest failures. Probably better to just do a full Try run before pushing again.

Comment 11

[Nicolas Silva [:nical]]

try https://tbpl.mozilla.org/?tree=Try&rev=067d681a6e60

Comment 12

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/477781ec7d38

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/477781ec7d38