562392 - Layers should use TEXTURE_RECTANGLE_ARB when possible

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

Currently on OS X machines with RadeonX1600 we can't use a framebuffer with a TEXTURE_2D texture since we are not using texture with a power of 2 size. We get 'INVALID_FRAMEBUFFER_OPERATION' when performing operations on the framebuffer.

As I understand it, we need to use TEXTURE_RECTANGLE_ARB where it is supported. This fixes the framebuffer error. Attached is the patch that fixes the issue for me. We should add a check and make sure TEXTURE_RECTANGLE_ARB is supported and have a fallback when it is not.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Use TEXTURE_RECTANGLE_ARB

Comment 2

[Joe Drew (not getting mail)]

Benoit and I talked about this; what happens is that on his machine, the Framebuffer extension apparently doesn't support non-power-of-two textures, so when you try to attach to the framebuffer, it is actually incomplete.

Ideally we'd do something like this:
- Somehow check if the framebuffer extension supports NPOT textures. At the very worst, this means doing all our work, and then seeing if the framebuffer says it's incomplete when we try to attach to it.
- If not, we'd try to use ARB_texture_rectangle.
- If that doesn't exist, look for EXT_texture_rectangle or NV_texture_rectangle, and use them.
- At this point, give up and just use software.

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to comment #2)
> Benoit and I talked about this; what happens is that on his machine, the
> Framebuffer extension apparently doesn't support non-power-of-two textures, so
> when you try to attach to the framebuffer, it is actually incomplete.
> 
> Ideally we'd do something like this:
> - Somehow check if the framebuffer extension supports NPOT textures. At the
> very worst, this means doing all our work, and then seeing if the framebuffer
> says it's incomplete when we try to attach to it.
> - If not, we'd try to use ARB_texture_rectangle.
> - If that doesn't exist, look for EXT_texture_rectangle or
> NV_texture_rectangle, and use them.
> - At this point, give up and just use software.

We should probably just generate a texture and try and bind it in the LayerManager initialization. For FBOs (only used in ContainerLayer and SetupBackbuffer, we should then use one of the extension texture targets.

I'm assuming in general his machine supports NPOT textures for GL_TEXTURE_2D, and it only doesn't for FBOs? Additionally, does it support binding FBOs to the extension targets?

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Use TEXTURE_RECTANGLE where required for backbuffer FBO

This should use the TEXTURE_RECTANGLE target for the main backbuffer FBO. We need to follow up on making the container layer use this too. But that needs some more testing, and since fullscreen video doesn't need it we should proceed with fixing the backbuffer problem here first to get fullscreen video working.

Comment 5

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

Wait bad -- layers should use TEXTURE_2D if we have NPOT textures, and only fall back to TEXTURE_RECTANGLE if we don't have NPOT textures.  (And if neither, then.. dunno.)  GLES doesn't have TEXTURE_RECTANGLE, but has basic NPOT textures.

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to comment #5)
> Wait bad -- layers should use TEXTURE_2D if we have NPOT textures, and only
> fall back to TEXTURE_RECTANGLE if we don't have NPOT textures.  (And if
> neither, then.. dunno.)  GLES doesn't have TEXTURE_RECTANGLE, but has basic
> NPOT textures.

Look at the patch, my patch is -only- for FBO's, and it's only trying to use TEXTURE_RECTANGLE in case TEXTURE_2D fails for them. So nothing to worry about really, if FBOs work with GL_TEXTURE_2D, we'll just use that as we always did!

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Use TEXTURE_RECTANGLE where required for backbuffer FBO v2

This patch fixes a couple of small mistakes in the code. This works fine for me (if I force the first check to fail), however on BenWa's machine the gave some strange layer artifacts.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 443103 [details] [diff] [review]
Use TEXTURE_RECTANGLE where required for backbuffer FBO v2

>diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp

>@@ -119,16 +119,64 @@ LayerManagerOGL::Initialize()

