Make TextureHost recycling implicit

RESOLVED FIXED in Firefox 50

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(5 attachments, 60 obsolete attachments)

4.65 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
171.56 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
11.83 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
22.31 KB, patch
nical
: review+
Details | Diff | Splinter Review
11.97 KB, patch
nical
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
TexureHost recycling uses explicit mechanism. It makes code annoying. It should be recycled by an implicit way.
(Assignee)

Updated

2 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 1

2 years ago
Created attachment 8726507 [details] [diff] [review]
patch - Make TextureHost recycling implicit
(Assignee)

Updated

2 years ago
Depends on: 1253478
(Assignee)

Updated

2 years ago
Depends on: 1253481
(Assignee)

Updated

2 years ago
Depends on: 1253489
(Assignee)

Comment 2

2 years ago
Created attachment 8726582 [details] [diff] [review]
patch - Make TextureHost recycling implicit
Attachment #8726507 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
See Also: → bug 1227718
(Assignee)

Comment 3

2 years ago
Created attachment 8731549 [details] [diff] [review]
patch - Make TextureHost recycling implicit
Attachment #8726582 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Created attachment 8731549 [details] [diff] [review]
> patch - Make TextureHost recycling implicit

rebased.
(Assignee)

Comment 5

2 years ago
Created attachment 8731556 [details] [diff] [review]
patch - Make TextureHost recycling implicit
Attachment #8731549 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
The patch seems to work basically, but it seems to cause TileLayer's TextureCliets waits longer.
(Assignee)

Comment 7

2 years ago
Created attachment 8732047 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Address Tiled Layers's problem.
Attachment #8731556 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8731556 - Attachment is obsolete: false
(Assignee)

Updated

2 years ago
Attachment #8732047 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
Created attachment 8733684 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Rebased.
Attachment #8731556 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1254006
(Assignee)

Comment 9

2 years ago
Created attachment 8733788 [details] [diff] [review]
patch - Make TextureHost recycling implicit
Attachment #8733684 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Created attachment 8733794 [details] [diff] [review]
patch - Make TextureHost recycling implicit
Attachment #8733788 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8733858 [details] [diff] [review]
patch - Make TextureHost recycling implicit
Attachment #8733794 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Created attachment 8734205 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Remove an unnecessary member.
Attachment #8733858 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8734223 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Code clean up.
Attachment #8734205 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
During testing attachment 8734223 [details] [diff] [review], I saw a problem during video playback on b2g aries. Sometimes video did not resume. From investigation, the problem is not related to attachment 8734223 [details] [diff] [review]. It is a problem of media framework, VideoSink holds too many video buffers and android::MediaCodec's out port becomes out of buffers.
(Assignee)

Updated

2 years ago
Depends on: 1259366
(Assignee)

Comment 15

2 years ago
Created attachment 8734597 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Rebased.
Attachment #8734223 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
Created attachment 8736242 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Rebase and fixed tiled layer problems on gonk.
Attachment #8734597 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1253062
(Assignee)

Comment 17

2 years ago
Created attachment 8736595 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Code clean up.
Attachment #8736242 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
attachment 8736595 [details] [diff] [review] still has a problem of tiled layer on gonk.
(Assignee)

Comment 19

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> attachment 8736595 [details] [diff] [review] still has a problem of tiled
> layer on gonk.

It seems to be caused by TileClient::DiscardFrontBuffer(). Assumption of the function seems to be broken during progressive update. TextureClient was returned to TextureClientPool even when TextureClient is still used on host side.
(Assignee)

Comment 20

2 years ago
Created attachment 8737030 [details] [diff] [review]
patch part 2 - Make TextureHost recycling implicit

Use TextureClient recycle callback in TextureClientPool.
(Assignee)

Comment 21

2 years ago
Created attachment 8737105 [details] [diff] [review]
patch part 2 - Use recycle callback in TextureClientPool

Fixed recycling problems.
Attachment #8737030 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
Created attachment 8737108 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Update nits.
Attachment #8736595 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
Created attachment 8737115 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737108 - Attachment is obsolete: true
(Assignee)

Comment 24

2 years ago
Created attachment 8737451 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737115 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
Created attachment 8737454 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737451 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8737105 - Attachment description: patch part 2 - Make TextureHost recycling implicit → patch part 2 - Use recycle callback in TextureClientPool
(Assignee)

Comment 26

2 years ago
Created attachment 8737468 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737454 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
Created attachment 8737475 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737468 - Attachment is obsolete: true
(Assignee)

Comment 28

2 years ago
Created attachment 8737478 [details] [diff] [review]
patch part 2 - Use recycle callback in TextureClientPool
Attachment #8737105 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
Created attachment 8737491 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737475 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
Created attachment 8737496 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737491 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dc631b6bc9e
(Assignee)

Comment 32

2 years ago
Created attachment 8737502 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737496 - Attachment is obsolete: true
(Assignee)

Comment 33

2 years ago
Created attachment 8737503 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737502 - Attachment is obsolete: true
(Assignee)

Comment 34

2 years ago
Created attachment 8737560 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737503 - Attachment is obsolete: true
(Assignee)

Comment 35

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c9baec83d78
(Assignee)

Comment 36

2 years ago
Comment on attachment 8737560 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

nical, can you feedback to the patch?
Attachment #8737560 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 37

2 years ago
attachment 8737560 [details] [diff] [review] changes a way of recycling between TextureClient and TextureHost. When TextureFlags::RECYCLE is set to TextureClient, TextureClient is expected to be recycled via recycle callback. Recycleing of TextureHost is automatically cared when the TextureFlags::RECYCLE is set.

With the patch applied, usage of AsyncTransactionTrackersHolder becomes minimized. Only one AsyncTransactionTrackersHolder exists per one ImageBridgeChild. It is used only to handle ImageBridgeChild::FlushAllImages(). 

Recycleing of TextureHost is checked by using CompositableRef + "use texture generation" by ShadowLayerForwarder and ImageBridgeChild. They hold TextureClient ref until they receive "host side usage end" or their shutdown. When CompositableRef becomes 0, TextureHost is thought that its usage is ended. The CompositableRef and TextureHost's "use texture generation" is notified to TextureClient. The "use texture generation" is used to care async remote operation between client and host. It is set by ShadowLayerForwarder and ImageBridgeChild.

On gonk, we also need to care about a recycling without recycle callback to deliver fence handle. We continue to support it until all layers transaction becomes async.
(Assignee)

Comment 38

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #37)
> The CompositableRef and TextureHost's "use texture
> generation" is notified to TextureClient.

Correction:
 The generation is notified to ShadowLayerForwarder or ImageBridgeChild and they decide if the host side usage is ended.
