Last Comment Bug 528013 - [webgl] bindFramebuffer checks target against wrong values
: [webgl] bindFramebuffer checks target against wrong values
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-11 13:02 PST by Ilmari Heikkinen
Modified: 2009-12-07 15:11 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix bindFramebuffer check (3.01 KB, patch)
2009-11-30 13:36 PST, Mark Steele
vladimir: review+
Details | Diff | Review

Description Ilmari Heikkinen 2009-11-11 13:02:13 PST
BindFramebuffer checks the target param against valid framebuffer attachments. This is completely wrong as the only valid target for BindFramebuffer is GL_FRAMEBUFFER.

http://www.khronos.org/opengles/sdk/docs/man/glBindFramebuffer.xml

The functions that take a framebuffer attachment value are FramebufferRenderbuffer and FramebufferTexture2D.

Further, the way framebuffers work is that you have only one framebuffer bound at any given time. All drawing goes to that framebuffer. The framebuffer then has attachments, which store the draw results. 

You can either attach textures using FramebufferTexture2D or renderbuffer objects with FramebufferRenderbuffer. Each attachment goes to one of the attachment targets: GL_COLOR_ATTACHMENT0, GL_DEPTH_ATTACHMENT, or GL_STENCIL_ATTACHMENT. Each attachment target can have only one object bound to it at any given time.

My understanding in JS terms:

Framebuffer.prototype = {
  colorAttachment : null,
  depthAttachment : null,
  stencilAttachment : null
}
Renderbuffer.prototype = {
  internalformat : null,
  width : null,
  height : null
}
bindFramebuffer = function(fbo){currentFbo = fbo}
bindRenderbuffer = function(rbo){currentRbo = rbo}
renderbufferStorage = function(internalformat, w, h){
  currentRbo.internalformat = internalformat;
  currentRbo.width = w;
  currentRbo.height = h;
}
framebufferTexture2D(attachment, tex){
  currentFbo[attachment] = tex;
}
framebufferRenderbuffer(attachment, rbo) {
  currentFbo[attachment] = rbo;
}


BindFramebuffer in WebGLContextGL.cpp:456
-    if (target >= LOCAL_GL_COLOR_ATTACHMENT0 &&
-        target < (LOCAL_GL_COLOR_ATTACHMENT0 + mBoundColorFramebuffers.Length()))
-    {
-        int targetOffset = target - LOCAL_GL_COLOR_ATTACHMENT0;
-        mBoundColorFramebuffers[targetOffset] = wfb;
-    } else if (target == LOCAL_GL_DEPTH_ATTACHMENT) {
-        mBoundDepthFramebuffer = wfb;
-    } else if (target == LOCAL_GL_STENCIL_ATTACHMENT) {
-        mBoundStencilFramebuffer = wfb;
-    } else {
+    if (target != LOCAL_GL_FRAMEBUFFER) {
         return ErrorMessage("glBindFramebuffer: invalid target");
     }
+    mBoundFramebuffer = wfb;
 
     gl->fBindFramebuffer(target, wfb ? wfb->GLName() : 0);
Comment 1 Martin Weusten 2009-11-24 02:24:20 PST
I can confirm this for Linux (x86_64) with build 2009-11-23.
Comment 2 Mark Steele 2009-11-30 13:36:46 PST
Created attachment 415232 [details] [diff] [review]
fix bindFramebuffer check
Comment 3 Vladimir Vukicevic [:vlad] [:vladv] 2009-12-07 15:11:28 PST
Good catch, checked in with some changes.

http://hg.mozilla.org/mozilla-central/rev/cf72d45dc256

We still need to track the framebuffer details at some point..

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