>+  mGLContext->fGenFramebuffers(1, &mFrameBuffer);

Could you add a comment saying that we're testing the ability of NPOT textures to bind to framebuffers, and we'll fall back to EXT_texture_rectangle if we need to?

(Alternately: move all this testing stuff into its own function.)

>+  GLenum textureTargets[] = { LOCAL_GL_TEXTURE_2D,
>+                              LOCAL_GL_TEXTURE_RECTANGLE_EXT };
>+  mFBOTextureTarget = 0;
>+
>+  for (int i = 0; i < sizeof(textureTargets) / sizeof(GLenum); i++) {

NS_ARRAY_SIZE(textureTargets)

>+    mGLContext->fTexImage2D(textureTargets[i],
>+                            0,
>+                            LOCAL_GL_RGBA,
>+                            200,
>+                            100,
>+                            0,
>+                            LOCAL_GL_RGBA,
>+                            LOCAL_GL_UNSIGNED_BYTE,
>+                            NULL);
>+
>+

Extra line of whitespace here.

>+    if (mGLContext->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER) ==
>+        LOCAL_GL_FRAMEBUFFER_COMPLETE) {
>+          mFBOTextureTarget = textureTargets[i];
>+          break;
>+    }

Hm, I'm not much in favour of breaking out of the loop like this. Maybe have a do/while loop?

>+    mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
>+    mGLContext->fBindTexture(textureTargets[i], 0);
>+    mGLContext->fDeleteTextures(1, &mBackBuffer);

I don't think you need to delete the texture here, right? Couldn't you just GenTexture once, along with the framebuffer, and then glTexImage2D with the same texture ID?

>@@ -300,21 +348,24 @@ LayerManagerOGL::Render()

>+    if (mFBOTextureTarget == LOCAL_GL_TEXTURE_RECTANGLE_EXT) {
>+      mGLContext->fEnable(LOCAL_GL_TEXTURE_RECTANGLE_EXT);
>+    }

Is constantly enabling and disabling TEXTURE_RECTANGLE sucky for perf? I guess we're only doing it once per frame.

>   /**
>    * Create the framebuffer and bind it to make our content render into our
>    * framebuffer.
>    */

You can remove the mention about "Create" here.

Overall, very much like moving the generation of texture and framebuffer ID to the Initialize() function.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we please please please get this landed? Please? :-)

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to comment #8)
> (From update of attachment 443103 [details] [diff] [review])
> I don't think you need to delete the texture here, right? Couldn't you just
> GenTexture once, along with the framebuffer, and then glTexImage2D with the
> same texture ID?

From the documentation of glBindTexture: 'GL_INVALID_OPERATION is generated if texture was previously created with a target that doesn't match that of target.' So we need to clear out the texture to be able to bind it to a different target.

>>+    if (mGLContext->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER) ==
>>+        LOCAL_GL_FRAMEBUFFER_COMPLETE) {
>>+          mFBOTextureTarget = textureTargets[i];
>>+          break;
>>+    }
>
>Hm, I'm not much in favour of breaking out of the loop like this. Maybe have a
>do/while loop?

That's tricky, since we'd just end up using a continue with the opposite conditional clause, since we need to clean some stuff up before going to the next loop iteration, in all I think it'd be worse.

> 
> >@@ -300,21 +348,24 @@ LayerManagerOGL::Render()
> 
> >+    if (mFBOTextureTarget == LOCAL_GL_TEXTURE_RECTANGLE_EXT) {
> >+      mGLContext->fEnable(LOCAL_GL_TEXTURE_RECTANGLE_EXT);
> >+    }
> 
> Is constantly enabling and disabling TEXTURE_RECTANGLE sucky for perf? I guess
> we're only doing it once per frame.
I doubt it, and yeah, only once per frame too.

Comment 11

[Joe Drew (not getting mail)]

Ok! Go ahead then.

Comment 12

[Benoit Girard (:BenWa)]

I don't think this bug is still relevant.