Closed Bug 844928 Opened 8 years ago Closed 8 years ago

[layers-refactoring] remove the PTexture protocol

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: nical, Assigned: nrc)

References

Details

Attachments

(2 files, 8 obsolete files)

Recently we changed texture transfer to go through CompositablesHost rather than to TextureHost directly. letting Texture clients/host communicate directly was the only purpose of PTexture, so we really want to remove PTexture, that has become useless and complicates things.
At the moment TextureParent owns the TextureHost, but CompositableHost sometimes acts like it owns instead of TextureParent which causes bugs.
So we need to:
- Remove PTexture entirely
- Every ipdl message that contained a PTexture should replace it by a PCompositable and a texture identifier for the few cases where a compositable has several textures.
- CompositableHost owns its TextureHosts and is responsible for managing them and changing them when needed.

This is a rather big change but it will reduce complexity and fix some problems.
Blocks: 858914
Assignee: nobody → ncameron
Got started on this today.
Attached patch 1 Remove PTexture (obsolete) — Splinter Review
Attachment #735608 - Flags: review?(nical.bugzilla)
These will def. need a try push because of the change to LayerManager::Destroy
Attachment #735609 - Flags: review?(nical.bugzilla)
Comment on attachment 735608 [details] [diff] [review]
1 Remove PTexture

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

::: gfx/layers/CompositorTypes.h
@@ +99,5 @@
> + * the id is unique for any texture owned by a particular compositable.
> + */
> +typedef uint32_t TextureIdentifier;
> +const TextureIdentifier TextureFront = 1;
> +const TextureIdentifier TextureBack = 2;

It'd be nice to use an enum instead

