Do you have a plan for this? Any extra API you want to add to the new texture clients/hosts? Or anything you want to specifically avoid? I could start looking at this I think - it is probably the best use of my time, of which I will have bits of in between debugging WinOMTC.

(In reply to Nick Cameron [:nrc] from comment #1) > Do you have a plan for this? Any extra API you want to add to the new > texture clients/hosts? Or anything you want to specifically avoid? > > I could start looking at this I think - it is probably the best use of my > time, of which I will have bits of in between debugging WinOMTC. Great news! I have no specific plans (except that I really want to convert all derpecated textures to the new stuff). We talked about adding a OpUseComponentAlphaTexture with the pair {onWhite, onBlack} to use instead of OpUseTexture when it applies, other than that I have not thought of any protocol changes. Overall I'd like the protocol to remain small if possible. For tiling I expect we'll have to add a bit of protocol too. The thing to whatch out for is gralloc: since buffer rotation requires synchronization between the front and back buffers, we should be careful about avoiding Locking the buffer to do the synchronization on the client side while Locking the buffer for compositing on the host side. I mention this because it's typically what we don't think about when writing code for any non-b2g platform, and then we find out later that genlock failures are creeping up. I am not sure if you mean to work on buffer rotation or tiling, but one of our goals is to move to tiling at some point maybe soon-ish, so maybe you should have a chat with BenWa to make sure we are not killing buffer rotation soon after you spent time refreshing it! More generally, I would like to keep avoid apis and rules that are specific to certain kinds of layers. I think the new textures are flexible enough for that. If we need to change the new textures API it's possible but we should do it so it applies to all uses of the textures (I am trying to avoid the situation we had in deprectaed texture where double buffering was handle by the texture in image layers and by the compositable in thebes layers, and where the same API was not doing the same things depending on the image vs thebes) Also I'd like to make sure things work the same way on B2G and elsewhere, so let's keep things cross-process and gralloc friendly as much as we can.

And remember that our current B2G testing (including, but not limited to performance) is not catching all the issues, so these types of changes would probably have to include manual testing...

Sorry, forgot to assign this to me earlier. I'm well into this.

Just for the record, I'm not planning on touching tiled content client/hosts here at all.

try push: https://tbpl.mozilla.org/?tree=Try&rev=614751e4bef9 windows: https://tbpl.mozilla.org/?tree=Try&rev=08ed44c0724b

(In reply to Nick Cameron [:nrc] from comment #6) > try push: https://tbpl.mozilla.org/?tree=Try&rev=614751e4bef9 > windows: https://tbpl.mozilla.org/?tree=Try&rev=08ed44c0724b Seems to be some issues on B2G ICS Emulator :-(

Comment on attachment 829052 [details] [diff] [review] Patch 5: Content clients Review of attachment 829052 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ContentClient.cpp @@ +177,5 @@ > + return false; > + } > + } > + > + MOZ_ASSERT(aClient->IsValid()); Do we really want to crash if aClient is not valid? @@ +203,5 @@ > + !AddTextureClient(mTextureClient)) { > + AbortTextureClientCreation(); > + return; > + } > + nit: trailing spaces, there are others in this patch @@ +237,5 @@ > + > +nsIntRegion > +ContentClientRemoteBufferNew::GetUpdatedRegion(const nsIntRegion& aRegionToDraw, > + const nsIntRegion& aVisibleRegion, > + bool aDidSelfCopy) nit: indentation mismatch @@ +264,5 @@ > + > +void > +ContentClientRemoteBufferNew::Updated(const nsIntRegion& aRegionToDraw, > + const nsIntRegion& aVisibleRegion, > + bool aDidSelfCopy) nit: indentation

