Closed Bug 984823 Opened 6 years ago Closed 6 years ago

Tiles are using GL_REPEAT

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Use clamp to edges with tiles (obsolete) — Splinter Review
This fixes the border artifacts on b2g.
Attachment #8392844 - Flags: review?(chrislord.net)
blocking-b2g: --- → 1.4?
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.
Attachment #8392844 - Flags: review?(chrislord.net) → review-
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.
Attached patch Use clamp to edges with tiles (obsolete) — Splinter Review
Updated version of the patch that asserts that we always use the same texture target with the pool.
Attachment #8392844 - Attachment is obsolete: true
Attachment #8393452 - Flags: review?(chrislord.net)
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?
Attachment #8393452 - Flags: review?(chrislord.net) → review-
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?
Can we please have a video describing the situation?
Flags: needinfo?(nical.bugzilla)
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?)
(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
Flags: needinfo?(nical.bugzilla)
(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)
Added the assertion in the other pool and removed the min/mag filters from the patch.
Attachment #8393452 - Attachment is obsolete: true
Attachment #8394728 - Flags: review?(chrislord.net)
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.
Attachment #8394728 - Flags: review?(chrislord.net) → review+
Duplicate of this bug: 986546
1.4+ for ugly video
blocking-b2g: 1.4? → 1.4+
https://hg.mozilla.org/mozilla-central/rev/b19ddc0219f5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Duplicate of this bug: 983255
This needs a branch-specific patch for Aurora uplift.
Flags: needinfo?(nical.bugzilla)
This depends on some patches from bug 980647
Depends on: 980647
Depends on: 987641
(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)
Flags: needinfo?(nical.bugzilla)
You need to log in before you can comment on or make changes to this bug.