Closed Bug 1200595 Opened 4 years ago Closed 4 years ago

Separate the ownership and the backend logic of TextureClient

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 3 open bugs)

Details

Attachments

(12 files, 7 obsolete files)

19.68 KB, patch
mattwoodrow
: review+
sotaro
: review+
bas.schouten
: review+
Details | Diff | Splinter Review
65.87 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
89.82 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
65.79 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
28.49 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
6.76 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.06 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.25 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
19.22 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
10.63 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
44.71 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
53.93 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
The idea is to have a TextureData class that doesn't have any ownership semantic. It is uniquely owned by the TextureClient object which forwards backend-specific calls to the TextureData.

I want this to be able to:
* properly use the TextureClient's refcounting without horrible hacks when attempting to recycle textures
* simplify the deallocation code
* add the possibility to asynchronously deallocate textures on the client side.
* remove a lot of code duplication between TextureClient implementations

As a bonus, while working on this I noticed a bunch of duplicate functionalities inside TextureClient itself (like the Fence and SyncObject stuff), which I want to clean up in the process.
Summary: Separate the ownersip and the backend logic of TextureClient → Separate the ownership and the backend logic of TextureClient
Blocks: 1193468
I added a temporary ClientTexture inheriting from TextureClient but they'll get merged back when all texture clients are based on TextureData.
Attachment #8655416 - Flags: review?(matt.woodrow)
Attachment #8655417 - Flags: review?(matt.woodrow)
Attached patch Gralloc part (obsolete) — Splinter Review
Attachment #8655419 - Flags: review?(sotaro.ikeda.g)
Attachment #8655416 - Flags: review?(sotaro.ikeda.g)
Attachment #8655416 - Flags: review?(bas)
Comment on attachment 8655416 [details] [diff] [review]
add the TextureData interface and make the X11 implementation use it

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

Skimmed over the actual changes a bit since they all seem fairly mechanical.

I really like the concept :)

::: gfx/layers/client/TextureClient.h
@@ +576,5 @@
> +  virtual already_AddRefed<gfx::DrawTarget> BorrowDrawTarget() = 0;
> +
> +  virtual void Deallocate(ISurfaceAllocator* aAllocator) = 0;
> +
> +  virtual bool Serialize(SurfaceDescriptor& aDescrptor) = 0;

aDescriptor (typo).

@@ +586,5 @@
> +
> +  virtual bool UpdateFromSurface(gfx::SourceSurface* aSurface) = 0;
> +};
> +
> +class ClientTexture : public TextureClient {

This naming is fun :)

If it's going to survive for more than just this patch queue, then please add a comment explaining that it's a TextureClient impl that is backed by a TextureData object, and will go away when all TextureClients are that.
Attachment #8655416 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8655417 [details] [diff] [review]
Make BufferTextureClient use TextureData

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