Comment on attachment 8737560 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

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

Still going through the patch, I am slowly getting a better image of the recycling mechanism but I haven't connected all of the pieces in my head yet. More comments would help (not only for the reviews/feedbacks, but in general). So far I like what I see.

::: gfx/layers/client/TextureClient.cpp
@@ +196,5 @@
>      mTextureData = nullptr;
>    }
>  }
>  
> +/* static */ Atomic<uint64_t> TextureClient::sSerialCounter(0);

nit: A comment explaining what this serial counter is used for would be useful.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +230,5 @@
>        NS_ASSERTION(tiledHost, "The compositable is not tiled");
>  
>        const SurfaceDescriptorTiles& tileDesc = op.tileLayerDescriptor();
> +
> +      // Set generation to TextureHost and enaure TextureHost::ReleaseCompositableRef() is called.

nit: s/enaure/ensure/
Comment on attachment 8737560 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

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

It took me a while to get an idea of what the UseTextureGeneration (which is passed around everywhere in this patch) is useful for.
I think that I would prefer a TransactionId (incremented every transaction) rather than a UseTextureGeneration. I think both give you the same type information you need here, which is "what was the last moment the texture was used, and is this OpNotifyCompositableRefReleased corresponding to the same moment or is it out of date". The UseTextureGeneration gives you a finer granularity of "moment" but I think that a transaction-level granularity is enough for your purpose (I might be wrong, let me know). The advantage of the Transaction id is that it's easier to understand than "UseTexture" (I think), and I have completely unrelated plans that would make use of an incremented TransactionId, so it would be more useful for other purposes as well.

What do you think?

::: gfx/layers/ipc/CompositableForwarder.h
@@ +177,5 @@
>    {
>      return mTextureFactoryIdentifier;
>    }
>  
> +  uint64_t GetUseTextureGeneration() { return ++mUseTextureGeneration; }

It's a bit counter-intuitive that a method that looks like a Getter will return a different value every time. How about GetNewUseTextureGeneration() to make it clear that it's here that we advance the generation counter?
(Assignee)

Comment 41

2 years ago
(In reply to Nicolas Silva [:nical] from comment #39)
> Comment on attachment 8737560 [details] [diff] [review]
> patch part 1 - Make TextureHost recycling implicit
> 
> Review of attachment 8737560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still going through the patch, I am slowly getting a better image of the
> recycling mechanism but I haven't connected all of the pieces in my head
> yet. More comments would help (not only for the reviews/feedbacks, but in
> general). So far I like what I see.
> 
> ::: gfx/layers/client/TextureClient.cpp
> @@ +196,5 @@
> >      mTextureData = nullptr;
> >    }
> >  }
> >  
> > +/* static */ Atomic<uint64_t> TextureClient::sSerialCounter(0);
> 
> nit: A comment explaining what this serial counter is used for would be
> useful.

I am going to add a comment in a next patch.

> 
> ::: gfx/layers/ipc/CompositableTransactionParent.cpp
> @@ +230,5 @@
> >        NS_ASSERTION(tiledHost, "The compositable is not tiled");
> >  
> >        const SurfaceDescriptorTiles& tileDesc = op.tileLayerDescriptor();
> > +
> > +      // Set generation to TextureHost and enaure TextureHost::ReleaseCompositableRef() is called.
> 
> nit: s/enaure/ensure/

I'll update in a next patch.
(Assignee)

Comment 42

2 years ago
(In reply to Nicolas Silva [:nical] from comment #40)
> Comment on attachment 8737560 [details] [diff] [review]
> patch part 1 - Make TextureHost recycling implicit
> 
> Review of attachment 8737560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It took me a while to get an idea of what the UseTextureGeneration (which is
> passed around everywhere in this patch) is useful for.
> I think that I would prefer a TransactionId (incremented every transaction)
> rather than a UseTextureGeneration. I think both give you the same type
> information you need here, which is "what was the last moment the texture
> was used, and is this OpNotifyCompositableRefReleased corresponding to the
> same moment or is it out of date". The UseTextureGeneration gives you a
> finer granularity of "moment" but I think that a transaction-level
> granularity is enough for your purpose (I might be wrong, let me know). The
> advantage of the Transaction id is that it's easier to understand than
> "UseTexture" (I think), and I have completely unrelated plans that would
> make use of an incremented TransactionId, so it would be more useful for
> other purposes as well.
> 
> What do you think?

Yea, it seems to work. I am going to update in a next patch.

> 
> ::: gfx/layers/ipc/CompositableForwarder.h
> @@ +177,5 @@
> >    {
> >      return mTextureFactoryIdentifier;
> >    }
> >  
> > +  uint64_t GetUseTextureGeneration() { return ++mUseTextureGeneration; }
> 
> It's a bit counter-intuitive that a method that looks like a Getter will
> return a different value every time. How about GetNewUseTextureGeneration()
> to make it clear that it's here that we advance the generation counter?

I am going to replace UseTextureGeneration with TransactionId.
I am waiting for the next version of the patch before going for another round of feedbacks, if that's okay with you.

Updated

2 years ago
Attachment #8737560 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 44

2 years ago
Created attachment 8740835 [details] [diff] [review]
patch part 2 - Use recycle callback in TextureClientPool

Rebased.
Attachment #8737478 - Attachment is obsolete: true
(Assignee)

Comment 45

2 years ago
Created attachment 8741199 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit
Attachment #8737560 - Attachment is obsolete: true
(Assignee)

Comment 46

2 years ago
There is other transaction id(by refresh driver). Then the id was named FwdTransactionId.
(Assignee)

Updated

2 years ago
Attachment #8741199 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8741199 - Flags: review?(nical.bugzilla) → feedback?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8741199 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 47

2 years ago
Created attachment 8741207 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Update nits.
Attachment #8741199 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8741207 - Flags: feedback?(nical.bugzilla)
Blocks: 1260611
(Assignee)

Comment 48

2 years ago
:nical, about when can you feedback to the patch? Thanks.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8741207 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

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

Sorry, I am being pickier than usual on the terminology. This is because the whole thing is pretty complicated and even after several rounds of feedbacks I am still forgetting stuff and having trouble understanding some parts. So I think it's important to compensate for the complexity by being very explicit in the naming and documentation. I'll probably have more nits about that.

As far as the logic goes I feel mostly good about this patch (just would like some clarification on the AutoCompositableAddRef business).

::: gfx/layers/client/TextureClient.h
@@ +588,5 @@
> +  void WaitFenceHandleOnImageBridge(Mutex& aMutex);
> +  void ClearWaitFenceHandleOnImageBridge(Mutex& aMutex);
> +  void CancelWaitFenceHandleOnImageBridge();
> +
> +  void UpdateFwdTransactionId(uint64_t aTransactionId)

nit: SetLastFwdTransactionId would be a better name because you already use UpdateFwdTransactionId to generate the ids which is different, and also it would better document what this is about: marking on the TextureClient the id of last transaction it has been used in.
Generally, I find that having the type of the parameter in the name of the function can be useful, but not as much as having the reason we us the parameter in the name (I say that because I also have the tendency to name things like "SetFoo(Foo*)" rather than think of a more meaningful names).

::: gfx/layers/composite/TextureHost.h
@@ +40,5 @@
>  
>  class BufferDescriptor;
>  class Compositor;
>  class CompositableParentManager;
> +class GrallocTextureHostOGL;

nit: is this needed?

@@ +487,5 @@
>     * Get the TextureHost corresponding to the actor passed in parameter.
>     */
>    static TextureHost* AsTextureHost(PTextureParent* actor);
>  
> +  static uint64_t AsTextureSerial(PTextureParent* actor);

nit: "As" implies that the two representations overlay the same thing, which is sort of OK for TextureHost and its IPDL actor, but doesn't make as much sense for a texture and an integer id (which doesn't represent the texture itself but the last transaction it was used in)

