893301 - Convert ContentClient/ContentHost classes to the new textures

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Nick Cameron [:nrc]]

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.

Comment 2

[Nicolas Silva [:nical]]

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

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 4

[Nick Cameron [:nrc]]

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

Comment 5

[Nick Cameron [:nrc]]

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

Comment 6

[Nick Cameron [:nrc]]

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

Comment 7

[Nick Cameron [:nrc]]

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

[Nick Cameron [:nrc]]

Attachment: Patch 1: Add AccessMode to TextureClient

Comment 9

[Nick Cameron [:nrc]]

Attachment: Patch 2: Rename GetTextureHost to GetAsTextureHost

Comment 10

[Nick Cameron [:nrc]]

Attachment: Patch 3: Add a path to RotatedContentBuffer for new textures

Comment 11

[Nick Cameron [:nrc]]

Attachment: Patch 4: Changes to texture clients

Comment 12

[Nick Cameron [:nrc]]

Attachment: Patch 5: Content clients

Comment 13

[Nick Cameron [:nrc]]

Attachment: Patch 6: Content hosts

Comment 14

[Nick Cameron [:nrc]]

Attachment: Patch 7: Tweeks to TextureHost

Comment 15

[Nicolas Silva [:nical]]

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 16

[Nicolas Silva [:nical]]

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 17

[Nicolas Silva [:nical]]

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 18

[Nicolas Silva [:nical]]

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

Comment 19

[Nicolas Silva [:nical]]

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);

Comment 20

[Nicolas Silva [:nical]]

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.

Comment 21

[Nicolas Silva [:nical]]

Attachment: clear the texture clients when creating it in ContentClient

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

Comment 22

[Nicolas Silva [:nical]]

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

Comment 23

[Nicolas Silva [:nical]]

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

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 25

[Nick Cameron [:nrc]]

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

[Nick Cameron [:nrc]]

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.

Comment 27

[Nick Cameron [:nrc]]

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

Comment 28

[Nick Cameron [:nrc]]

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

[Matt Woodrow (:mattwoodrow)]

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

Comment 30

[Nicolas Silva [:nical]]

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

Comment 31

[Matt Woodrow (:mattwoodrow)]

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

Comment 32

[Matt Woodrow (:mattwoodrow)]

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

Comment 33

[Nicolas Silva [:nical]]

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

Comment 34

[Nicolas Silva [:nical]]

(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

Comment 35

[Nick Cameron [:nrc]]

Attachment: patch 9: rename and sort indents

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

Comment 36

[Nick Cameron [:nrc]]

Attachment: patch 9: rename and sort indents

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

Comment 37

[Nick Cameron [:nrc]]

Attachment: patch 10: more renaming

Comment 38

[Nick Cameron [:nrc]]

needinfo for comment 28

Comment 39

[Nick Cameron [:nrc]]

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

[Nick Cameron [:nrc]]

Attachment: patch 6.1: Get rid of RemoveTextureHostDeferred

Comment 41

[Nick Cameron [:nrc]]

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.

Comment 42

[Nick Cameron [:nrc]]

Attachment: Patch 5: Content clients

Slightly updated

Comment 43

[Nick Cameron [:nrc]]

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

[Nicolas Silva [:nical]]

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

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

nice

Comment 45

[Nicolas Silva [:nical]]

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

Comment 46

[Nicolas Silva [:nical]]

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

[Nicolas Silva [:nical]]

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 48

[Nicolas Silva [:nical]]

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.

Comment 49

[Nicolas Silva [:nical]]

Attachment: Add a flag In BufferTextureClient to clear the buffer when allocating

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.

Comment 50

[Nick Cameron [:nrc]]

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

[Nick Cameron [:nrc]]

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 52

[Nick Cameron [:nrc]]

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

Comment 53

[Matt Woodrow (:mattwoodrow)]

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

Comment 54

[Nicolas Silva [:nical]]

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

Comment 55

[Nick Cameron [:nrc]]

Attachment: patch 10: remove access mode stuff

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

Comment 56

[Nick Cameron [:nrc]]

Comment on attachment 8333549 [details] [diff] [review]
patch 10: remove access mode stuff

Whoops! Little bit keen there, need to check those TODOs first.

Comment 57

[Nick Cameron [:nrc]]

Attachment: patch 11: remove access mode stuff

Mostly removing stuff.

Comment 58

[Nick Cameron [:nrc]]

Attachment: patch 12: rebasing fix ups

Comment 59

[Nick Cameron [:nrc]]

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

Comment 60

[Nick Cameron [:nrc]]

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

Comment 61

[Nick Cameron [:nrc]]

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

Comment 62

[Nick Cameron [:nrc]]

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.

Comment 63

[Nicolas Silva [:nical]]

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

Comment 64

[Nicolas Silva [:nical]]

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

Comment 65

[Nick Cameron [:nrc]]

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

Comment 66

[Nick Cameron [:nrc]]

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

Comment 67

[Nicolas Silva [:nical]]

(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

Comment 68

[Nick Cameron [:nrc]]

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

Comment 69

[Ryan VanderMeulen [:RyanVM]]

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

Comment 70

[Benoit Jacob [:bjacob] (mostly away)]

Backed out for accidentally disabling gralloc for thebes layers on B2G, as reported by nical:

https://hg.mozilla.org/mozilla-central/rev/526e12792fc8

Comment 71

[Sotaro Ikeda [:sotaro]]

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

Comment 72

[Nicolas Silva [:nical]]

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

Comment 73

[Sotaro Ikeda [:sotaro]]

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

Comment 74

[Sotaro Ikeda [:sotaro]]

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

Comment 75

[Nick Cameron [:nrc]]

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

Comment 76

[Sotaro Ikeda [:sotaro]]

(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

Comment 77

[Sotaro Ikeda [:sotaro]]

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

Comment 78

[Sotaro Ikeda [:sotaro]]

old days, gralloc buffers are lockked like following.

DeprecatedContentClientRemoteBuffer::CreateBuffer()
 ->DeprecatedTextureClientShmem::LockDrawTarget()
 ->DeprecatedTextureClientShmem::GetSurface()
 ->ShadowLayerForwarder::OpenDescriptor()
 ->GraphicBuffer::lock()

Comment 79

[Benoit Jacob [:bjacob] (mostly away)]

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

Comment 80

[Nick Cameron [:nrc]]

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

[Nick Cameron [:nrc]]

Attachment: disable new textures for b2g

Comment 82

[Benoit Jacob [:bjacob] (mostly away)]

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 83

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 84

[Nick Cameron [:nrc]]

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

Comment 85

[Nick Cameron [:nrc]]

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.

Comment 86

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 87

[Sotaro Ikeda [:sotaro]]

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

Comment 88

[Sotaro Ikeda [:sotaro]]

Set 1.3?, because this bug blocks Bug 925444.

Comment 89

[Nick Cameron [:nrc]]

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)

Comment 90

[Nick Cameron [:nrc]]

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

Comment 91

[Carsten Book [:Tomcat]]

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

Comment 92

[Preeti Raghunath(:Preeti)]

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

Comment 93

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 94

[Sotaro Ikeda [:sotaro]]

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.

Comment 95

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

>  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?