696768 - Consistently check OGL framebuffer completeness in non-debug builds

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ali Juma [:ajuma]]

Currently, in some places we check framebuffer completeness in both debug and non-debug builds, and in other places we only check in debug builds. For consistency and correctness, we should add the missing checks to non-debug builds.

Comment 1

[Ali Juma [:ajuma]]

Attachment: Add missing framebuffer completeness checks to non-debug builds

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 569073 [details] [diff] [review]
Add missing framebuffer completeness checks to non-debug builds

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

Path looks great.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +777,5 @@
>    }
>  
> +  // We can't draw if we aren't able to setup the back buffer,
> +  // so just return.
> +  if (!SetupBackBuffer(width, height)) {

small nit: The comment doesn't add any non trivial info so it should either be changed to something more usefull or removed.

bjacob mentioned that the rules for framebuffers can differ arbitrarily. Perhaps mention that instead?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 569073 [details] [diff] [review]
Add missing framebuffer completeness checks to non-debug builds

 
>-  NS_ASSERTION(mGLContext->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER) ==
>-               LOCAL_GL_FRAMEBUFFER_COMPLETE, "Error setting up framebuffer.");
>+  if (mGLContext->fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER) !=
>+      LOCAL_GL_FRAMEBUFFER_COMPLETE) {
>+    NS_WARNING("Error setting up framebufifer --- framebuffer not complete");
>+    mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
>+    mGLContext->fDeleteFramebuffers(1, &fbo);
>+    mGLContext->fDeleteTextures(1, &tex);
>+    return false;
>+  }


I worry that it's not worth trying to handle this failure cleanly. I think we might be better of just aborting if we don't get a complete framebuffer. I think you should also add a comment about this being needed to prevent crashes on tegra.

>

Comment 6

[Ali Juma [:ajuma]]

Attachment: Abort when framebuffer completeness check fails.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 570772 [details] [diff] [review]
Abort when framebuffer completeness check fails.


> 
>-    if (aTexture) {
>-        DebugOnly<GLenum> status = fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER);
>+    if (aTexture && (fCheckFramebufferStatus(LOCAL_GL_FRAMEBUFFER) !=
>+                     LOCAL_GL_FRAMEBUFFER_COMPLETE)) {
> 
>-        // Note: if you are hitting this assertion, it is likely that
>+        // Note: if you are hitting this,, it is likely that

double ,, here

Comment 8

[Ali Juma [:ajuma]]

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

Comment 9

[Marco Bonardo [:mak]]

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