GetTextureSerial would be better I think.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +69,5 @@
>        !aPictureRect.IsEmpty();
>  }
>  #endif
>  
> +class MOZ_STACK_CLASS AutoTilesCompositableAddRef

Could you give a bit more detail about why we need this?

setting the FwdTransactionId could be done in one of the loops in UseTiles, and It's not entirely clear to me why we need to AddCompositableRef and ReleaseCompositableRef for all textures when receiving the transaction.
Is it because a texture may end up never being AddCompositableRef'ed because of whatever failure case and that would cause the child side to wait forever?
If that's so the relationship between the CompositableRef and the child side deadlocking isn't explicit and I fear that we'll eventually break this guarantee without knowing.
Having a method that gets called on the texture whenever it fails that does the signaling to the child explicitly would be easier to understand, although off hands I don't have a precise idea of how to make something like this explicit in the API in a safe way. I am not sure that's the actual problem this is solving.
At least a detailed comment would help a lot.

::: gfx/layers/ipc/CompositableTransactionParent.h
@@ +32,5 @@
>  
>    virtual void SendAsyncMessage(const InfallibleTArray<AsyncParentMessageData>& aMessage) = 0;
>  
> +  virtual void NotifyCompositableRefReleased(PTextureParent* aTexture,
> +                                             uint64_t aTransactionId) = 0;

