The default bug view has changed. See this FAQ.

Consistently check OGL framebuffer completeness in non-debug builds

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ajuma, Assigned: ajuma)

Tracking

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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)

Comment 1

6 years ago
Created attachment 569073 [details] [diff] [review]
Add missing framebuffer completeness 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+

Updated

6 years ago
Blocks: 695246

Updated

6 years ago
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-

Updated

6 years ago
Duplicate of this bug: 695246

Updated

6 years ago
Blocks: 607684
No longer blocks: 695246
(Assignee)

Comment 6

6 years ago
Created attachment 570772 [details] [diff] [review]
Abort when framebuffer completeness check fails.
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+
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/debd724ba3c8
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/debd724ba3c8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 705641
You need to log in before you can comment on or make changes to this bug.