Closed Bug 962101 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 6 obsolete files)

+++ 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.
Blocks: 957323
No longer depends on: 951732
Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
This is wip patch. Function names are temporary, but it works on master hamachi.
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.
Attachment #8368209 - Flags: feedback?(nical.bugzilla)
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?
(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?
(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.
(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.
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();
}
Nical, thanks for the comment. I am going to update the patch to be more async way.
(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.
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.
Attachment #8368209 - Flags: feedback?(nical.bugzilla)
Apply the comments.
Attachment #8368209 - Attachment is obsolete: true
Attachment #8371045 - Flags: review?(nical.bugzilla)
Attachment #8371045 - Flags: review?(nical.bugzilla)
Fix nits.
Attachment #8371045 - Attachment is obsolete: true
Fix nits.
Attachment #8371065 - Attachment is obsolete: true
Attachment #8371074 - Flags: review?(nical.bugzilla)
Duplicate of this bug: 959487
Noming per the dupe - this is a blocker for a 1.4 committed user story for video recording.
blocking-b2g: --- → 1.4?
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)
(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.
Additional reason.
- After applying attachment 8371074 [details] [diff] [review], we could remove TEXTURE_DEALLOCATE_DEFERRED flag.
> ::: 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.
(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.
Apply the comment.
Attachment #8371074 - Attachment is obsolete: true
Attachment #8371074 - Flags: review?(nical.bugzilla)
Attachment #8371545 - Flags: review?(nical.bugzilla)
(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!
Attachment #8371545 - Flags: review?(nical.bugzilla) → review+
Blocks: 968872
(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.
Committable patch. Carry "r=nical".
Attachment #8371545 - Attachment is obsolete: true
Attachment #8373086 - Flags: review+
Keywords: checkin-needed
Bitrotted, please rebase.
Keywords: checkin-needed
Un-bitrot.
Attachment #8373086 - Attachment is obsolete: true
Attachment #8373364 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/634e3ed4fb1d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
blocking-b2g: 1.4? → 1.4+
You need to log in before you can comment on or make changes to this bug.