Simplify the deallocation protocol of TextureClient/TextureHost

RESOLVED FIXED in mozilla27

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 804471 [details] [diff] [review]
Remove the flag TEXTURE_DEALLOCATE_HOST
Attachment #804471 - Flags: review?(ncameron)
(Assignee)

Comment 2

4 years ago
try push https://tbpl.mozilla.org/?tree=Try&rev=d1c4b2b665ec
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?
(Assignee)

Comment 4

4 years ago
(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

4 years ago
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
Attachment #804471 - Flags: review?(ncameron) → review+
(Assignee)

Comment 6

4 years ago
(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
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 919022
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bafb8bf3b279
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
And some B2G reftest failures. Probably better to just do a full Try run before pushing again.
(Assignee)

Comment 11

4 years ago
try https://tbpl.mozilla.org/?tree=Try&rev=067d681a6e60
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/477781ec7d38
https://hg.mozilla.org/mozilla-central/rev/477781ec7d38
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

4 years ago
Depends on: 970002
You need to log in before you can comment on or make changes to this bug.