Closed Bug 696768 Opened 8 years ago Closed 8 years ago

Consistently check OGL framebuffer completeness in non-debug builds

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: ajuma, Assigned: ajuma)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → ajuma
Status: NEW → ASSIGNED
Attachment #569073 - Flags: review?(jmuizelaar)
Attachment #569073 - Flags: feedback?(bgirard)
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?
Attachment #569073 - Flags: feedback?(bgirard) → feedback+
Blocks: 695246
Duplicate of this bug: 623493
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.

>
Attachment #569073 - Flags: review?(jmuizelaar) → review-
Duplicate of this bug: 695246
Blocks: opengl-mobile
No longer blocks: 695246
Attachment #569073 - Attachment is obsolete: true
Attachment #570772 - Flags: review?(jmuizelaar)
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
Attachment #570772 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/debd724ba3c8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 705641
You need to log in before you can comment on or make changes to this bug.