How about "NotifiyNotUsed" instead? Sounds like that name would better discribe the intent (which is to notify that texture is no longer used by the compositor, rather than the symptom (the ReleaseCompositableRef was called down to a ref count of 0).
Attachment #8741207 - Flags: feedback?(nical.bugzilla) → feedback+

Updated

2 years ago
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 50

2 years ago
Comment on attachment 8740835 [details] [diff] [review]
patch part 2 - Use recycle callback in TextureClientPool

Recycling problem should be addressed by bug 1265873
Attachment #8740835 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Depends on: 1265873
(Assignee)

Comment 51

2 years ago
(In reply to Nicolas Silva [:nical] from comment #49)
> Comment on attachment 8741207 [details] [diff] [review]
> patch part 1 - Make TextureHost recycling implicit

> 
> ::: gfx/layers/client/TextureClient.h
> @@ +588,5 @@
> > +  void WaitFenceHandleOnImageBridge(Mutex& aMutex);
> > +  void ClearWaitFenceHandleOnImageBridge(Mutex& aMutex);
> > +  void CancelWaitFenceHandleOnImageBridge();
> > +
> > +  void UpdateFwdTransactionId(uint64_t aTransactionId)
> 
> nit: SetLastFwdTransactionId would be a better name because you already use
> UpdateFwdTransactionId to generate the ids which is different, and also it
> would better document what this is about: marking on the TextureClient the
> id of last transaction it has been used in.

I will update a patch and add a comment here.

> 
> ::: gfx/layers/composite/TextureHost.h
> @@ +40,5 @@
> >  
> >  class BufferDescriptor;
> >  class Compositor;
> >  class CompositableParentManager;
> > +class GrallocTextureHostOGL;
> 
> nit: is this needed?

It is not necessary anymore. It was necessary in early stage of implementation.

> 
> @@ +487,5 @@
> >     * Get the TextureHost corresponding to the actor passed in parameter.
> >     */
> >    static TextureHost* AsTextureHost(PTextureParent* actor);
> >  
> > +  static uint64_t AsTextureSerial(PTextureParent* actor);
> 
> nit: "As" implies that the two representations overlay the same thing, which
> is sort of OK for TextureHost and its IPDL actor, but doesn't make as much
> sense for a texture and an integer id (which doesn't represent the texture
> itself but the last transaction it was used in)
> 
> GetTextureSerial would be better I think.

I will update in a next patch.


> 
> ::: gfx/layers/ipc/CompositableTransactionParent.cpp
> @@ +69,5 @@
> >        !aPictureRect.IsEmpty();
> >  }
> >  #endif
> >  
> > +class MOZ_STACK_CLASS AutoTilesCompositableAddRef
> 
> Could you give a bit more detail about why we need this?
> 
> setting the FwdTransactionId could be done in one of the loops in UseTiles,
> and It's not entirely clear to me why we need to AddCompositableRef and
> ReleaseCompositableRef for all textures when receiving the transaction.
> Is it because a texture may end up never being AddCompositableRef'ed because
> of whatever failure case and that would cause the child side to wait forever?
> If that's so the relationship between the CompositableRef and the child side
> deadlocking isn't explicit and I fear that we'll eventually break this
> guarantee without knowing.
> Having a method that gets called on the texture whenever it fails that does
> the signaling to the child explicitly would be easier to understand,
> although off hands I don't have a precise idea of how to make something like
> this explicit in the API in a safe way. I am not sure that's the actual
> problem this is solving.
> At least a detailed comment would help a lot.

AutoTilesCompositableAddRef is used to ensure to call ReleaseCompositableRef. If CompositableHost does not call TextureHost's AddCompositableRef, ReleaseCompositableRef will not be called. ReleaseCompositableRef have to be called if TextureClient has TextureFlags::RECYCLE flag.

When TextureClient has TextureFlags::RECYCLE, CompositableForwarder holds its ref until host sides end its usage. ReleaseCompositableRef is a trigger to send host side's end of its usage. If ReleaseCompositableRef is not called, CompositableForwarder is going to hold many TextureClients. The symptom becomes memory leak. Except gonk, it does not cause deadlocking on client side. SurfaceFactory and TextureClientRecycleAllocator just allocate new TextureClient if TextureCliets are not recycled.

It seems better to detect the leak happening as ASSERT. I am going to add a check of it.

 
> ::: gfx/layers/ipc/CompositableTransactionParent.h
> @@ +32,5 @@
> >  
> >    virtual void SendAsyncMessage(const InfallibleTArray<AsyncParentMessageData>& aMessage) = 0;
> >  
> > +  virtual void NotifyCompositableRefReleased(PTextureParent* aTexture,
> > +                                             uint64_t aTransactionId) = 0;
> 
> How about "NotifiyNotUsed" instead? Sounds like that name would better
> discribe the intent (which is to notify that texture is no longer used by
> the compositor, rather than the symptom (the ReleaseCompositableRef was
> called down to a ref count of 0).

I will update in a next patch.
(Assignee)

Comment 52

2 years ago
Created attachment 8745893 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Apply the comments except the leak check.
Attachment #8741207 - Attachment is obsolete: true
(Assignee)

Comment 53

2 years ago
Created attachment 8745935 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Correct patch.
Attachment #8745893 - Attachment is obsolete: true
See Also: bug 1227718
Is there some way we could combine what tiling does (gfxSharedReadLock), and what texture recycling does (SetRecycleCallback) to avoid duplication?

Could we use locks for everything, and get rid of the callback system?

Texture recycling could keep an array of allocated textures, and during allocation it can scan the list looking for one with a read lock count of 0.

We also need a solution to deal with the compositor reading from texture memory after the TextureHost has been dropped (see bug 1260611).

TiledContentHost/locking deals with this by holding on to the lock until the next composite for textures that don't have an intermediate buffer.

This still isn't quite correct though, as BasicCompositor isn't deferred, so we're holding the lock longer than necessary in that case.

One possible solution would be to let the Compositor know about locks (probably by having them available from the TextureSource), and have it grab a read lock if it needs to, and then be responsible for releasing it.

A BufferTextureHost would release it's read lock during upload, and create a TextureSource with no lock. A D3D11 TextureHost would create a TextureSource referencing the same data, so would also share the same lock. The TextureHost would release it's lock when it's no longer attached to a compositable.

CompositorD3D11 would grab an extra read lock each time a D3D11 texture source is drawn, and then release it again at the appropriate time.
(In reply to Matt Woodrow (:mattwoodrow) from comment #54)
> Is there some way we could combine what tiling does (gfxSharedReadLock), and
> what texture recycling does (SetRecycleCallback) to avoid duplication?

I agree that there's de-duplication to do, and that something clever could be done with copy-on write locks like we do with tiling, but it's quite different from what sotaro is doing here which is a nice improvement of the current state of things, so I'd rather let sotaro finish this and then we figure something out along what you suggest after that.

The London workweek could be a good time to settle for a good and unified design (one that could leverage the tile-style locks, wouldn't rely on fiddling with the texture's reference count, would not require a destruction callback, etc.). There's plenty of things to fix, unify and improve and I don't expect it to be a simple patch to write so i would like sotaro's current work to land in the mean time.
(Assignee)

Comment 56

2 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #54)
> Is there some way we could combine what tiling does (gfxSharedReadLock), and
> what texture recycling does (SetRecycleCallback) to avoid duplication?

Yea, it is better to unify the recycling system to one. But I want to put off as a follow up bug.

There are 3 ways of recycling. This bug is going to reduce to 2(tile and another). Recycling callback based "b2g recycling" was originally implemented to deliver fence object from compositor side to client side. If we want to use gfxSharedReadLock every where, we need to make clear how to handle fence object.
(Assignee)

Comment 57

2 years ago
(In reply to Nicolas Silva [:nical] from comment #55)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #54)

> The London workweek could be a good time to settle for a good and unified
> design (one that could leverage the tile-style locks, wouldn't rely on
> fiddling with the texture's reference count, would not require a destruction
> callback, etc.).

Yup, London workweek could be a good time.
(Assignee)

Comment 58

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #56)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #54)
> > Is there some way we could combine what tiling does (gfxSharedReadLock), and
> > what texture recycling does (SetRecycleCallback) to avoid duplication?
> 
> Yea, it is better to unify the recycling system to one. But I want to put
> off as a follow up bug.

There are another reason why callback style is used. In media framework, Image could be used my multiple users. Even when, host side releases it, a client side user still could use it and release timing could be any time.
(Assignee)

Updated

2 years ago
No longer blocks: 1260611
(Assignee)

Updated

2 years ago
(Assignee)

Updated

2 years ago
Depends on: 1260611
(Assignee)

Updated

2 years ago
URL: 1260611
(Assignee)

Comment 59

2 years ago
Created attachment 8750652 [details] [diff] [review]
patch - Make TextureHost recycling implicit

Rebased and add comments.
Attachment #8745935 - Attachment is obsolete: true
(Assignee)

Comment 60

2 years ago
Created attachment 8750663 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Update comments.
Attachment #8750652 - Attachment is obsolete: true
(Assignee)

Comment 61

2 years ago
Created attachment 8750714 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected
(Assignee)

Updated

2 years ago
Attachment #8750663 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 62

2 years ago
attachment 8750714 [details] [diff] [review] checks if TextureHosts are CompositableAddRefed correctly in ReceiveCompositableUpdate() by helper classes. If TextureHosts are not compositableAddRefed by the helper classes, it is detected by assert.
(Assignee)

Updated

2 years ago
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8750714 - Attachment description: patch - Check if CompositableRef works as expected → patch part 2 - Check if CompositableRef works as expected
(Assignee)

Updated

2 years ago
Attachment #8750663 - Attachment description: patch - Make TextureHost recycling implicit → patch part 1 - Make TextureHost recycling implicit

Updated

2 years ago
Attachment #8750663 - Flags: feedback?(nical.bugzilla) → feedback+
(Assignee)

Comment 63

2 years ago
Created attachment 8751118 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611
(Assignee)

Updated

2 years ago
Attachment #8751118 - Attachment description: patch - Keep video recycling workaround of bug 1260611 → patch part 3 - Keep video recycling workaround of bug 1260611
(Assignee)

Comment 64

2 years ago
Created attachment 8751593 [details] [diff] [review]
patch part 0 - Remove TextureClientRecycleAllocator workaround of bug 1260611
(Assignee)

Comment 65

2 years ago
Created attachment 8751596 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

Rebased.
Attachment #8751118 - Attachment is obsolete: true
(Assignee)

Comment 66

2 years ago
Created attachment 8752038 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

Add an additional sanity check.
Attachment #8750714 - Attachment is obsolete: true
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 67

2 years ago
Created attachment 8752053 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Update nits.
Attachment #8750663 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 68

2 years ago
Created attachment 8752061 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

Fix build failure.
Attachment #8752038 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8752061 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 69

2 years ago
Created attachment 8752063 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611
Attachment #8751596 - Attachment is obsolete: true
Comment on attachment 8752061 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

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

In my opinion this adds too much complexity for the sake of debug-only checks.
Attachment #8752061 - Flags: feedback?(nical.bugzilla) → feedback-
(Assignee)

Updated

2 years ago
Attachment #8752038 - Attachment is obsolete: false
(Assignee)

Updated

2 years ago
Attachment #8752061 - Attachment is obsolete: true
(Assignee)

Comment 71

2 years ago
Comment on attachment 8752038 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

nical, how about this patch. It is previous one. I do not think it is too complex.
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8752038 - Attachment is obsolete: true
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8750714 - Attachment is obsolete: false
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 72

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #71)
> nical, how about this patch. It is previous one. I do not think it is too
> complex.

Sorry, Attachment 8750714 [details] [diff] is a patch that I am asking to feedback again.
(Assignee)

Comment 73

2 years ago
nica, do you think that even Attachment 8750714 [details] [diff] is not necessary?
(Assignee)

Comment 74

2 years ago
Comment on attachment 8750714 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

I am going to update the patch to simplify the check.
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8750714 - Attachment is obsolete: true
(Assignee)

Comment 75

2 years ago
By Bug 1264764, TextureParent does not hold CompositableParentManager any more. Instead, it holds just ISurfaceAllocator.
(Assignee)

Updated

2 years ago
Depends on: 1274145
(Assignee)

Comment 76

2 years ago
Created attachment 8754685 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Updated a patch based on bug 1274145.
Attachment #8752053 - Attachment is obsolete: true
(Assignee)

Comment 77

2 years ago
Created attachment 8754686 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

Simplified compared to previous one.
(Assignee)

Comment 78

2 years ago
Created attachment 8754691 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

Rebased.
Attachment #8752063 - Attachment is obsolete: true
(Assignee)

Comment 79

2 years ago
Comment on attachment 8754686 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

:nical, can you feedback again? The patch is simpler than the previous one, it does not do though sanity check though.
Attachment #8754686 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 80

2 years ago
Created attachment 8755253 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Fix build failure.
Attachment #8754685 - Attachment is obsolete: true
(Assignee)

Comment 81

2 years ago
Created attachment 8755265 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Fix build failure.
Attachment #8755253 - Attachment is obsolete: true
Comment on attachment 8754686 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

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

I am not convinced that this is very useful. If I understand correctly you want to add assertions to check that NotifyNotUsed is sent, so the code tracks that the extra CompositableRef stuff from the other patch is done, and the actual assertion is called by NotifyNotUsed itself. So instead of checking the result we care about (NotifyNotUsed is sent), we end up checking that some extra steps we take to make it happen was actually done.
This is not that invasive so if you really think it's worth it go ahead, but in this case please add a comment that explains why we check this particular thing and what it means if this assertion is hit some day in the future.
Attachment #8754686 - Flags: feedback?(nical.bugzilla)
I think I am still missing something. The AutoTilesCompositableAddRef RAII goes through every tile in the array of TileDescriptors to make sure that AddCompositableRef is called for all TextureHosts in the array. Is that right, and is there something else that it also ensures ?

Now looking at the code in UseTiles, the first step also goes through all of the tiles from the same array, creates TileHost objects which also calls AddCompositableRef. So we should have the exact same guarantee, right ?

The only exception to this is if UseTiles early-returned false because (!aAllocator || !aCompositor) or if the resolution is invalid before the loop, but in these cases we will kill the child process anyway. So I don't understand what the RAII added in Part 1 gives us. Considering how subtle the CompositableRef stuff is I am not super found of touching it if we don't explicitly need to (TextureHost::BindTextureSource behaves differently if the count is 2 rather than 1, if the count is 2 when it could be 1 the behavior is correct but slower, which means it's hard to detect we are going down the slow path).

Sorry for still questioning this logic after so many feedback rounds. I'd really like to understand the full picture because I am also writing code that interacts with this and I'd like to make sure everything stays compatible (and also because it's already pretty complex).
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 84

2 years ago
Created attachment 8755659 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected
Attachment #8754686 - Attachment is obsolete: true
(Assignee)

Comment 85

2 years ago
(In reply to Nicolas Silva [:nical] from comment #83)
> I think I am still missing something. The AutoTilesCompositableAddRef RAII
> goes through every tile in the array of TileDescriptors to make sure that
> AddCompositableRef is called for all TextureHosts in the array. Is that
> right, and is there something else that it also ensures ?

AutoTilesCompositableAddRef exist for 2 reason.
- Make sure TextureHosts are always Compositable AddRefed in ReceiveCompositableUpdate()
- Set last fwd transaction id of TextureHosts.

> Now looking at the code in UseTiles, the first step also goes through all of
> the tiles from the same array, creates TileHost objects which also calls
> AddCompositableRef. So we should have the exact same guarantee, right ?
> 
> The only exception to this is if UseTiles early-returned false because
> (!aAllocator || !aCompositor) or if the resolution is invalid before the
> loop, but in these cases we will kill the child process anyway. So I don't
> understand what the RAII added in Part 1 gives us.

Originally I implemented RAII to set last fwd transaction id in UseTiles() then moved to ReceiveCompositableUpdate() to remove recycling handing code from CompositableHost as much as possible.

I do not wanted recycling code to depend on only CompositableHost's AddCompositableRef. It seems easily broken silently. For example, ImageHost via ImageBridge case, just after start up, there could be a case that ImageHost does not have Compositor during the transaction. And Compositor renewal might hit the condition.

> Considering how subtle
> the CompositableRef stuff is I am not super found of touching it if we don't
> explicitly need to (TextureHost::BindTextureSource behaves differently if
> the count is 2 rather than 1, if the count is 2 when it could be 1 the
> behavior is correct but slower, which means it's hard to detect we are going
> down the slow path).

It is a good point! I am going to update the patch.

> 
> Sorry for still questioning this logic after so many feedback rounds. I'd
> really like to understand the full picture because I am also writing code
> that interacts with this and I'd like to make sure everything stays
> compatible (and also because it's already pretty complex).

No problem about it :)
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 86

2 years ago
Created attachment 8755670 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

Addressed "compositable ref == 2" problem in Comment 83.
Attachment #8755659 - Attachment is obsolete: true
(Assignee)

Comment 87

2 years ago
Comment on attachment 8755670 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

:nical, how about the patch. It addressed "compositable ref == 2" problem.
Attachment #8755670 - Flags: feedback?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #85)
> (In reply to Nicolas Silva [:nical] from comment #83)
> > I think I am still missing something. The AutoTilesCompositableAddRef RAII
> > goes through every tile in the array of TileDescriptors to make sure that
> > AddCompositableRef is called for all TextureHosts in the array. Is that
> > right, and is there something else that it also ensures ?
> 
> AutoTilesCompositableAddRef exist for 2 reason.
> - Make sure TextureHosts are always Compositable AddRefed in
> ReceiveCompositableUpdate()
> - Set last fwd transaction id of TextureHosts.

Ah! I forgot about this one.

> 
> > Now looking at the code in UseTiles, the first step also goes through all of
> > the tiles from the same array, creates TileHost objects which also calls
> > AddCompositableRef. So we should have the exact same guarantee, right ?
> > 
> > The only exception to this is if UseTiles early-returned false because
> > (!aAllocator || !aCompositor) or if the resolution is invalid before the
> > loop, but in these cases we will kill the child process anyway. So I don't
> > understand what the RAII added in Part 1 gives us.
> 
> Originally I implemented RAII to set last fwd transaction id in UseTiles()
> then moved to ReceiveCompositableUpdate() to remove recycling handing code
> from CompositableHost as much as possible.

I am still not sure why we have to do the AddCompositableRef/ReleaseCOmpositableRef thing at all, because the loop inside of UseTiles already assigns all textures to CompositableTextureRefs slots.

How about adding a loop that only does that without the AddRef/Release?
If you want to assert that the texture was assigned to a CompositableTextureTextureRef slot and that it does not change, you could add something just after UseTiles that would do look like:

> for (size_t i = 0; i < tileDesc.tiles().Length(); i++) {
>   const TileDescriptor& tileDesc = tileDesc.tiles()[i];
>   RefPtr<TextureHost> tex = TextureHost::AsTextureHost(texturedDesc.textureParent();
>   tex->SetFwdTransactionId(id);
>   // Make sure that each texture was handled by the compositable
>   // because the recycling logic depends on it.
>   MOZ_ASSERT(tex->NumCompositableRefs() > 0);
> }

I think that this loop achieves what you need by verifying that the compositable did not forget any texture and the CompositableRefs stuff is correct, as well as setting the id without needing to modify the CompositableRefs itself, and is also less code.

By the way the texture's read locks used by tiling (and soon by other layer types) also rely on the fact that no texture has been forgotten by the compositable, so it is good to assert after the fact that the compositable did not forget any texture (better than re-touching every texture after the fact to be sure, because this will not fix the read-lock case).

> 
> I do not wanted recycling code to depend on only CompositableHost's
> AddCompositableRef. It seems easily broken silently. For example, ImageHost
> via ImageBridge case, just after start up, there could be a case that
> ImageHost does not have Compositor during the transaction. And Compositor
> renewal might hit the condition.

Makes sense. What do you think above the loop I proposed above? Would it solve the problems you are solving with the RAII mechanism?
(Assignee)

Comment 89

2 years ago
Created attachment 8756175 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Rebased.
Attachment #8755265 - Attachment is obsolete: true
(Assignee)

Comment 90

2 years ago
> > > Now looking at the code in UseTiles, the first step also goes through all of
> > > the tiles from the same array, creates TileHost objects which also calls
> > > AddCompositableRef. So we should have the exact same guarantee, right ?
> > > 
> > > The only exception to this is if UseTiles early-returned false because
> > > (!aAllocator || !aCompositor) or if the resolution is invalid before the
> > > loop, but in these cases we will kill the child process anyway. So I don't
> > > understand what the RAII added in Part 1 gives us.
> > 
> > Originally I implemented RAII to set last fwd transaction id in UseTiles()
> > then moved to ReceiveCompositableUpdate() to remove recycling handing code
> > from CompositableHost as much as possible.
> 
> I am still not sure why we have to do the
> AddCompositableRef/ReleaseCOmpositableRef thing at all, because the loop
> inside of UseTiles already assigns all textures to CompositableTextureRefs
> slots.

It is because, RAII code implementation is transparent about inside of UseTiles. "(!aAllocator || !aCompositor)" exists now and might be changed in future.

> How about adding a loop that only does that without the AddRef/Release?
> If you want to assert that the texture was assigned to a
> CompositableTextureTextureRef slot and that it does not change, you could
> add something just after UseTiles that would do look like:
> 
> > for (size_t i = 0; i < tileDesc.tiles().Length(); i++) {
> >   const TileDescriptor& tileDesc = tileDesc.tiles()[i];
> >   RefPtr<TextureHost> tex = TextureHost::AsTextureHost(texturedDesc.textureParent();
> >   tex->SetFwdTransactionId(id);
> >   // Make sure that each texture was handled by the compositable
> >   // because the recycling logic depends on it.
> >   MOZ_ASSERT(tex->NumCompositableRefs() > 0);
> > }
> 
> I think that this loop achieves what you need by verifying that the
> compositable did not forget any texture and the CompositableRefs stuff is
> correct, as well as setting the id without needing to modify the
> CompositableRefs itself, and is also less code.
> 
> By the way the texture's read locks used by tiling (and soon by other layer
> types) also rely on the fact that no texture has been forgotten by the
> compositable, so it is good to assert after the fact that the compositable
> did not forget any texture (better than re-touching every texture after the
> fact to be sure, because this will not fix the read-lock case).

It is OK for me to remove AddCompositableRef and ReleaseCompositableRef from AutoTilesCompositableAddRef and insert assert.


> > I do not wanted recycling code to depend on only CompositableHost's
> > AddCompositableRef. It seems easily broken silently. For example, ImageHost
> > via ImageBridge case, just after start up, there could be a case that
> > ImageHost does not have Compositor during the transaction. And Compositor
> > renewal might hit the condition.
> 
> Makes sense. What do you think above the loop I proposed above? Would it
> solve the problems you are solving with the RAII mechanism?

It is OK for me. I am going to update the patch.
(Assignee)

Comment 91

2 years ago
Created attachment 8756180 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected
Attachment #8755670 - Attachment is obsolete: true
Attachment #8755670 - Flags: feedback?(nical.bugzilla)
(Assignee)

Comment 92

2 years ago
Comment on attachment 8756180 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

nical, how about this patch?
Attachment #8756180 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8756180 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

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

I really think you should remove these RAII classes, they hide the logic more than anything and only protect us against someone in the future early-returning before calling Compositable->Use***, but the same person might as well early-return before the RAII guard causing the same issue anyway.

::: gfx/layers/composite/TextureHost.h
@@ +603,5 @@
>    PTextureParent* mActor;
>    TextureFlags mFlags;
>    int mCompositableCount;
>    uint64_t mFwdTransactionId;
> +  bool mIsTrackingForCompositableRef;

We should not mIsTrackingForCompositableRef and the extra methods that come with it, since all we care about is asserting that all textures have been assigned to CompositableTextureRef slots.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +179,1 @@
>          texture->ReleaseCompositableRef();

As far as I understand, this is not correct (not this patch particularly but the approach in general for timed textures). When an ImageHost receives a set of timed textures, only the most "current" is assigned to a CompositableTextureRef slot. If you add and release all texture's compositableRef, it means that all textures except the current one will be marked as recyclable, which should not be the case, because they will be used later (most probably) when their timestamp correspond to the compositor's timestamp.

To fix this you could make the ImageHost's TimedImage hold a CompositableTextureHostRef insted of a RefPtr<TextureHost>. I think this would make sense. If you do this, it is very important that you make mCurrentTextureHost a RefPtr<TextureHost> instead of a CompositableTextureHostRef, because otherwise you'll end up with a compositableRef counter always equal to 2 for the current texture, which will go down the slow path every time.

There should be no place where you have to do the manual Add/Release CompositableRefs (be it tiling or any other layer type), because if you do it manually, it means that the compositable has missed something and it is a bug on the compositable's side and should be fixed there, and/or you are marking as recyclable a texture that will be used later.

@@ +220,1 @@
>        mTextureHost->ReleaseCompositableRef();

Same here, if the texture was not assigned to a CompositableTextureRef slot, it's a bug in the CompositableHost and it will break plenty of other things. It's better to document that CompositableHost are supposed to do something, than add some extra code in case they don't do what they are supposed to do.
Instead of 34 lines of RAII class, we can just add a single call to SetLastFwdTransactionId per texture and optionally an assertion about the NumCompositableTextureRefs if you are worried that bugs may be introduced in the CompositableHost (I personally think that a comment explaining why it is important would be more effective than the assertion, but asserting the right thing can't hurt).
Attachment #8756180 - Flags: feedback?(nical.bugzilla) → feedback-
(Assignee)

Comment 94

2 years ago
(In reply to Nicolas Silva [:nical] from comment #93)
> 
> Same here, if the texture was not assigned to a CompositableTextureRef slot,
> it's a bug in the CompositableHost and it will break plenty of other things.
> It's better to document that CompositableHost are supposed to do something,
> than add some extra code in case they don't do what they are supposed to do.
> Instead of 34 lines of RAII class, we can just add a single call to
> SetLastFwdTransactionId per texture and optionally an assertion about the
> NumCompositableTextureRefs if you are worried that bugs may be introduced in
> the CompositableHost (I personally think that a comment explaining why it is
> important would be more effective than the assertion, but asserting the
> right thing can't hurt).

The prerequisite about CompositableHost seems different than me. It is OK to remove the check and assigning CompositableTextureRef should be a responsibility of CompositableHost. But I am not sure about your comment of mentioning about how to do "assertion about the NumCompositableTextureRefs" without RAII.
(Assignee)

Comment 95

2 years ago
Created attachment 8756670 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

Apply the comment except removing RAII.
Attachment #8756180 - Attachment is obsolete: true
(Assignee)

Comment 96

2 years ago
Comment on attachment 8756670 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

:nical, can you feedback to the patch? I applied the comment except removing RAII, since your idea of removing RAII was not clear for me.
Attachment #8756670 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8756670 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

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

Sorry for the confusing explanations about the RAII thing, Thanks for removing the manual Add/ReleaseCompositableRefs, though.

It's not worth arguing forever about RAII vs explicit control flow, so if you feel strongly about the RAII class, go ahead. But please rename it to AutoSetFwdTransactionId.

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +113,2 @@
>          }
>        }

By removing the RAII, I meant simply putting this loop directly after the line "bool success = tiledHost->UseTiledLayerBuffer(this, tileDesc);" in CompositableTransactionParent.cpp, just like you did for UseComponentAlphaTextures. Same thing for OpUseTexture.

We don't need this to be triggered by an RAII guard, we just need it to run right after UseTiledLayerBuffer. RAII guards are great when you need something to happen at the end of a scope and the function can return from several places in that scope. But that comes at the cost of hiding what is happening since the destructor is called implicitly and not explicitly. In our case the control flow is simple, the function doesn't return half-way through and you only need the loop to run after the Use***() call (not specifically at the end of the scope). Seeing the sequence of instructions explcitly written where they are run makes it simpler to understand what the code does at a glance.
Attachment #8756670 - Flags: feedback?(nical.bugzilla) → feedback+
(Assignee)

Comment 98

2 years ago
I understand the RAII part. I am going to remove it.
(Assignee)

Comment 99

2 years ago
Created attachment 8757742 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected
Attachment #8756670 - Attachment is obsolete: true
(Assignee)

Comment 100

2 years ago
Created attachment 8757755 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

Rebased.
Attachment #8754691 - Attachment is obsolete: true
(Assignee)

Comment 101

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f65ada16c0d&selectedJob=21646873
(Assignee)

Updated

2 years ago
Attachment #8751593 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8756175 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8757742 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8757755 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 102

2 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #101)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2f65ada16c0d&selectedJob=21646873

There is a crash of SharedSurface_IOSurface(). If we wait Bug 1167235, it might be addressed.

Updated

2 years ago
Attachment #8751593 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8756175 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

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

Looks good, could you fold part 2 into this patch ?

::: gfx/layers/client/TextureClient.h
@@ +588,5 @@
> +  void CancelWaitFenceHandleOnImageBridge();
> +
> +  /**
> +   * Set last transaction id of CompositableForwarder.
> +   * 

nit: trailing space

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +385,5 @@
>    mSyncObject = SyncObject::CreateSyncObject(aIdentifier.mSyncHandle);
>  }
>  
> +ShadowLayerForwarder::ShadowLayerForwarder(ClientLayerManager* aClientLayerManager)
> + : mClientLayerManager(aClientLayerManager) 

nit: trailing space
Attachment #8756175 - Flags: review?(nical.bugzilla) → review+

Updated

2 years ago
Attachment #8757742 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8757755 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

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

::: gfx/layers/composite/LayerManagerComposite.h
@@ +399,5 @@
>  
>    nsTArray<ImageCompositeNotification> mImageCompositeNotifications;
>  
> +  nsTArray<CompositableTextureHostRef> mCurrentHeldTextureHosts;
> +  nsTArray<CompositableTextureHostRef> mPreviousHeldTextureHosts;

I am not found of this. for one, it doesn't interact well with the ReadLock patch queue (although it's not then end of the world if this is only used with TextureFlags::RECYCLE on ImageHost). More generally we should really use CompositableTextureRef only for what it was intended: counting the number of compositables that are using a given texture at any time.

The original bug advertises this as a short-term fix. Do we have plans to fix the video flickering issue in a different way?
(Assignee)

Updated

2 years ago
Depends on: 1272600
(Assignee)

Comment 105

2 years ago
(In reply to Nicolas Silva [:nical] from comment #104)
> 
> The original bug advertises this as a short-term fix. Do we have plans to
> fix the video flickering issue in a different way?

The patch is independent from bug 1272600. Based on bug 1272600, it is possible to implement. I am going to update the patch.
(Assignee)

Updated

2 years ago
Attachment #8757755 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Depends on: 1167235
(Assignee)

Updated

2 years ago
No longer depends on: 1167235
See Also: → bug 1167235
(Assignee)

Comment 106

2 years ago
Created attachment 8760110 [details] [diff] [review]
patch part 0 - Remove TextureClientRecycleAllocator workaround of bug 1260611

Rebased.
Attachment #8751593 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8760110 - Flags: review+
(Assignee)

Comment 107

2 years ago
Created attachment 8760115 [details] [diff] [review]
patch part 1 - Make TextureHost recycling implicit

Rebased.
Attachment #8756175 - Attachment is obsolete: true
Attachment #8760115 - Flags: review+
(Assignee)

Comment 108

2 years ago
Created attachment 8760118 [details] [diff] [review]
patch part 2 - Check if CompositableRef works as expected

Rebased.
Attachment #8757742 - Attachment is obsolete: true
Attachment #8760118 - Flags: review+
(Assignee)

Comment 109

2 years ago
Created attachment 8760136 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

Rebased and apply the comment.
Attachment #8757755 - Attachment is obsolete: true
(Assignee)

Comment 110

2 years ago
Created attachment 8760137 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

Update nits.
Attachment #8760136 - Attachment is obsolete: true
(Assignee)

Comment 111

2 years ago
Created attachment 8760152 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

Fix build failure.
Attachment #8760137 - Attachment is obsolete: true
(Assignee)

Comment 112

2 years ago
Created attachment 8760532 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611
Attachment #8760152 - Attachment is obsolete: true
(Assignee)

Comment 113

2 years ago
Created attachment 8760568 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611
Attachment #8760532 - Attachment is obsolete: true
(Assignee)

Comment 114

2 years ago
Created attachment 8760676 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611
Attachment #8760568 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8760676 - Flags: review?(nical.bugzilla)
Comment on attachment 8760676 [details] [diff] [review]
patch part 3 - Keep video recycling workaround of bug 1260611

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

Looks good. Wasn't there something about D3D11 textures as well? Or did the patch that blocks compositing Frame N until frame N-1 is finished take care of the D3D11 issue completely (I don't remember)?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +394,5 @@
>  
>    if (mRoot && !(aFlags & END_NO_IMMEDIATE_REDRAW)) {
>      MOZ_ASSERT(!aTimeStamp.IsNull());
>      UpdateAndRender();
> +    mCompositor->FlushPendingNotifyNotUsed();

Wouldn't it be simpler to call this in Compositor::EndFrame?
Attachment #8760676 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Comment 116

2 years ago
(In reply to Nicolas Silva [:nical] from comment #115)
> >  
> >    if (mRoot && !(aFlags & END_NO_IMMEDIATE_REDRAW)) {
> >      MOZ_ASSERT(!aTimeStamp.IsNull());
> >      UpdateAndRender();
> > +    mCompositor->FlushPendingNotifyNotUsed();
> 
> Wouldn't it be simpler to call this in Compositor::EndFrame?

attachment 8760676 [details] [diff] [review] should work mostly similar to bug 1260611.

Compositor::EndFrame is called too early for FlushPendingNotifyNotUsed(). Then it was not used. In CompositorD3D11::EndFrame(), Compositor::EndFrame() is called in the beginning. "Previous frame complete waiting code" is in the bottom of CompositorD3D11::EndFrame().
   https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#1297
(Assignee)

Comment 117

2 years ago
(In reply to Nicolas Silva [:nical] from comment #115)
> Comment on attachment 8760676 [details] [diff] [review]
> patch part 3 - Keep video recycling workaround of bug 1260611
> 
> Review of attachment 8760676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Wasn't there something about D3D11 textures as well? Or did the
> patch that blocks compositing Frame N until frame N-1 is finished take care
> of the D3D11 issue completely (I don't remember)?
> 

CompositorD3D11::EndFrame() has a code to wait previous frame. Therefore if FlushPendingNotifyNotUsed() is called after the EndFrame(), it means wait n-1 frame complete.
(Assignee)

Comment 118

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a68b6559f6&selectedJob=22111542

There is a test failure on mac that is caused by SharedSurface_IOSurface that seems alive longer than GL context. The problem might be addressed by waiting bug 1167235.
(Assignee)

Comment 119

a year ago
Created attachment 8762714 [details] [diff] [review]
patch part 4 - Add CancelWaitForRecycle()

Revived CancelWaitForCompositorRecycle() as CancelWaitForRecycle() to shorten TextureClients lifetime. It is for fixing Comment 118.
(Assignee)

Comment 120

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a288feef81b3
(Assignee)

Updated

a year ago
Attachment #8762714 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 121

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf1bd78af075
Attachment #8762714 - Flags: review?(nical.bugzilla) → review+

Comment 122

a year ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5748a69df90b
Make TextureHost recycling implicit r=nical

Comment 123

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5748a69df90b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1281169
(Assignee)

Updated

a year ago
No longer blocks: 1281169
Depends on: 1281169
Depends on: 1287627
Sotaro, could this have caused crashes like this one: https://crash-stats.mozilla.com/report/index/99a61377-b65f-49e3-9718-7d7e82160820.  Linux only, in 50 and 51.  Tracked in bug 1298158.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 125

a year ago
(In reply to Milan Sreckovic [:milan] from comment #124)
> Sotaro, could this have caused crashes like this one:
> https://crash-stats.mozilla.com/report/index/99a61377-b65f-49e3-9718-
> 7d7e82160820.  Linux only, in 50 and 51.  Tracked in bug 1298158.

I take a look. It might be related to Bug 1155000.
Flags: needinfo?(sotaro.ikeda.g)
You need to log in before you can comment on or make changes to this bug.