984823 - Tiles are using GL_REPEAT

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Which looks bad while zooming on b2g. It appears we are not specifying the wrap mode with gralloc textures, and GL_REPEAT is the default in opengl.
We need to use clamp-to-edge instead.
bjacob was the one who found that out.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Use clamp to edges with tiles

This fixes the border artifacts on b2g.

Comment 2

[Nicolas Silva [:nical]]

try: https://tbpl.mozilla.org/?tree=Try&rev=9c642958d897

Comment 3

[Chris Lord [:cwiiis]]

Comment on attachment 8392844 [details] [diff] [review]
Use clamp to edges with tiles

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

I think you should initialise these texture parameters in ::Lock - not least because the target could change and that may break, or produce unexpected results (as only tiles uses this, it's not currently a problem, but it will be when we re-fix bug 980647). Either that, or have per-target pools in CompositorOGL.

Comment 4

[Nicolas Silva [:nical]]

The change of target issue is the same if you initialize it in the TextureHost/Source, actually it's worse because if the texture pool doesn't know about the target it cannot attempt to separate TEXTURE_2Ds from TEXTURE_EXTERNALs, so when you ask for the next texture you don't know if it is a new one or if it's one that we recycled and already has a texture target (which value we don't know).

This patch makes it easy to do a per-target pool when we need it (I tried to get the smallest/least risky patch here instead of splitting the pool, etc, because I think this should be uplifted)

I'll write a followup for the per-target pool and we can land them together.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Use clamp to edges with tiles

Updated version of the patch that asserts that we always use the same texture target with the pool.

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 8393452 [details] [diff] [review]
Use clamp to edges with tiles

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

Sorry to be awkward, but we need this assertion in both pools - I could see this being something that'd catch you out in nasty, hard-to-debug ways.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1435,2 @@
>    }
>    return mTextures[index];

We'll need the same assertion in this implementation of GetTexture as has been added to PerFrameTexturePoolOGL. Perhaps rather than just storing the texture ID, the array could store a struct of the target and the ID?

Comment 7

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

Comment on attachment 8393452 [details] [diff] [review]
Use clamp to edges with tiles

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

::: gfx/layers/opengl/CompositorOGL.h
@@ +122,5 @@
>  class PerFrameTexturePoolOGL : public CompositorTexturePoolOGL
>  {
>  public:
>    PerFrameTexturePoolOGL(gl::GLContext* aGL)
> +  : mTextureTarget(0) // zero is never a valid texture target

We don't have an enum/define for the "invalid texture target", do we?

@@ +143,5 @@
>  
>  protected:
>    void DestroyTextures();
>  
> +  GLenum mTextureTarget;

This is currently just used for the assert(s), right?  But it's there so that we can do the per target pools in the future?

Comment 8

[Preeti Raghunath(:Preeti)]

Can we please have a video describing the situation?

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8393452 [details] [diff] [review]
Use clamp to edges with tiles

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

Shouldn't we also be obeying the TEXTURE_ALLOW_REPEAT flag, rather than making these CLAMP unconditionally?

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1428,5 @@
>      }
>      mGL->fGenTextures(1, &mTextures[index]);
> +    mGL->fBindTexture(aTarget, mTextures[index]);
> +    mGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_NEAREST);
> +    mGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);

Don't both setting the filters here, since we set them again when we draw (also, why is the minification filter linear?)

Comment 10

[Nicolas Silva [:nical]]

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> Can we please have a video describing the situation?

Here is a video showing artifacts at tile boundaries du to the GL_REPEAT texture parameter
https://dl.dropboxusercontent.com/u/7742672/tilesclamp.mp4

Comment 11

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #7)
> We don't have an enum/define for the "invalid texture target", do we?

We don't. The OpenGL spec just specifies the texture targets constants, none of which are zero.

> > +  GLenum mTextureTarget;
> 
> This is currently just used for the assert(s), right?  But it's there so
> that we can do the per target pools in the future?

Yes, right now it is just to catch potential bugs. If at some point we need to have a mix of TEXTURE_2D and TEXTURE_EXTERNAL in the pool, we'll add support for this in the pool without changing the API (it's not clear to me when we'll need this so right now we keep it as a simple assertion)

Comment 12

[Nicolas Silva [:nical]]

Attachment: Use clamp to edges with tiles

Added the assertion in the other pool and removed the min/mag filters from the patch.

Comment 13

[Chris Lord [:cwiiis]]

Comment on attachment 8394728 [details] [diff] [review]
Use clamp to edges with tiles

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

LGTM - Any problems will be caught by the assertions, so this is good to go imo.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1459,5 @@
>  {
> +  if (mTextureTarget == 0) {
> +    mTextureTarget = aTarget;
> +  }
> +  MOZ_ASSERT(mTextureTarget == aTarget);

I'd prefer that we assert that the texture target remains the same for each unit, rather than having one for all units, but this is fine for now.

@@ +1478,5 @@
>      }
>      mGL->fGenTextures(1, &mTextures[index]);
> +    mGL->fBindTexture(aTarget, mTextures[index]);
> +    mGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE);
> +    mGL->fTexParameteri(aTarget, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE);

It strikes me as odd that the magnification/minification filters are set elsewhere, but the wrap parameters aren't - either way, I'm fine with setting up default settings here.

Comment 14

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b19ddc0219f5

Comment 16

[Preeti Raghunath(:Preeti)]

1.4+ for ugly video

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b19ddc0219f5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This needs a branch-specific patch for Aurora uplift.

Comment 20

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

This depends on some patches from bug 980647

Comment 21

[Nicolas Silva [:nical]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> This needs a branch-specific patch for Aurora uplift.

I filed bug 987641 and uploaded the patches that apply to aurora there (sorry for the confusion)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

No problem, thanks :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Bug 987641 was backed out for bustage.