Comment on attachment 829053 [details] [diff] [review] Patch 6: Content hosts Review of attachment 829053 [details] [diff] [review]: ----------------------------------------------------------------- Watch out for trailling spaces and indentation mismatches in the parameters of method implementations. ::: gfx/layers/composite/CompositableHost.h @@ +298,5 @@ > + // texture host dies. > + virtual void RemoveTextureHostDeferred(TextureHost* aTexture) > + { > + MOZ_CRASH("Compositable was not expecting to handle deferred removal of texture hosts"); > + } I'd prefer that we have only one RemoveTextureHost method that checks for aTexture->GetFlags and handle deferred/non-deferred. Soon CompositableHost will not have RemoveTextureHost at all and deallocation will be handled entirely by the Texture. I am okay with fixing that in a followup but we really gotta do it before we keep going it this direction. ::: gfx/layers/composite/ContentHost.cpp @@ +157,5 @@ > + } > + > + nsIntRegion region(*renderRegion); > + nsIntPoint origin = GetOriginOffset(); > + region.MoveBy(-origin); // translate into TexImage space, buffer origin might not be at texture (0,0) please place this comment above the concerned line. @@ +257,5 @@ > + DiagnosticTypes diagnostics = DIAGNOSTIC_CONTENT | DIAGNOSTIC_BIGIMAGE; > + diagnostics |= iterOnWhite ? DIAGNOSTIC_COMPONENT_ALPHA : 0; > + GetCompositor()->DrawDiagnostics(diagnostics, rect, aClipRect, > + aTransform, aOffset); > + } nit: the indentation of this block is wrong by 2 spaces @@ +308,5 @@ > +#ifdef MOZ_DUMP_PAINTING > +void > +ContentHostBaseNew::Dump(FILE* aFile, > + const char* aPrefix, > + bool aDumpHtml) nit: indentation mismatch @@ +720,5 @@ > + } > + > + // We don't need to calculate an update region because we assume that if we > + // are using double buffering then we have render-to-texture and thus no > + // upload to do. This really needs to be documented in ContentClientDoubleBufferedNew's class documentation! ::: gfx/layers/composite/ContentHost.h @@ +112,5 @@ > + > + virtual void SetCompositor(Compositor* aCompositor) MOZ_OVERRIDE; > + > +#ifdef MOZ_DUMP_PAINTING > + virtual already_AddRefed<gfxImageSurface> GetAsSurface(); nit: MOZ_OVERRIDE @@ +119,5 @@ > + const char* aPrefix="", > + bool aDumpHtml=false) MOZ_OVERRIDE; > +#endif > +#ifdef MOZ_LAYERS_HAVE_LOG > + virtual void PrintInfo(nsACString& aTo, const char* aPrefix); nit: MOZ_OVERRIDE

Comment on attachment 829046 [details] [diff] [review] Patch 1: Add AccessMode to TextureClient Review of attachment 829046 [details] [diff] [review]: ----------------------------------------------------------------- Please document what this access mode is, how it relates to the OpenMode in TextureClient::Lock, and the texture being marked as immutable. It is not shared with the texture host. I am not convinced it belongs to TextureClient. It looks like it is only a convenience for ContentClient and should be an external state stored directly in ContentClient instead. I could probably be convinced otherwise.

Comment on attachment 829047 [details] [diff] [review] Patch 2: Rename GetTextureHost to GetAsTextureHost Review of attachment 829047 [details] [diff] [review]: ----------------------------------------------------------------- The whole GetTextureHost thing is a bit dodgy in the first place, because it doesn't always make sense for a compositable to expose only one texture. This makes it look even more like a cast, why do we want this? It looks to me like we should not be exposing the TextureHost in the first place, and expose directly the information we actually care about outside of CompositableHost (the size for instance).

A lot of the failing reftests get graphical glitches when a 90 degrees rotation is applied to the element like so: -moz-transform: rotate(90deg);

Oh, I think I know what's up: Creating a TextureClient does not clear the allocated buffer (Deprecated ones do, but no the new ones). So the transparent parts get random garbage.

With this patch I can't reproduce the glitches anymore \o/

