962101 - Handle ImageClientSingle::FlushAllImages()'s task in Compositable level

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #951732 +++

This bug is created based on Bug 951732 Comment 19.

In current gecko's implementation, ImageClientSingle::FlushAllImages() uses TextureClient::ForceRemove(). From the following reasons, it should be handled by CompositableClient/Host as same as before PTexture.
- Fence object need to be delivered to TextureClient even when ImageClientSingle::FlushAllImages() is called.
- When TextureClient is rendered to multiple targets, ImageClientSingle::FlushAllImages() should affect to the TextureClient, but not to other CompositableClient. In current gecko, ImageClientSingle::FlushAllImages() disable the all Compositable's rendering that using same TexutreClient.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Handle Remove Texture in Compositable level

This is wip patch. Function names are temporary, but it works on master hamachi.

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8368209 [details] [diff] [review]
wip patch - Handle Remove Texture in Compositable level

Nical, can I have a feed back about the patch from you? This way of change is necessary to handle multiple target rendering and fence.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8368209 [details] [diff] [review]
wip patch - Handle Remove Texture in Compositable level

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

I am not entirely certain about what this is achieving, so i'll give it a second pass tomorrow and maybe I'll need to chat with you too.
The sync transaction for every texture removal really does ring an alarm bell. Right now destroying textures in async except in few special cases such as hardware accelerated video with GonkNativeWindow. Even in those special cases I hope to make it async most of the time (but that would first require Bug 952404 to be fixed). This patch seems to make it always synchronous which really hurts. We should look for a solution that minimizes the amount of synchronous messages.
It looks like there are a few bugs, see comments below.
Beware that textures can outlive compositables so expecting a compositable to be present every time we destroy a texture is a dangerous assumption (not sure whether this affects your patch or not)

::: gfx/layers/client/CompositableClient.h
@@ +135,5 @@
> +   /**
> +   * Tells the Compositor to delete the TextureHost corresponding to this
> +   * TextureClient.
> +   */
> +  virtual void NewRemoveTextureClient(TextureClient* aClient);

How about naming this "RemoveAfterTransaction"?