::: gfx/layers/client/CanvasClient.cpp
@@ +40,2 @@
>  {
> +  mForwarder->UpdateTexture(this, 1, mTextureClient->LockSurfaceDescriptor());

lock without unlock in the same scope makes me uncomfortable.
maybe it needs a GetSurfaceDescriptor, this line doesn't seem to be needing the lock/unlock semantic

::: gfx/layers/client/CompositableClient.h
@@ +75,2 @@
>    {
> +    NS_ERROR("This method should be overridden");

Use MOZ_NOT_REACHED for consistency with the code below

::: gfx/layers/client/ContentClient.cpp
@@ +251,5 @@
>    mFrontBufferRotation = nsIntPoint();
>  
> +  mForwarder->CreatedDoubleBuffer(this,
> +                                  *mFrontClient->LockSurfaceDescriptor(),
> +                                  *mTextureClient->LockSurfaceDescriptor(),

Lock is not appropriate for this situation, we need a proper getter

@@ +420,3 @@
>  {
> +  mForwarder->CreatedSingleBuffer(this,
> +                                  *mTextureClient->LockSurfaceDescriptor(),

Lock is not appropriate for this situation, we need a proper getter (again)

::: gfx/layers/client/ContentClient.h
@@ +123,2 @@
>    {
>      MOZ_ASSERT(false, "Should not be called on non-remote ContentClient");

Nit: MOZ_NOT_REACHED

::: gfx/layers/client/ImageClient.cpp
@@ +189,5 @@
>  void
>  ImageClientSingle::Updated()
>  {
> +  mForwarder->UpdateTexture(this, 1, mTextureClient->LockSurfaceDescriptor());
> +  //NS_ASSERTION(!IsSurfaceDescriptorValid(*mTextureClient->LockSurfaceDescriptor()), "didn't delete SurfaceDescriptor");

Commented out code

::: gfx/layers/client/TiledContentClient.cpp
@@ +180,5 @@
>                                              const nsIntRect& aDirtyRect)
>  {
>    if (aTile.IsPlaceholderTile()) {
> +    TextureInfo textureInfo;
> +    textureInfo.mCompositableType = BUFFER_TILED;

I see that we have a lot of places where we create a default initialized TextureInfo, set it's CompositableType and pass it to a function, how about we add a constructor to TextureInfo that takes a Compositable type, and pass it directly?

::: gfx/layers/composite/ContentHost.cpp
@@ +213,3 @@
>  {
> +  MOZ_ASSERT(aTextureId == TextureFront);
> +  mNewFrontHost = TextureHost::CreateTextureHost(aSurface.type(),

Shouldn't we handle or assert the case where mNewFrontHost already contains something?
Or perhaps EnsureTextureHost is not the right name and it should be something like CreateFrontHost?

@@ +288,5 @@
>    DestroyFrontHost();
>  }
>  
> +bool
> +ContentHostDoubleBuffered::EnsureTextureHost(TextureIdentifier aTextureId,

Same thing here, this what happens if we call it twice?

::: gfx/layers/composite/ContentHost.h
@@ +149,4 @@
>                              const nsIntRegion& aOldValidRegionBack,
>                              nsIntRegion* aUpdatedRegionBack);
>  
> +  virtual bool EnsureTextureHost(TextureIdentifier aTextureId,

This need some documentation, it's not clear what happens if, say, we call it several times.
(cf. comments on the implementation)

::: gfx/layers/composite/ImageHost.cpp
@@ +24,5 @@
>    }
>  }
>  
> +bool
> +ImageHostSingle::EnsureTextureHost(TextureIdentifier aTextureId,

Same concerns as with ContentHost::EnsureTextureHost

@@ +149,5 @@
>    return GetTextureHost()->IsValid();
>  }
>  
> +bool
> +ImageHostBuffered::EnsureTextureHost(TextureIdentifier aTextureId,

idem

::: gfx/layers/ipc/CompositableTransactionParent.cpp
@@ +31,2 @@
>        ContentHostBase* content = static_cast<ContentHostBase*>(compositableParent->GetCompositableHost());
> + 

Nit: trailing space
Attachment #735609 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #4)
> Comment on attachment 735608 [details] [diff] [review]
> 1 Remove PTexture
> 
> Review of attachment 735608 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/CompositorTypes.h
> @@ +99,5 @@
> > + * the id is unique for any texture owned by a particular compositable.
> > + */
> > +typedef uint32_t TextureIdentifier;
> > +const TextureIdentifier TextureFront = 1;
> > +const TextureIdentifier TextureBack = 2;
> 
> It'd be nice to use an enum instead
> 
I used an int so we could use this for tiles or other compositables with an unbounded number of textures in the future, but this is some way off. Do you prefer an enum in the mean time?

> ::: gfx/layers/client/CanvasClient.cpp
> @@ +40,2 @@
> >  {
> > +  mForwarder->UpdateTexture(this, 1, mTextureClient->LockSurfaceDescriptor());
> 
> lock without unlock in the same scope makes me uncomfortable.
> maybe it needs a GetSurfaceDescriptor, this line doesn't seem to be needing
> the lock/unlock semantic
> 

Yeah, I'll and a Getter, we've (I've) been kind of abusing LockSurfaceDescriptor for some time, it's only a matter of time until it got ridiculous.

> ::: gfx/layers/composite/ContentHost.cpp
> @@ +213,3 @@
> >  {
> > +  MOZ_ASSERT(aTextureId == TextureFront);
> > +  mNewFrontHost = TextureHost::CreateTextureHost(aSurface.type(),
> 
> Shouldn't we handle or assert the case where mNewFrontHost already contains
> something?

No, that is fine, we will do that some times, or at least potentially we could.

> Or perhaps EnsureTextureHost is not the right name and it should be
> something like CreateFrontHost?
> 

It's an overriden method for all compositable hosts, so I can't mention front host. I'm not sure 'create' captures the behaviour any better than 'ensure' in the general case, because it is up to the implementation whether it creates a texture host or not. I'll add some comments.

> ::: gfx/layers/composite/ImageHost.cpp
> @@ +24,5 @@
> >    }
> >  }
> >  
> > +bool
> > +ImageHostSingle::EnsureTextureHost(TextureIdentifier aTextureId,
> 
> Same concerns as with ContentHost::EnsureTextureHost
>
I'll check the multiple calls thing here, I'm not sure for image hosts.
 
> @@ +149,5 @@
> >    return GetTextureHost()->IsValid();
> >  }
> >  
> > +bool
> > +ImageHostBuffered::EnsureTextureHost(TextureIdentifier aTextureId,
> 
> idem
> 
I don't understand this, sorry.


I'll fix everything else...
Attached patch 1 Remove PTexture (obsolete) — Splinter Review
with changes
Attachment #735608 - Attachment is obsolete: true
Attachment #735608 - Flags: review?(nical.bugzilla)
Attachment #736033 - Flags: review?(nical.bugzilla)
(In reply to Nick Cameron [:nrc] from comment #5)
> (In reply to Nicolas Silva [:nical] from comment #4)
> > Comment on attachment 735608 [details] [diff] [review]
> > 1 Remove PTexture
> > 
> > Review of attachment 735608 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/CompositorTypes.h
> > @@ +99,5 @@
> > > + * the id is unique for any texture owned by a particular compositable.
> > > + */
> > > +typedef uint32_t TextureIdentifier;
> > > +const TextureIdentifier TextureFront = 1;
> > > +const TextureIdentifier TextureBack = 2;
> > 
> > It'd be nice to use an enum instead
> > 
> I used an int so we could use this for tiles or other compositables with an
> unbounded number of textures in the future, but this is some way off. Do you
> prefer an enum in the mean time?

No, this is a good reason to use an int, let's keep it like that.


> > ::: gfx/layers/composite/ContentHost.cpp
> > @@ +213,3 @@
> > >  {
> > > +  MOZ_ASSERT(aTextureId == TextureFront);
> > > +  mNewFrontHost = TextureHost::CreateTextureHost(aSurface.type(),
> > 
> > Shouldn't we handle or assert the case where mNewFrontHost already contains
> > something?
> 
> No, that is fine, we will do that some times, or at least potentially we
> could.

Do you mean it is possible to call it several times? If so then the implementation should make sure not to leak it's previously allocated mNewFrontHost.

> I'll check the multiple calls thing here, I'm not sure for image hosts.
>  
> > @@ +149,5 @@
> > >    return GetTextureHost()->IsValid();
> > >  }
> > >  
> > > +bool
> > > +ImageHostBuffered::EnsureTextureHost(TextureIdentifier aTextureId,
> > 
> > idem
> > 
> I don't understand this, sorry.

It means "same thing" I guess only French people use that (latin) word, sorry :)
Attachment #736033 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #7)
> 
> Do you mean it is possible to call it several times? If so then the
> implementation should make sure not to leak it's previously allocated
> mNewFrontHost.
> 

Actually forget about this, it looks like there's no problem.
Attached patch 1 Remove PTexture (obsolete) — Splinter Review
carrying r=nical
Attachment #736033 - Attachment is obsolete: true
Attachment #736085 - Flags: review+
Attached patch 1 Remove PTexture (obsolete) — Splinter Review
Attachment #736085 - Attachment is obsolete: true
Attachment #736087 - Flags: review+
carrying r=nical
Attachment #735609 - Attachment is obsolete: true
Attachment #736088 - Flags: review+
Attached patch 1 Remove PTexture (obsolete) — Splinter Review
Attachment #736087 - Attachment is obsolete: true
Attachment #736568 - Flags: review+
carrying r=nical for both patches
Attachment #736088 - Attachment is obsolete: true
Attachment #736569 - Flags: review+
Attachment #736568 - Attachment is obsolete: true
Attachment #736645 - Flags: review+
Attachment #736569 - Attachment is obsolete: true
Attachment #736646 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f2ab85125928
https://hg.mozilla.org/mozilla-central/rev/7bf4a91d224f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.