Last Comment Bug 696768 - Consistently check OGL framebuffer completeness in non-debug builds
: Consistently check OGL framebuffer completeness in non-debug builds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Ali Juma [:ajuma]
:
Mentors:
: 623493 695246 (view as bug list)
Depends on: 705641
Blocks: opengl-mobile
  Show dependency treegraph
 
Reported: 2011-10-24 08:16 PDT by Ali Juma [:ajuma]
Modified: 2011-11-28 11:47 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add missing framebuffer completeness checks to non-debug builds (11.67 KB, patch)
2011-10-24 08:28 PDT, Ali Juma [:ajuma]
jmuizelaar: review-
bgirard: feedback+
Details | Diff | Splinter Review
Abort when framebuffer completeness check fails. (3.61 KB, patch)
2011-10-31 11:35 PDT, Ali Juma [:ajuma]
jmuizelaar: review+
Details | Diff | Splinter Review

Description Ali Juma [:ajuma] 2011-10-24 08:16:57 PDT
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] 2011-10-24 08:28:27 PDT
Created attachment 569073 [details] [diff] [review]
Add missing framebuffer completeness checks to non-debug builds
Comment 2 Benoit Girard (:BenWa) 2011-10-24 10:02:40 PDT
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 3 Benoit Girard (:BenWa) 2011-10-28 13:09:54 PDT
*** Bug 623493 has been marked as a duplicate of this bug. ***
Comment 4 Jeff Muizelaar [:jrmuizel] 2011-10-28 13:38:14 PDT
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 5 Benoit Girard (:BenWa) 2011-10-28 14:57:29 PDT
*** Bug 695246 has been marked as a duplicate of this bug. ***
Comment 6 Ali Juma [:ajuma] 2011-10-31 11:35:26 PDT
Created attachment 570772 [details] [diff] [review]
Abort when framebuffer completeness check fails.
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-11-01 13:50:38 PDT
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 9 Marco Bonardo [::mak] 2011-11-03 08:26:19 PDT
https://hg.mozilla.org/mozilla-central/rev/debd724ba3c8

Note You need to log in before you can comment on or make changes to this bug.