::: gfx/layers/composite/ImageHost.cpp
@@ +46,5 @@
> +ImageHost::RemoveTextureHost(TextureHost* aTexture)
> +{
> +  CompositableHost::RemoveTextureHost(aTexture);
> +  if (!aTexture && mFrontBuffer == aTexture) {
> +    aTexture->SetCompositableBackendSpecificData(nullptr);

Looks like you meant to write "if (aTexture ..."

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +225,5 @@
> +      CompositableHost* compositable = AsCompositable(op);
> +      RefPtr<TextureHost> tex = TextureHost::AsTextureHost(op.textureParent());
> +
> +      MOZ_ASSERT(tex.get());
> +      //compositable->RemoveTextureHost(tex);

Maybe you mean to uncomment this line? it's the only one that corresponds to what the message says (RemoveTexture)

@@ +228,5 @@
> +      MOZ_ASSERT(tex.get());
> +      //compositable->RemoveTextureHost(tex);
> +
> +      if (IsAsync()) {
> +        ScheduleComposition(op);

Why does removing a Texture schedule composition?

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +440,5 @@
> +                                       TextureClient* aTexture)
> +{
> +  mTxn->AddEdit(OpRemoveTexture(nullptr, aCompositable->GetIPDLActor(),
> +                                nullptr, aTexture->GetIPDLActor()));
> +  mTxn->MarkSyncTransaction();

This will make a lot of transactions synchronous which hurts perf pretty bad. Isn't there a way to remain async?

Comment 4

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> Comment on attachment 8368209 [details] [diff] [review]
> wip patch - Handle Remove Texture in Compositable level
> 
> Nical, can I have a feed back about the patch from you? This way of change
> is necessary to handle multiple target rendering and fence.

This patch doesn't contain anything about fences, can you explain how does it interacts with the fence stuff?
I haven't figured out what putting the removal in the transaction provides us in this patch (it looks like it just triggers a composition right now).
I am also probably being confused about the meaning of OpRemoveTexture since in the old days it was the message to delete the texture, whereas here it probably means something different such as telling the compositable host about the texture being detached from the compositable and asking it some fence related stuff which is missng from the patch maybe?

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #3)
> Comment on attachment 8368209 [details] [diff] [review]
> wip patch - Handle Remove Texture in Compositable level
> 
> Review of attachment 8368209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not entirely certain about what this is achieving, so i'll give it a
> second pass tomorrow and maybe I'll need to chat with you too.
> The sync transaction for every texture removal really does ring an alarm
> bell. Right now destroying textures in async except in few special cases
> such as hardware accelerated video with GonkNativeWindow. Even in those
> special cases I hope to make it async most of the time (but that would first
> require Bug 952404 to be fixed). This patch seems to make it always
> synchronous which really hurts. We should look for a solution that minimizes
> the amount of synchronous messages.

It is not for destroy texture. It is for removing TextureHost from Compositable. Current implementation already using TextureClient::ForceRemove() to remove texture. It triggers individual synchronous transaction at PTexture protocol level. Why does the change make more harm than current implementation?

> It looks like there are a few bugs, see comments below.
> Beware that textures can outlive compositables so expecting a compositable
> to be present every time we destroy a texture is a dangerous assumption (not
> sure whether this affects your patch or not)

This patch is not for destroying the texture. This patch is for to unregister Texutre from Compositable.

> ::: gfx/layers/client/CompositableClient.h
> @@ +135,5 @@
> > +   /**
> > +   * Tells the Compositor to delete the TextureHost corresponding to this
> > +   * TextureClient.
> > +   */
> > +  virtual void NewRemoveTextureClient(TextureClient* aClient);
> 
> How about naming this "RemoveAfterTransaction"?

As in comment 1, I put just temporary function name. I prefer "RemoveTexutreFromCompositable" or such kind of name.

> ::: gfx/layers/composite/ImageHost.cpp
> @@ +46,5 @@
> > +ImageHost::RemoveTextureHost(TextureHost* aTexture)
> > +{
> > +  CompositableHost::RemoveTextureHost(aTexture);
> > +  if (!aTexture && mFrontBuffer == aTexture) {
> > +    aTexture->SetCompositableBackendSpecificData(nullptr);
> 
> Looks like you meant to write "if (aTexture ..."

yes.

> 
> ::: gfx/layers/ipc/CompositableTransactionParent.cpp
> @@ +225,5 @@
> > +      CompositableHost* compositable = AsCompositable(op);
> > +      RefPtr<TextureHost> tex = TextureHost::AsTextureHost(op.textureParent());
> > +
> > +      MOZ_ASSERT(tex.get());
> > +      //compositable->RemoveTextureHost(tex);
> 
> Maybe you mean to uncomment this line? it's the only one that corresponds to
> what the message says (RemoveTexture)

yes.

> 
> @@ +228,5 @@
> > +      MOZ_ASSERT(tex.get());
> > +      //compositable->RemoveTextureHost(tex);
> > +
> > +      if (IsAsync()) {
> > +        ScheduleComposition(op);
> 
> Why does removing a Texture schedule composition?

It is unintentional. The base code is just copied from somewhere.

> 
> ::: gfx/layers/ipc/ShadowLayers.cpp
> @@ +440,5 @@
> > +                                       TextureClient* aTexture)
> > +{
> > +  mTxn->AddEdit(OpRemoveTexture(nullptr, aCompositable->GetIPDLActor(),
> > +                                nullptr, aTexture->GetIPDLActor()));
> > +  mTxn->MarkSyncTransaction();
> 
> This will make a lot of transactions synchronous which hurts perf pretty
> bad. Isn't there a way to remain async?

Why is it a problem? Current implementation is already synchronous transaction at PTexture protocol level.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #4)
> (In reply to Sotaro Ikeda [:sotaro] from comment #2)
> > Comment on attachment 8368209 [details] [diff] [review]
> > wip patch - Handle Remove Texture in Compositable level
> > 
> > Nical, can I have a feed back about the patch from you? This way of change
> > is necessary to handle multiple target rendering and fence.
> 
> This patch doesn't contain anything about fences, can you explain how does
> it interacts with the fence stuff?
> I haven't figured out what putting the removal in the transaction provides
> us in this patch (it looks like it just triggers a composition right now).
> I am also probably being confused about the meaning of OpRemoveTexture since
> in the old days it was the message to delete the texture, whereas here it
> probably means something different such as telling the compositable host
> about the texture being detached from the compositable and asking it some
> fence related stuff which is missng from the patch maybe?

OpRemoveTexture means remove texture from compositable. On video playback/camera preview, remove texture means return buffer to ANativeWindow. To the buffer, fence object need to deliver from parent side to child side.

Comment 7

[Sotaro Ikeda [:sotaro]]

I misunderstand the code, the following is not always synchronous.

void TextureClient::ForceRemove()
{
  if (mValid && mActor) {
    if (GetFlags() & TEXTURE_DEALLOCATE_CLIENT) {
      mActor->SetTextureData(DropTextureData());
      if (mActor->IPCOpen()) {
        mActor->SendRemoveTextureSync();
      }
      mActor->DeleteTextureData();
    } else {
      if (mActor->IPCOpen()) {
        mActor->SendRemoveTexture();
      }
    }
  }
  MarkInvalid();
}

Comment 8

[Sotaro Ikeda [:sotaro]]

Nical, thanks for the comment. I am going to update the patch to be more async way.

Comment 9

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Nical, thanks for the comment. I am going to update the patch to be more
> async way.

Ok, great, the synchronous stuff is what was really bothering me, the rest doesn't look too alarming even though I'll probably have to chat with you to understand the final patch.

Comment 10

[Sotaro Ikeda [:sotaro]]

Yea, the patch was kind of initial sketch. From your comment, I understand and recognize some problems. I am going to fix them in a next patch.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Handle Remove Texture in Compositable level

Apply the comments.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Handle Remove Texture in Compositable level

Fix nits.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Handle Remove Texture in Compositable level

Fix nits.

Comment 15

[Jason Smith [:jsmith]]

Noming per the dupe - this is a blocker for a 1.4 committed user story for video recording.

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 8371074 [details] [diff] [review]
patch v4 - Handle Remove Texture in Compositable level

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

Looks good, could you remind me why we need to do this in the transaction (instead of just doing it in PTexture::RemoveTexture)?

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +931,5 @@
>    return SendPTextureConstructor(aSharedData, aFlags);
>  }
>  
> +void
> +ImageBridgeChild::RemoveTexutreFromCompositable(CompositableClient* aCompositable,

there's a typo here (Texutre)

::: gfx/layers/ipc/ShadowLayers.h
@@ +278,5 @@
>    virtual void UpdateTexture(CompositableClient* aCompositable,
>                               TextureIdentifier aTextureId,
>                               SurfaceDescriptor* aDescriptor) MOZ_OVERRIDE;
>  
> +  virtual void RemoveTexutreFromCompositable(CompositableClient* aCompositable,

Please add a comment explaining the difference between this and RemoveTexture (we should rename RemoveTexture at some point to make it less confusing)

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #16)
> 
> Looks good, could you remind me why we need to do this in the transaction
> (instead of just doing it in PTexture::RemoveTexture)?

- To support that one Texture is rendered to multiple CompositableHost. If we want to support it, we need an explicit way to remove TextureHost from CompositableHost. In current implementation, TextureHost lifetime is almost tied to one CompositableHost's TextureHost usage.

- To return Fence object to client side when CompositableHost's TextureHost usage is ended.

Comment 18

[Sotaro Ikeda [:sotaro]]

Additional reason.
- After applying attachment 8371074 [details] [diff] [review], we could remove TEXTURE_DEALLOCATE_DEFERRED flag.

Comment 19

[Sotaro Ikeda [:sotaro]]

> ::: gfx/layers/ipc/ImageBridgeChild.cpp
> @@ +931,5 @@
> >    return SendPTextureConstructor(aSharedData, aFlags);
> >  }
> >  
> > +void
> > +ImageBridgeChild::RemoveTexutreFromCompositable(CompositableClient* aCompositable,
> 
> there's a typo here (Texutre)

Will update in a next patch.

> 
> ::: gfx/layers/ipc/ShadowLayers.h
> @@ +278,5 @@
> >    virtual void UpdateTexture(CompositableClient* aCompositable,
> >                               TextureIdentifier aTextureId,
> >                               SurfaceDescriptor* aDescriptor) MOZ_OVERRIDE;
> >  
> > +  virtual void RemoveTexutreFromCompositable(CompositableClient* aCompositable,
> 
> Please add a comment explaining the difference between this and
> RemoveTexture (we should rename RemoveTexture at some point to make it less
> confusing)

Will add a comment in a next patch.

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> 
> > 
> > ::: gfx/layers/ipc/ShadowLayers.h
> > @@ +278,5 @@
> > >    virtual void UpdateTexture(CompositableClient* aCompositable,
> > >                               TextureIdentifier aTextureId,
> > >                               SurfaceDescriptor* aDescriptor) MOZ_OVERRIDE;
> > >  
> > > +  virtual void RemoveTexutreFromCompositable(CompositableClient* aCompositable,
> > 
> > Please add a comment explaining the difference between this and
> > RemoveTexture (we should rename RemoveTexture at some point to make it less
> > confusing)
> 
> Will add a comment in a next patch.

I recognized that the function's explanation is already in CompositableForwarder.h. I am going to update the comment.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch v5 - Handle Remove Texture in Compositable level

Apply the comment.

Comment 22

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> - To support that one Texture is rendered to multiple CompositableHost. If
> we want to support it, we need an explicit way to remove TextureHost from
> CompositableHost. In current implementation, TextureHost lifetime is almost
> tied to one CompositableHost's TextureHost usage.
> 
> - To return Fence object to client side when CompositableHost's TextureHost
> usage is ended.

Ok makes, sense. The and the Fence stuff will come as a followup (what confused me a bit was that I remembered you mentioned it had something to do with Fence, but there is no mention to Fence in this patch)

(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> Additional reason.
> - After applying attachment 8371074 [details] [diff] [review], we could
> remove TEXTURE_DEALLOCATE_DEFERRED flag.

Nice!

Comment 23

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #22)
> > 
> > - To return Fence object to client side when CompositableHost's TextureHost
> > usage is ended.
> 
> Ok makes, sense. The and the Fence stuff will come as a followup (what
> confused me a bit was that I remembered you mentioned it had something to do
> with Fence, but there is no mention to Fence in this patch)
> 

The fence is going to be implemented in bug 957323.

Comment 24

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=e04ef942cec0

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch v6 - Handle Remove Texture in Compositable level (r=nical)

Committable patch. Carry "r=nical".

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Bitrotted, please rebase.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch v7 - Handle Remove Texture in Compositable level (r=nical)

Un-bitrot.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/634e3ed4fb1d

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/634e3ed4fb1d