844928 - [layers-refactoring] remove the PTexture protocol

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

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.

Comment 1

[Nick Cameron [:nrc]]

Got started on this today.

Comment 2

[Nick Cameron [:nrc]]

Attachment: 1 Remove PTexture

Comment 3

[Nick Cameron [:nrc]]

Attachment: 2 Smarten up content client/host texture creation

These will def. need a try push because of the change to LayerManager::Destroy

Comment 4

[Nicolas Silva [:nical]]

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

Comment 5

[Nick Cameron [:nrc]]

(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...

Comment 6

[Nick Cameron [:nrc]]

Attachment: 1 Remove PTexture

with changes

Comment 7

[Nicolas Silva [:nical]]

(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 :)

Comment 8

[Nicolas Silva [:nical]]

(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.

Comment 9

[Nick Cameron [:nrc]]

Attachment: 1 Remove PTexture

carrying r=nical

Comment 10

[Nick Cameron [:nrc]]

Attachment: 1 Remove PTexture

Comment 11

[Nick Cameron [:nrc]]

Attachment: 2 Smarten up content client/host texture creation

carrying r=nical

Comment 12

[Nick Cameron [:nrc]]

Attachment: 1 Remove PTexture

Comment 13

[Nick Cameron [:nrc]]

Attachment: 2 Smarten up content client/host texture creation

carrying r=nical for both patches

Comment 14

[Nick Cameron [:nrc]]

Attachment: 1 Remove PTexture

Comment 15

[Nick Cameron [:nrc]]

Attachment: 2 Smarten up content client/host texture creation

Comment 16

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5aa2cfa4fcc8

Comment 17

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ab85125928
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf4a91d224f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2ab85125928
https://hg.mozilla.org/mozilla-central/rev/7bf4a91d224f