Maybe we should be smarter and only clear when the SurfaceFormat has an alpha channel (deprecated textures always clear so this won't be a regression).

try push https://tbpl.mozilla.org/?tree=Try&rev=ae02d92e101c

(In reply to Nicolas Silva [:nical] from comment #22) > Maybe we should be smarter and only clear when the SurfaceFormat has an > alpha channel (deprecated textures always clear so this won't be a > regression). If this is a "refactoring", you don't have a choice - you can't willingly change the behaviour of the code :)

(In reply to Nicolas Silva [:nical] from comment #23) > try push https://tbpl.mozilla.org/?tree=Try&rev=ae02d92e101c All green - nice! And thanks!

Comment on attachment 829391 [details] [diff] [review] clear the texture clients when creating it in ContentClient Review of attachment 829391 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer the clear happens in the texture client - perhaps AllocateForSurface should do it? Content is the only one to use that method so it could always clear. Alternatively, you could pass a flag to that method or to some other creation method. My reasoning is that one day perhaps we change or add some other compositable which requires clearing and some poor dev is going to debug this exact same bug, whereas if there is a flag or it is always cleared then it is more obvious that the TextureClient is provided without a clear. ::: gfx/layers/client/ContentClient.cpp @@ +180,5 @@ > > + if (aClient->Lock(OPEN_READ_WRITE)) { > + RefPtr<DrawTarget> dt = aClient->AsTextureClientDrawTarget()->GetAsDrawTarget(); > + gfx::IntSize size = dt->GetSize(); > + dt->ClearRect(gfx::Rect(0.0, 0.0, size.width, size.height)); we shouldn't need the gfx:: here, add a |using namespace| instead if we do Rect takes float params, so prefer 0.0f I kind of think you should just lock and assume it succeeds. Perhaps use a DebugOnly<bool> and assert success. Because if we can't lock it directly after creating it we are pretty screwed. And using an if statement suggests that that is a reasonable possibility.

(In reply to Nicolas Silva [:nical] from comment #16) > Comment on attachment 829053 [details] [diff] [review] > Patch 6: Content hosts > > Review of attachment 829053 [details] [diff] [review]: > ----------------------------------------------------------------- > > Watch out for trailling spaces and indentation mismatches in the parameters > of method implementations. > So a lot of the indentation mismatch is because I indented assuming the 'New' postfix would be removed in the not too distant future and to align according to that to make it easier to do the renaming. I also thought that maybe I should remove the 'New' now and add 'Deprecated' to the old versions to be consistent with other places. Do you have an opinion on that?

(In reply to Nicolas Silva [:nical] from comment #17) > Comment on attachment 829046 [details] [diff] [review] > Patch 1: Add AccessMode to TextureClient > > Review of attachment 829046 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please document what this access mode is, how it relates to the OpenMode in > TextureClient::Lock, and the texture being marked as immutable. It is not > shared with the texture host. I am not convinced it belongs to > TextureClient. It looks like it is only a convenience for ContentClient and > should be an external state stored directly in ContentClient instead. I > could probably be convinced otherwise. Yeah, I was kind of wandering about this. At the moment, as far as I can tell OpenMode is a total fiction - it is just ignored everywhere. AccessMode is only used for debugging at the moment. I should check why it was passed to the compositor before, but now doesn't need to be. It is in the TextureClient because there is an access mode per texture not per compositable, so putting it in ContentClient does not make sense. Would you be happier if I made it always |ifdef DEBUG|?

Comment on attachment 829048 [details] [diff] [review] Patch 3: Add a path to RotatedContentBuffer for new textures Review of attachment 829048 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/RotatedBuffer.cpp @@ +285,5 @@ > + } else if (mDTBuffer) { > + format = mDTBuffer->GetFormat(); > + } > + > + switch (format) { Can we replace this switch with ContentForFormat from gfx2DGlue.h please. @@ +323,5 @@ > RotatedContentBuffer::EnsureBufferOnWhite() > { > if (!mDTBufferOnWhite && mBufferProviderOnWhite) { > mDTBufferOnWhite = mBufferProviderOnWhite->LockDrawTarget(); > } I think these 3 lines are meant to go away. ::: gfx/layers/RotatedBuffer.h @@ +300,5 @@ > > mBufferProvider = aClient; > if (!mBufferProvider) { > mDTBuffer = nullptr; > + mNewBufferProvider = nullptr; I don't think you need this, clearing the old-textures buffer provider shouldn't need to clear the new-textures one too. You could probably just assert that mNewBufferProvider is null, we should need mix texture types with a single RCB. Same goes for the other 3 cases like this. @@ +330,5 @@ > + mDTBuffer = nullptr; > + mBufferProvider = nullptr; > + } > + } > + Trailing whitespace, here and in a few other places (in both files).

(In reply to Milan Sreckovic [:milan] from comment #24) > (In reply to Nicolas Silva [:nical] from comment #22) > > Maybe we should be smarter and only clear when the SurfaceFormat has an > > alpha channel (deprecated textures always clear so this won't be a > > regression). > > If this is a "refactoring", you don't have a choice - you can't willingly > change the behaviour of the code :) Yes, I threw the idea there so that we could think about doing it as a followup if we think it's useful. Clearing an entire thebes layer just before painting all of its pixels with opaque content is certainly wasteful.

(In reply to Nick Cameron [:nrc] from comment #27) > So a lot of the indentation mismatch is because I indented assuming the > 'New' postfix would be removed in the not too distant future and to align > according to that to make it easier to do the renaming. I also thought that > maybe I should remove the 'New' now and add 'Deprecated' to the old versions > to be consistent with other places. Do you have an opinion on that? I think we should! Make the new ones exactly the way we want them, and push horrible things like 'Deprecated' onto the old ones to strengthen our resolve to get rid of them.

(In reply to Nicolas Silva [:nical] from comment #30) > Yes, I threw the idea there so that we could think about doing it as a > followup if we think it's useful. Clearing an entire thebes layer just > before painting all of its pixels with opaque content is certainly wasteful. It's also a regression that happened at some point. We definitely didn't do that for main-thread layers.

(In reply to Nick Cameron [:nrc] from comment #26) > Comment on attachment 829391 [details] [diff] [review] > clear the texture clients when creating it in ContentClient > > Review of attachment 829391 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would prefer the clear happens in the texture client - perhaps > AllocateForSurface should do it? Content is the only one to use that method > so it could always clear. I'd prefer have this kind of things explicit. Especially with something that may affect performances like this. > Alternatively, you could pass a flag to that > method or to some other creation method. Yeah, I was going to do that and hesitated and ended up going for the simplest patch. > My reasoning is that one day > perhaps we change or add some other compositable which requires clearing and > some poor dev is going to debug this exact same bug, whereas if there is a > flag or it is always cleared then it is more obvious that the TextureClient > is provided without a clear. I agree. We should not sacrifice performance to ease of use, so I am not found of always clearing but I'll add a flag for it and clear in the BufferTextureClient implementations. The patch won't be as nice and small though.

(In reply to Matt Woodrow (:mattwoodrow) from comment #31) > (In reply to Nick Cameron [:nrc] from comment #27) > > So a lot of the indentation mismatch is because I indented assuming the > > 'New' postfix would be removed in the not too distant future and to align > > according to that to make it easier to do the renaming. I also thought that > > maybe I should remove the 'New' now and add 'Deprecated' to the old versions > > to be consistent with other places. Do you have an opinion on that? > > I think we should! Make the new ones exactly the way we want them, and push > horrible things like 'Deprecated' onto the old ones to strengthen our > resolve to get rid of them. Agreed

Rename Content* to DeprecatedContent* and Content*New to Content*. Sorts any indentation issues.

(Post-qref, this time). Sorry for the bug-spam.

needinfo for comment 28

(In reply to Nicolas Silva [:nical] from comment #18) > Comment on attachment 829047 [details] [diff] [review] > Patch 2: Rename GetTextureHost to GetAsTextureHost > > Review of attachment 829047 [details] [diff] [review]: > ----------------------------------------------------------------- > > The whole GetTextureHost thing is a bit dodgy in the first place, because it > doesn't always make sense for a compositable to expose only one texture. > This makes it look even more like a cast, why do we want this? It looks to > me like we should not be exposing the TextureHost in the first place, and > expose directly the information we actually care about outside of > CompositableHost (the size for instance). This is for when we want to use the image data from compositable somewhere other than just compositing it. At the moment it is only used for mask layers - we call GetAsTextureHost and to get the mask data as a texture for use as a mask rather than to composite the mask layer to the screen. I guess it might be more appropriate to use a texture source with new textures, but other than that I think the API is appropriate, if not well-documented.

Comment on attachment 829391 [details] [diff] [review] clear the texture clients when creating it in ContentClient Clearing the review to stop the reminders and since nical is preparing a new patch.

Slightly updated

(In reply to Nick Cameron [:nrc] from comment #42) > Created attachment 830557 [details] [diff] [review] > Patch 5: Content clients > > Slightly updated TBH, this probably doesn't need re-review. I just fixed a couple of small bugs in how we use mFrontAndBackBufferDiffer and mIsNewBuffer. If you grep for those in the patch you'll see the changes.

Comment on attachment 830043 [details] [diff] [review] patch 6.1: Get rid of RemoveTextureHostDeferred Review of attachment 830043 [details] [diff] [review]: ----------------------------------------------------------------- nice

(In reply to Nick Cameron [:nrc] from comment #28) > (In reply to Nicolas Silva [:nical] from comment #17) > > Comment on attachment 829046 [details] [diff] [review] > > Patch 1: Add AccessMode to TextureClient > > > > Review of attachment 829046 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Please document what this access mode is, how it relates to the OpenMode in > > TextureClient::Lock, and the texture being marked as immutable. It is not > > shared with the texture host. I am not convinced it belongs to > > TextureClient. It looks like it is only a convenience for ContentClient and > > should be an external state stored directly in ContentClient instead. I > > could probably be convinced otherwise. > > Yeah, I was kind of wandering about this. At the moment, as far as I can > tell OpenMode is a total fiction - it is just ignored everywhere. AccessMode > is only used for debugging at the moment. I should check why it was passed > to the compositor before, but now doesn't need to be. It is in the > TextureClient because there is an access mode per texture not per > compositable, so putting it in ContentClient does not make sense. Would you > be happier if I made it always |ifdef DEBUG|? I want TextureClient to be as small as possible because I want every piece of code that holds reference to data shared with the compositor to use it. Some people are already worried that TextureClient (even just the name of the class) infers something overkill when they just want to hold on to a shmem or a gralloc buffer. So I think facilities like this one that are only useful to one user of TextureClient, and do not affect the behavior of TextureClient should be taken out of TextureClient. If it is only used in ContentClient, then it makes sense for me that ContentClient gets members like mTextureState, mTextureStateOnWhite, mBackTextureState, etc. It adds a bit of bloat in one place and removes some from the class that should be used in many places.

(In reply to Nick Cameron [:nrc] from comment #39) > (In reply to Nicolas Silva [:nical] from comment #18) > > Comment on attachment 829047 [details] [diff] [review] > > Patch 2: Rename GetTextureHost to GetAsTextureHost > > > > Review of attachment 829047 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > The whole GetTextureHost thing is a bit dodgy in the first place, because it > > doesn't always make sense for a compositable to expose only one texture. > > This makes it look even more like a cast, why do we want this? It looks to > > me like we should not be exposing the TextureHost in the first place, and > > expose directly the information we actually care about outside of > > CompositableHost (the size for instance). > > This is for when we want to use the image data from compositable somewhere > other than just compositing it. At the moment it is only used for mask > layers - we call GetAsTextureHost and to get the mask data as a texture for > use as a mask rather than to composite the mask layer to the screen. I guess > it might be more appropriate to use a texture source with new textures, but > other than that I think the API is appropriate, if not well-documented. Ah, fair enough. It would be nice that the mask layer use case be documented in GetAsTextureHost and I agree that a TextureSource would be better (but let's see to that part later)

Comment on attachment 829047 [details] [diff] [review] Patch 2: Rename GetTextureHost to GetAsTextureHost Review of attachment 829047 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/CompositableHost.h @@ +201,5 @@ > > virtual DeprecatedTextureHost* GetDeprecatedTextureHost() { return nullptr; } > > /** > * Returns the front buffer. please add a little comment about what this is useful for.

Comment on attachment 829046 [details] [diff] [review] Patch 1: Add AccessMode to TextureClient Review of attachment 829046 [details] [diff] [review]: ----------------------------------------------------------------- Still not convinced about this part.

This version of the fix implements clearing the buffer as an optional flag in BufferTextureClient::AllocateForSurface (instead of clearing externally through moz2d as the previous patch did.

(In reply to Nicolas Silva [:nical] from comment #48) > Comment on attachment 829046 [details] [diff] [review] > Patch 1: Add AccessMode to TextureClient > > Review of attachment 829046 [details] [diff] [review]: > ----------------------------------------------------------------- > > Still not convinced about this part. OK, how about I just get rid of the AccessMode stuff completely. It's only used for debugging now, and I'm not sure it gives us a lot of benefit. I would feel bad about polluting ContentClients with so much extra state just for some questonable debugging aid.

Comment on attachment 829391 [details] [diff] [review] clear the texture clients when creating it in ContentClient This is obsoleted by the new patch, I believe.

Comment on attachment 830785 [details] [diff] [review] Add a flag In BufferTextureClient to clear the buffer when allocating Review of attachment 830785 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I'll land this with the rest of the patches (once the massive chain of blockers evaporates :-( ).

(In reply to Nick Cameron [:nrc] from comment #50) > OK, how about I just get rid of the AccessMode stuff completely. It's only > used for debugging now, and I'm not sure it gives us a lot of benefit. I > would feel bad about polluting ContentClients with so much extra state just > for some questonable debugging aid. That's probably ok. It was added because the ContentClient has physical access to multiple TextureClient, but some of them are in use by the compositor and shouldn't be written to. It was meant to protect against silly bugs there, and assert if we tried to use the textures wrong.

(In reply to Nick Cameron [:nrc] from comment #50) > OK, how about I just get rid of the AccessMode stuff completely. It's only > used for debugging now, and I'm not sure it gives us a lot of benefit. I > would feel bad about polluting ContentClients with so much extra state just > for some questonable debugging aid. Works for me.

r=me because just removing code from earlier patches as discussed in above comments.

Comment on attachment 8333549 [details] [diff] [review] patch 10: remove access mode stuff Whoops! Little bit keen there, need to check those TODOs first.

Mostly removing stuff.

And just to check we're still good on try: https://tbpl.mozilla.org/?tree=Try&rev=213668ded258

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f23d703959e https://hg.mozilla.org/integration/mozilla-inbound/rev/e13baf7467aa https://hg.mozilla.org/integration/mozilla-inbound/rev/71dd1c97a886 https://hg.mozilla.org/integration/mozilla-inbound/rev/6826ab279dfe https://hg.mozilla.org/integration/mozilla-inbound/rev/1621be075765 https://hg.mozilla.org/integration/mozilla-inbound/rev/0869ef3e4bd5 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c21d59e99af https://hg.mozilla.org/integration/mozilla-inbound/rev/05abcd9325c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/1a704641cba8 https://hg.mozilla.org/integration/mozilla-inbound/rev/983efc0db5dd https://hg.mozilla.org/integration/mozilla-inbound/rev/c444703cff15

and backed out for b2g bustage :-( https://hg.mozilla.org/integration/mozilla-inbound/rev/d49f9fd30c02 https://hg.mozilla.org/integration/mozilla-inbound/rev/c00e4acb093e https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd24384fbb8

Nical: I'm away for a few days, if you want to investigate the b2g orange to get this landed quicker, I'm totally cool with that ;-) If you'd rather not (and tbh I wouldn't blame you), I'll look into it on Tuesday. I suspect we might just need to make the fuzz more generous since I think (haven't really investigated) all the failing tests are already fuzzed, but not enough. We should check that there isn't something fundamentally flawed too, of course.

(In reply to Nick Cameron [:nrc][PTO until Dec 3rd] from comment #62) > Nical: I'm away for a few days, if you want to investigate the b2g orange to > get this landed quicker, I'm totally cool with that ;-) If you'd rather not > (and tbh I wouldn't blame you), I'll look into it on Tuesday. I suspect we > might just need to make the fuzz more generous since I think (haven't really > investigated) all the failing tests are already fuzzed, but not enough. We > should check that there isn't something fundamentally flawed too, of course. OK, I'll look into that as soon as the new D3D textures are green on try.

Nick, I think you landed your stuff without attachment 830785 [details] [diff] [review] which fixes those reftests :p

(In reply to Nicolas Silva [:nical] from comment #64) > Nick, I think you landed your stuff without attachment 830785 [details] [diff] [review] > [diff] [review] which fixes those reftests :p d'oh. I'll try again. Thanks!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=22385c303fce

(In reply to Nick Cameron [:nrc][PTO until Dec 3rd] from comment #66) > Try push: https://tbpl.mozilla.org/?tree=Try&rev=22385c303fce It looks good except for attachment 830785 [details] [diff] [review] in which I forgot to add the parameter in GrallocTextureClient::AllocateForSurface

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d370b1d204c https://hg.mozilla.org/integration/mozilla-inbound/rev/1c38d1531703 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d7c2a559865 https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1266b12137 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ab7774da1a https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3f0bab5d09 https://hg.mozilla.org/integration/mozilla-inbound/rev/49788093a6e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/cecf614ac8bf https://hg.mozilla.org/integration/mozilla-inbound/rev/e6fd8fac8ddc https://hg.mozilla.org/integration/mozilla-inbound/rev/cc42ffb0d95a https://hg.mozilla.org/integration/mozilla-inbound/rev/00defb97dd54 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c112718a04

https://hg.mozilla.org/mozilla-central/rev/4d370b1d204c https://hg.mozilla.org/mozilla-central/rev/1c38d1531703 https://hg.mozilla.org/mozilla-central/rev/3d7c2a559865 https://hg.mozilla.org/mozilla-central/rev/9f1266b12137 https://hg.mozilla.org/mozilla-central/rev/e3ab7774da1a https://hg.mozilla.org/mozilla-central/rev/7f3f0bab5d09 https://hg.mozilla.org/mozilla-central/rev/49788093a6e7 https://hg.mozilla.org/mozilla-central/rev/cecf614ac8bf https://hg.mozilla.org/mozilla-central/rev/e6fd8fac8ddc https://hg.mozilla.org/mozilla-central/rev/cc42ffb0d95a https://hg.mozilla.org/mozilla-central/rev/00defb97dd54 https://hg.mozilla.org/mozilla-central/rev/f5c112718a04

Backed out for accidentally disabling gralloc for thebes layers on B2G, as reported by nical: https://hg.mozilla.org/mozilla-central/rev/526e12792fc8

Bug 908196 and the following seems related part for disabling gralloc. - CompositableClient::CreateBufferTextureClient() http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/CompositableClient.cpp#199

(In reply to Sotaro Ikeda [:sotaro] from comment #71) > Bug 908196 and the following seems related part for disabling gralloc. > - CompositableClient::CreateBufferTextureClient() > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ > CompositableClient.cpp#199 Yes, I wouldn't block on bug 908196 thoug. CompositableClient::CreateTextureClientForDrawing could return a GrallocTextureClient in the case of B2G, even if it would be nice to have 908196 fixed as well (we would be using grallc in more cases for video).

I locally enabled gralloc buffer. But screen was always black. DrawTarget creation was failed at ContentClientRemoteBuffer::CreateBuffer(). Try to get a mapped buffer without locking gralloc buffer. http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ContentClient.cpp#205

In old texture(DeprecatecTexture) era, thebes layer's buffer locking is handled at ContentClient. Same thing needs to be done on new texture.

(In reply to Sotaro Ikeda [:sotaro] from comment #73) > I locally enabled gralloc buffer. But screen was always black. DrawTarget > creation was failed at ContentClientRemoteBuffer::CreateBuffer(). Try to get > a mapped buffer without locking gralloc buffer. > > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ > ContentClient.cpp#205 How did you do this? It was not clear to me how we should be using gralloc texture clients from a compositable - we don't do it anywhere and there is a bunch of missing code.

(In reply to Sotaro Ikeda [:sotaro] from comment #74) > In old texture(DeprecatecTexture) era, thebes layer's buffer locking is > handled at ContentClient. Same thing needs to be done on new texture. It seem incorrect, when a SurfaceDescriptor is opened, ShadowLayerForwarder::PlatformOpenDescriptor() lock the gralloc buffer. http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#462

(In reply to Nick Cameron [:nrc] from comment #75) > How did you do this? It was not clear to me how we should be using gralloc > texture clients from a compositable - we don't do it anywhere and there is a > bunch of missing code. Sorry, Comment 74 seems incorrect. As in Comment 76, gralloc buffer is locked when SurfaceDescriptor is oppen.

old days, gralloc buffers are lockked like following. DeprecatedContentClientRemoteBuffer::CreateBuffer() ->DeprecatedTextureClientShmem::LockDrawTarget() ->DeprecatedTextureClientShmem::GetSurface() ->ShadowLayerForwarder::OpenDescriptor() ->GraphicBuffer::lock()

(In reply to Milan Sreckovic [:milan] from comment #3) > And remember that our current B2G testing (including, but not limited to > performance) is not catching all the issues, so these types of changes would > probably have to include manual testing... That was a good prediction for what happened today!

(In reply to Benoit Jacob [:bjacob] from comment #79) > (In reply to Milan Sreckovic [:milan] from comment #3) > > And remember that our current B2G testing (including, but not limited to > > performance) is not catching all the issues, so these types of changes would > > probably have to include manual testing... > > That was a good prediction for what happened today! And note that we wouldn't even have caught this with the usual manual testing, would need to have been debugging, looking specifically for use of gralloc.

Comment on attachment 8342684 [details] [diff] [review] disable new textures for b2g Review of attachment 8342684 [details] [diff] [review]: ----------------------------------------------------------------- I would like to understand: This patch makes no difference on B2G whatsoever, right? And only forces new-layers outside of B2G? Does it _replace_ a preexisting patch in your queue, that was turning on new-layers for B2G? If yes please obsolete that patch.

Comment on attachment 8342684 [details] [diff] [review] disable new textures for b2g Review of attachment 8342684 [details] [diff] [review]: ----------------------------------------------------------------- r+ because this doesn't make anything worse by itself. But I don't understand at the moment what is the complete set of patches that is being considered for pushing at the moment.

(In reply to Benoit Jacob [:bjacob] from comment #82) > Comment on attachment 8342684 [details] [diff] [review] > disable new textures for b2g > > Review of attachment 8342684 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would like to understand: > > This patch makes no difference on B2G whatsoever, right? And only forces > new-layers outside of B2G? > Yes, that is the idea. > Does it _replace_ a preexisting patch in your queue, that was turning on > new-layers for B2G? If yes please obsolete that patch. No, it sits on top of the patch queue. There is a patch which turns new textures content on for all platforms, and this sits on top of that, making b2g an exception.

I'm not in a horrible rush to land this (as in the patch queue), but I think it is a good idea to land sooner rather than later. So, I'll wait for tomorrow morning to make sure there are no objections from bjacob or others in TO. As far as I can see, this will not affect b2g at all, the risk is if the last patch is wrong in some way, but that risk seems small to me since it is a small, simple patch.

For lack of a better place to note this, the landing on 12/3 showed a ~50ms improvement in launch time for apps on b2g. The backout on 12/4 showed a similar regression of ~50ms.

I locally confirmed the new texture are partially working on gonk. I am going to create a bug to enable it on gonk.

Set 1.3?, because this bug blocks Bug 925444.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e796e364815 https://hg.mozilla.org/integration/mozilla-inbound/rev/10932f3a0ba0 (Landed as a roll-up patch + patch 13)

Disabled on b2g and Windows (r=nical via irc for the latter bit).

https://hg.mozilla.org/mozilla-central/rev/0e796e364815 https://hg.mozilla.org/mozilla-central/rev/10932f3a0ba0

1.3+ given that in 1.3 our target phone will be JB supported.

This is a really large change. It may be worth delaying the uplift a bit, until we have a full solution to bug 925444 that needs this change. All indications are that will be the case, but delaying the uplift for a week is probably worth it.

This bug's patches are commited before v1.3 branch. There's no need to uplift them. But "new textures on ContentClient/ContentHost" is disabled on v1.3 now. The new texture are going to be enabled on gonk by Bug 946720.

> mTextureInfo.mTextureFlags = (aFlags & ~TEXTURE_DEALLOCATE_CLIENT) | > TEXTURE_DEALLOCATE_DEFERRED; Why does this matter? Does this mean we can't do DEALLOCATE_CLIENT for content textures?