::: gfx/layers/BufferTexture.cpp
@@ +88,5 @@
> +                          gfx::BackendType aMoz2DBackend, TextureFlags aFlags,
> +                          TextureAllocationFlags aAllocFlags,
> +                          ISurfaceAllocator* aAllocator)
> +{
> +  if (!aAllocator || aAllocator->IsSameProcess()) {

Do we want to support a nullptr allocator? The old code didn't.

@@ +152,5 @@
> +{
> +  switch (mFormat) {
> +    case gfx::SurfaceFormat::YUV:
> +    case gfx::SurfaceFormat::NV12:
> +    case gfx::SurfaceFormat::UNKNOWN:

Might be better to list the formats that we do support in case someone adds some other weird video one and forget to update this.

::: gfx/layers/BufferTexture.h
@@ +30,5 @@
> +  size_t mBufferSize;
> +  gfx::SurfaceFormat mFormat;
> +};
> +
> +class RawYCbCrTextureBuffer : public RefCounted<RawYCbCrTextureBuffer>

This appears to be unused?

::: gfx/layers/ipc/SharedRGBImage.cpp
@@ +96,2 @@
>    }
> +  return 0;

nullptr
Attachment #8655417 - Flags: review?(matt.woodrow) → review+
Attachment #8655416 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8655419 [details] [diff] [review]
Gralloc part

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

::: widget/gonk/nativewindow/GonkBufferQueueJB.cpp
@@ +426,5 @@
> +        ISurfaceAllocator* allocator = ImageBridgeChild::GetSingleton();
> +        GrallocTextureData* texData = GrallocTextureData::Create(IntSize(w,h),
> +                                                                 SurfaceFormatForPixelFormat(format),
> +                                                                 gfx::BackendType::NONE, usage,
> +                                                                 allocator);

Does it support to allocate chip dependent YUV format buffers allocation? 'format' seems not directly used for gralloc allocation.
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> Does it support to allocate chip dependent YUV format buffers allocation?
> 'format' seems not directly used for gralloc allocation.

Hm, actually I might be loosing some format information in the process here (if we have pixel formats that are not handled in SurfaceFormatForPizelFormat which seems to be the case). I'll update the patch to take android's pixel format instead of SurfaceFormat as input.
Attachment #8655419 - Flags: review?(sotaro.ikeda.g)
Attachment #8655419 - Attachment is obsolete: true
Attachment #8657869 - Flags: review?(sotaro.ikeda.g)
Attached patch (WIP) D3D11/D3D10 TextureData (obsolete) — Splinter Review
Attached patch (WIP) D3D9 TextureData (obsolete) — Splinter Review
Attached patch (WIP) TextureClient deallocation (obsolete) — Splinter Review
This patch moves all of the deallocation logic into a single function and adds the possibility to deallocation TextureClients on the client side without waiting for the compositor.
The function handles dispatching the deallocation to the IPDL thread, blocking or not blocking, and makes sure the TextureData is deallocated after the host is gone (so we don't need the KeepAlive thing).

There are a few little things to figure out, like the RecycleCallback. I would like to use an interface like TextureRecycler or whatnot, but in practice it would not be very different from what we do with the function pointer today (just a bit more flexible, and no more ClearRecycleCallback for the callback: you create a texture with a recycler and the latter decides whether to recycle the TextureData or not).
Attachment #8657869 - Flags: review?(sotaro.ikeda.g) → review+
A bug that took me a while to figure out was that BufferTextureClient exposes pointers to its raw buffer (with some metadata at the beginning of the buffer) whereas most users of BufferTextureClient really only want to get the address of the pixels themselves and I had a place where I'd give a pointer to the beginning of the buffer instead of the beginning of the pixels. This patch is mosty identical to the one that got r+ with the notable exception that TextureClient can now expose address to pixels using the method bool BorrowMappedData(MappedTextureData& descriptor) and its YCbCr equivalent, that provide access to the pixels independently of how they are stored in the buffer instead of exposing the buffer and letting the called figure out how do read from it. As a bonus, this will allow the gralloc texture to expose its pixels with the same interface which it couldn't do previously. I think that there are some software video decoding code paths that could remove an extra copy by using gralloc instead of BufferTextureClient thanks to this (need to check). 

Anyway, re-asking for a review because of the code changes related to BorrowMappedData.
Attachment #8655417 - Attachment is obsolete: true
Attachment #8678883 - Flags: review?(matt.woodrow)
Attachment #8678884 - Flags: review?(bas)
Attached patch TextureData: EGLSplinter Review
Attachment #8657870 - Attachment is obsolete: true
Attachment #8657871 - Attachment is obsolete: true
Attachment #8678885 - Flags: review?(bas)
Attachment #8678887 - Flags: review?(matt.woodrow)
Attachment #8678891 - Flags: review?(matt.woodrow)
Attachment #8678893 - Flags: review?(matt.woodrow)
Comment on attachment 8678883 [details] [diff] [review]
TextureData: BufferTextureClient + improve the way we expose pointers to the texture's buffer

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

I'm trusting that most of the new BufferTextureData code is just copied from BufferTextureClient, I didn't check it line by line.

::: gfx/layers/client/TextureClient.cpp
@@ +345,5 @@
> +{
> +  MOZ_ASSERT(mValid);
> +
> +  // TODO - SharedRGBImage just accesses the buffer without properly locking
> +  // the texture. It's bad.

File a followup to fix this?
Attachment #8678883 - Flags: review?(matt.woodrow) → review+
Attachment #8678887 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8678891 [details] [diff] [review]
TextureData: android surface

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

::: gfx/layers/client/TextureClient.cpp
@@ +939,5 @@
>        uint8_t* dst = aDst.data + i * aDst.stride;
>        for (int32_t j = 0; j < size.width; ++j) {
>          *dst = *src;
>          src += 1 + skip;
> +        dst += 1 + aDst.skip;

I assume you'll fix this in the original patch, not here.

::: gfx/layers/opengl/TextureClientOGL.cpp
@@ +79,5 @@
> +  if (!aSurfTex || !XRE_IsParentProcess()) {
> +    return nullptr;
> +  }
> +
> +  // XXX - This is quite sad and slow.

Can you elaborate on this? (in the previous patch too). I doubt this is useful as it stands (except to you).
Attachment #8678891 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8678893 [details] [diff] [review]
TextureData: MacIOSurface

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

::: gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "MacIOSurfaceTextureClientOGL.h"
> +#include "mozilla/gfx/MacIOSurface.h" 

Whitespace.
Attachment #8678893 - Flags: review?(matt.woodrow) → review+
Attached patch TextureData: DIBSplinter Review
Attachment #8683229 - Flags: review?(bas)
SharedSurfaceTextureClient was trickier to convert because a lot of the SharedSurface code poke at the SharedTextureClient's internals, so I kept a special TextureClient class for it which is a bit awkward, but the patch remains reasonably small and we still have a dedicated TextureData object which is what I need to finally fix the destruction sequence and recycling logic.

The interaction between TextureClient and SharedSurface is still full of surprises and it'll eventually need to be improved in a way that doesn't require synchronous deallocation and where the data is really owned by the TextureClient. so I still consider it in a temporary state and there's plenty more to do in this area.
Attachment #8683668 - Flags: review?(jgilbert)
Comment on attachment 8683668 [details] [diff] [review]
TextureData: SharedSurface

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

::: gfx/layers/client/TextureClientSharedSurface.h
@@ +55,5 @@
> +  virtual bool Serialize(SurfaceDescriptor& aOutDescriptor) override;
> +
> +  virtual void Deallocate(ISurfaceAllocator*) override;
> +
> +  gl::SharedSurface* Surf() const { return mSurf.get(); }

Why is this added? mSurf is const, so it is its own getter. No need to add a useless function.
Attachment #8683668 - Flags: review?(jgilbert) → review+
There we go, the awkward ClientTexture class is gone.
Attachment #8684974 - Flags: review?(matt.woodrow)
This is one of the big wins from this patch queue. TextureClient's deallocation code used to be spread in different places including some duplicated parts in ShadowLayers.cpp and ImageBridgeChiold.cpp. Now all of the logic goes into one function which is much clearer. In addition, Textures can be deallocated on the client side asynchronously which was not possible before that and that will let us remove some really nasty and slow synchronous deallocation cases. And also we don't need the KeepUntilFullDeallocation stuff and other refcounting tricks anymore, because all of the TextureData objects are kept until TextureChild::ActorDestroy is called.
Attachment #8657875 - Attachment is obsolete: true
Attachment #8684979 - Flags: review?(matt.woodrow)
The previous version had a silly issue with synchronous deallocation.
Attachment #8684979 - Attachment is obsolete: true
Attachment #8684979 - Flags: review?(matt.woodrow)
Bas, in case that eases the reviews, the first patch (the one that introduces the ClientTexture class) adds some amount of ugliness which is removed in the last patch of the series. I had to do that to keep things working until all texture types were converted, but it looks good once all patches are in place.
The only thing to worry about is whether the XYZTextureData type converted from TextureClientXYZ has the same behavior.
Separating TextureClient and TextureData into several objects has several benefits, some of which are coming from the fact that the TextureData needs to stay alive for the entire deallocation handshake between the TextureChild and TextureParent, while the TextureHost object is destroyed earlier at the beginning of the handshake when its reference count reaches zero. It also reduces the amount of duplicated code since TextureClient takes care of checking inputs/states and the TextureData just implements the backend bits, but fixing the complexity and awfulness of how we currently destroy textures is the strongest motivator here.
Comment on attachment 8684974 [details] [diff] [review]
Merge back TextureClient and ClientTexture

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

\o/
Attachment #8684974 - Flags: review?(matt.woodrow) → review+
Review ping. The patchy queue is looking good on try and rebasing it is a long a risky process because of the amount of changes involved.
Comment on attachment 8655416 [details] [diff] [review]
add the TextureData interface and make the X11 implementation use it

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

::: gfx/layers/client/TextureClient.cpp
@@ +209,5 @@
> +        RefPtr<DataSourceSurface> dataSurf = snapshot->GetDataSurface();
> +        mReadbackSink->ProcessReadback(dataSurf);
> +      }
> +    }
> +    mBorrowedDrawTarget = nullptr;

Currently we keep the DT around in a lot of cases even when the texture is unlocked I think.. are we sure we want to null this? Creating the DT can have its cost. Although I guess we can have mData decide to hold on to it or now.

::: gfx/layers/client/TextureClient.h
@@ +588,5 @@
> +};
> +
> +class ClientTexture : public TextureClient {
> +public:
> +  ClientTexture(TextureData* aData, TextureFlags aFlags, ISurfaceAllocator* aAllocator)

Since you're deleting the object pointed to by aData here in the destructor be sure to comment on the transfer of ownership and perhaps even use some C++ magic to add safety.

@@ +616,5 @@
> +
> +  virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aOutDescriptor) override;
> +
> +  already_AddRefed<TextureClient>
> +  CreateSimilar(TextureFlags aFlags = TextureFlags::DEFAULT,

nit: indent
Attachment #8655416 - Flags: review?(bas) → review+
Comment on attachment 8678885 [details] [diff] [review]
TextureData: D3D9 implementation

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +538,4 @@
>      return nullptr;
>    }
>  
> +  RefPtr<DrawTarget> dt;

It worries me that we'll now longer be caching this through lock/unlock pairs.
Attachment #8678885 - Flags: review?(bas) → review+
Comment on attachment 8678884 [details] [diff] [review]
TextureData: D3D11 implementation

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

I'm assuming this is mostly code moving around, I haven't reviewed the bodies of a bunch of actual functions in detail, if this was a mistake let me know.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +680,5 @@
>  }
>  
> +already_AddRefed<DrawTarget>
> +D3D11TextureData::BorrowDrawTarget()
> +{

It's good to see we're continuing to cache this one since we definitely have to do so here.
Attachment #8678884 - Flags: review?(bas) → review+
Attachment #8683229 - Flags: review?(bas) → review+
I tried to keep caching the DT in the places we used to but I'll double check and make sure we keep around the D3D9 one if we did before the patch.

I have a patch in the making to use UniquePtr with TextureData (should have done that in the first place)
Woops, I forgot to set the review flag for this one.

A bit too much got folded into this patch, sorry, but basically it consolidates the destruction logic in one place that is easier to understand and cover all corner cases (there are more of them than I expected). It also cleans up a whole bunch of stuff that was made useless by this patch and the previous ones, and contains the various fixes I needed to get things green on try.
Attachment #8685041 - Attachment is obsolete: true
Attachment #8689130 - Flags: review?(matt.woodrow)
Comment on attachment 8689130 [details] [diff] [review]
Consolidate the texture deallocation code + fixes

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

::: gfx/2d/2D.h
@@ +1132,5 @@
>  {
>  public:
> +  struct Config {
> +    int32_t mMaxTextureSize;
> +    int32_t mMaxAllocSize;

What is this added for? I don't see it being used.

::: gfx/layers/client/TextureClient.cpp
@@ +225,5 @@
> +///
> +/// This funciton takes care of dispatching work to the right thread using
> +/// a synchronous proxy if needed, and handles client/host deallocation.
> +void
> +TextureClientDeallocation(TextureDeallocParams params)

DoTextureClientDeallocation, or DeallocateTextureClient.
Attachment #8689130 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #35)
> What is this added for? I don't see it being used.

Ah, darnit. That's me working on two patch sets at the same time. This has nothing to do here.

> DoTextureClientDeallocation, or DeallocateTextureClient.

ok
(In reply to Pulsebot from comment #37)

sorry had to back this out since something seems have caused https://treeherder.mozilla.org/logviewer.html#?job_id=17609171&repo=mozilla-inbound here :(
Flags: needinfo?(nical.bugzilla)
Duplicate of this bug: 1039535
Blocks: 972444
Blocks: 1202394
It's possible this also caused an intermittent failure in web-platform-tests(1) like https://treeherder.mozilla.org/logviewer.html#?job_id=17751633&repo=mozilla-inbound but I'm less sure about that one.
Hello, this has caused build bustage on B2G emulator builds (as of now). Can you please look urgently?
Flags: needinfo?(nical.bugzilla)
Hi Nical,

Recently I'd working on 2d canvas in Worker thread, which the bug id is Bug 801176. It aims at operating 2d canvas in Worker thread. It seems that your recently patch landing in this bug changes behaviors with my working patch.

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#481 

You put a check mechanism to make sure this function runs in main thread. May I know your concern if it runs in Worker? Really thanks for your comment. :)
Flags: needinfo?(nical.silva)
(In reply to Vincent Liu[:vliu] from comment #50)
> Hi Nical,
> 
> Recently I'd working on 2d canvas in Worker thread, which the bug id is Bug
> 801176. It aims at operating 2d canvas in Worker thread. It seems that your
> recently patch landing in this bug changes behaviors with my working patch.
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TextureClient.cpp#481 
> 
> You put a check mechanism to make sure this function runs in main thread.
> May I know your concern if it runs in Worker? Really thanks for your
> comment. :)
Hi,

The video pipeline needed to update a TextureClient off the main thread and we had to add the TextureClient::UpdateFromSurface in order to do that. I think that this is because of a restriction with the D2D backend. I guess that we would be fine doing this with the cario-image backend but probably not with skia, and probably not with cairo-xlib either, I don't know for core graphics (Bas, probably knows more about the threading restrictions involved here with the different backends).

If you only need to copy the content of the canvas into the TextureClient, then you can use UpdateFromSurface. If the canvas draws directly into the TextureClient, then we need to figure something out (and probably restrict to cairo-image, and even with this, we'll run into issues rendering text (can't do that in parallel with our version of cairo, right now).
Flags: needinfo?(nical.silva) → needinfo?(bas)
nical, jerry found that attachment 8657869 [details] [diff] [review] remove WaitForBufferOwnership(). It is necessary to wait the case when fence is not delivered to client side. It could cause a regression.
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #51)
> If you only need to copy the content of the canvas into the TextureClient,
> then you can use UpdateFromSurface. If the canvas draws directly into the
> TextureClient, then we need to figure something out (and probably restrict
> to cairo-image, and even with this, we'll run into issues rendering text
> (can't do that in parallel with our version of cairo, right now).

Please correct me if I got anything wrong. As your suggestion, I need to get SourceSurface as argument before calling UpdateFromSurface. To get it, I need to have DrawTarget. The thing is I can't get DrawTarget when calling BorrowDrawTarget. If so, could you please have more suggestions for the problem I met? Thanks.
Flags: needinfo?(nical.silva)
(In reply to Vincent Liu[:vliu] from comment #53)
> To get it, I need to have DrawTarget.

You don't need a DrawTarget to get a SourceSurface, although that's one way to do it.
If the data you want to copy into the TextureClient is on the cpu, you are probably better off wrapping a temporary SourceSurface around with gfx::Factory::CreateWrappingDataSourceSurface.
What kind of backend are you dealing with off the main thread? is it exclusively cpu backends (cairo-image and skia) or can it also be a gpu backend like D2D?
Flags: needinfo?(nical.silva)
Flags: needinfo?(bas)
Depends on: 1421187
You need to log in before you can comment on or make changes to this bug.