+++ This bug was initially created as a clone of Bug #912134 +++ This bug is for a follow up of Bug 912134 Comment 86.

Copy of the relevant comment: Comment on attachment 804019 [details] [diff] [review] [diff] [review] patch v4 - allocate texture per CompositableHost Review of attachment 804019 [details] [diff] [review] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/CompositableHost.h @@ +59,5 @@ > > /** > + * A base class for doing CompositableHost and platform dependent task on TextureHost. > + */ > +class CompositableQuirks : public RefCounted<CompositableQuirks> This class really needs a more explicit name but we can rename it later. Let's not spend time debating names now, this looks like an important patch. ::: gfx/layers/ipc/CompositableTransactionParent.cpp @@ +242,5 @@ > + // set CompositableQuirks > + // on gonk, create EGLImage if possible. > + // create EGLImage during buffer swap could reduce the graphic driver's task > + // during rendering. > + tex->SetCompositableQuirks(compositable->GetCompositableQuirks()); It looks like this call should be in AddTextureHost ::: gfx/layers/opengl/TextureHostOGL.cpp @@ +1122,2 @@ > DeleteTextures(); > +#if 1 Remove the useless #if 1 before landing @@ +1156,5 @@ > MOZ_ASSERT(gl()); > gl()->MakeCurrent(); > > + mQuirks->SetCompositor(mCompositor); > + GLuint tex = static_cast<CompositableQuirksGonkOGL*>(mQuirks.get())->GetTexture(); I see a pattern of doing the two lines above in many places. You should wrap this in a GetGLTexture() method in the Gralloc TextureHost classes.

CompositableQuirks' role is to hold backend specific textures at the level of Compositable (which is backend independent). How about naming it "CompositableTextureSlot" or "CompositableTextureContainer"?

Do we want to hardcode the idea that it's only for temporary textures (as it currently is) or could we leave it more vague, like CompositableBackendSpecificData ?

I am worried that if we introduce the idea of vague "user data", people will be tempted to use it for dirty hacks and that it will be tempting to fix a problem that is first seen on a given platform there, rather than fixing the problem at the platform-independent level which is always a better solution. If right now we only need it for temporary textures, I would prefer being strict and explicit about what it is meant to do.

OK. But on the other hand, CompositableTextureContainer seems confusing because it will sound like this is where all textures used for all compositables should go. So it seems nontrivial to find a specific name that does more good than harm. CompositableBackendSpecificTemporaryTexture is a bit long...

Probably the 3 most simple and low risk patches that I ever wrote

Comment on attachment 805263 [details] [diff] [review] remove #if 1 Review of attachment 805263 [details] [diff] [review]: ----------------------------------------------------------------- * thinks very, very hard ... * OK, this should work.

(In reply to Benoit Jacob [:bjacob] from comment #5) > OK. But on the other hand, CompositableTextureContainer seems confusing > because it will sound like this is where all textures used for all > compositables should go. So it seems nontrivial to find a specific name that > does more good than harm. CompositableBackendSpecificTemporaryTexture is a > bit long... That is true. CompositableBackendSpecificData is already a good improvement over CompositableQuirks so it's fine by me (and not worth spending time arguing over).

Comment on attachment 805266 [details] [diff] [review] Avoid some code dupplication in gralloc TextureHost Looks good. Thanks.

https://hg.mozilla.org/integration/b2g-inbound/rev/362ba1a32ec8 https://hg.mozilla.org/integration/b2g-inbound/rev/be3d833564fb https://hg.mozilla.org/integration/b2g-inbound/rev/2f8b5a47a1b8

https://hg.mozilla.org/mozilla-central/rev/362ba1a32ec8 https://hg.mozilla.org/mozilla-central/rev/be3d833564fb https://hg.mozilla.org/mozilla-central/rev/2f8b5a47a1b8

Comment on attachment 811948 [details] [diff] [review] Rename CompositableQuirks Review of attachment 811948 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/CompositableHost.h @@ +291,5 @@ > protected: > TextureInfo mTextureInfo; > Compositor* mCompositor; > Layer* mLayer; > + RefPtr<CompositableBackendSpecificData> mQuirks; Should not be named nQuirks anymore, then! ::: gfx/layers/composite/TextureHost.h @@ +392,5 @@ > protected: > uint64_t mID; > RefPtr<TextureHost> mNextTexture; > TextureFlags mFlags; > + RefPtr<CompositableBackendSpecificData> mQuirks; Same ::: gfx/layers/opengl/GrallocTextureHost.cpp @@ +331,4 @@ > } > > void > +GrallocTextureHostOGL::SetCompositableBackendSpecificData(CompositableBackendSpecificData* aQuirks) Same

Fixed the remaining occurences of "quirk" and landed https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b561566a3e

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f05f70b425c https://hg.mozilla.org/releases/mozilla-aurora/rev/844e6581a968 https://hg.mozilla.org/releases/mozilla-aurora/rev/9e8d832f13b8 https://hg.mozilla.org/releases/mozilla-aurora/rev/65523793a6bf