Bug 33416 - Hook up WebGLContextAttributes to OpenGL context creation code
Summary: Hook up WebGLContextAttributes to OpenGL context creation code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on: 31169
Blocks: 36908 38560
  Show dependency treegraph
 
Reported: 2010-01-08 19:18 PST by Kenneth Russell
Modified: 2010-06-18 12:01 PDT (History)
9 users (show)

See Also:


Attachments
patch (66.43 KB, patch)
2010-03-10 14:24 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
modified patch (64.69 KB, patch)
2010-03-10 14:42 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
fixed style problems (64.57 KB, patch)
2010-03-10 14:57 PST, Zhenyao Mo
levin: review-
Details | Formatted Diff | Diff
removed the whitespace modifications (49.90 KB, patch)
2010-03-10 17:46 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
modified acording to Ken Russell review (51.50 KB, patch)
2010-03-11 18:40 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
fix a warning that my compiler fails to catch (51.53 KB, patch)
2010-03-12 09:34 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
patch after Ken Russell's refac (57.65 KB, patch)
2010-03-23 19:20 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch : responding to Ken Russell's review (57.18 KB, patch)
2010-03-30 18:12 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch : fixed the changeLog (deleted)
2010-03-30 18:29 PDT, Zhenyao Mo
fishd: review-
Details | Formatted Diff | Diff
revised patch: responding to Darin Fisher's review. (55.87 KB, patch)
2010-03-31 11:03 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-01-08 19:18:22 PST
https://bugs.webkit.org/show_bug.cgi?id=31169 adds the plumbing to pass a JavaScript object down to the creation of the WebGLRenderingContext and associated GraphicsContext3D. We need to pay attention to these flags when creating the OpenGL context and associated framebuffer objects. This is a moderately sized chunk of work so it is being split off as a separate task.
Comment 1 Zhenyao Mo 2010-03-10 14:24:10 PST
Created attachment 50438 [details]
patch
Comment 2 Zhenyao Mo 2010-03-10 14:42:30 PST
Created attachment 50441 [details]
modified patch
Comment 3 WebKit Review Bot 2010-03-10 14:48:27 PST
Attachment 50441 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:230:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp:230:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/GraphicsContext3D.cpp:725:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/chromium/src/GraphicsContext3D.cpp:732:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Zhenyao Mo 2010-03-10 14:57:36 PST
Created attachment 50442 [details]
fixed style problems
Comment 5 Chris Marrin 2010-03-10 15:19:44 PST
You should really try to avoid making insignificant changes in your patches. Almost half of this patch is deleting extra spaces and other changes which make it really hard to understand what is really being changed. I suggest you resubmit the patch without the insignificant changes.
Comment 6 David Levin 2010-03-10 15:22:07 PST
Comment on attachment 50442 [details]
fixed style problems

r- based on Chris's comment, which I agree with.
Comment 7 Zhenyao Mo 2010-03-10 17:46:43 PST
Created attachment 50458 [details]
removed the whitespace modifications
Comment 8 Chris Marrin 2010-03-11 06:08:24 PST
Comment on attachment 50458 [details]
removed the whitespace modifications


> +function getWebGL(canvasName, contextAttribs, clearColor, clearDepth, clearStencil)
> +{
> +    var canvas = document.getElementById(canvasName);
> +    var gl = canvas.getContext("experimental-webgl", contextAttribs);
> +    if (!gl) {
> +        alert("No WebGL context found");
> +        return null;
> +    }
> +    var actualContextAttribs = gl.getContextAttributes();
> +    
> +    // Add a console
> +    gl.console = ("console" in window) ? window.console : { log: function() { } };
> +
> +    // create our shaders
> +    var vertexShader = loadShader(gl, "vshader");
> +    var fragmentShader = loadShader(gl, "fshader");
> +
> +    if (!vertexShader || !fragmentShader)
> +        return null;
> +
> +    // Create the program object
> +    gl.program = gl.createProgram();
> +
> +    if (!gl.program)
> +        return null;
> +
> +    // Attach our two shaders to the program
> +    gl.attachShader(gl.program, vertexShader);
> +    gl.attachShader(gl.program, fragmentShader);
> +
> +    // Bind attributes
> +    var attribs = [ "pos", "colorIn" ];
> +    for (var i in attribs)
> +        gl.bindAttribLocation(gl.program, i, attribs[i]);
> +
> +    // Link the program
> +    gl.linkProgram(gl.program);
> +
> +    // Check the link status
> +    var linked = gl.getProgramParameter(gl.program, gl.LINK_STATUS);
> +    if (!linked) {
> +        // something went wrong with the link
> +        var error = gl.getProgramInfoLog (gl.program);
> +        gl.console.log("Error in program linking:"+error);
> +
> +        gl.deleteProgram(gl.program);
> +        gl.deleteProgram(fragmentShader);
> +        gl.deleteProgram(vertexShader);
> +
> +        return null;
> +    }
> +
> +    gl.useProgram(gl.program);
> +
> +    gl.clearColor(clearColor[0], clearColor[1], clearColor[2], clearColor[3]);
> +    gl.clearDepth(clearDepth);
> +    gl.clearStencil(clearStencil);
> +    gl.enable(gl.DEPTH_TEST);
> +    gl.enable(gl.STENCIL_TEST);
> +    gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT | gl.STENCIL_BUFFER_BIT);
> +
> +    return gl;
> +}
> +
> +function drawAndReadPixel(gl, vertices, colors, x, y)
> +{
> +    var colorOffset = vertices.byteLength;
> +
> +    var vbo = gl.createBuffer();
> +    gl.bindBuffer(gl.ARRAY_BUFFER, vbo);
> +    gl.bufferData(gl.ARRAY_BUFFER, colorOffset + colors.byteLength, gl.STATIC_DRAW);
> +    gl.bufferSubData(gl.ARRAY_BUFFER, 0, vertices);
> +    gl.bufferSubData(gl.ARRAY_BUFFER, colorOffset, colors);
> +
> +    gl.vertexAttribPointer(0, 3, gl.FLOAT, false, 0, 0);
> +    gl.enableVertexAttribArray(0);
> +    gl.vertexAttribPointer(1, 4, gl.UNSIGNED_BYTE, true, 0, colorOffset);
> +    gl.enableVertexAttribArray(1);
> +
> +    gl.drawArrays(gl.TRIANGLES, 0, vertices.length / 3);
> +
> +    return gl.readPixels(x, y, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE);
> +}

Don't we have some common code to do this? If not we should. We will eventually have many tests using this technique.
Comment 9 Chris Marrin 2010-03-11 06:08:51 PST
Comment on attachment 50458 [details]
removed the whitespace modifications


> +function getWebGL(canvasName, contextAttribs, clearColor, clearDepth, clearStencil)
> +{
> +    var canvas = document.getElementById(canvasName);
> +    var gl = canvas.getContext("experimental-webgl", contextAttribs);
> +    if (!gl) {
> +        alert("No WebGL context found");
> +        return null;
> +    }
> +    var actualContextAttribs = gl.getContextAttributes();
> +    
> +    // Add a console
> +    gl.console = ("console" in window) ? window.console : { log: function() { } };
> +
> +    // create our shaders
> +    var vertexShader = loadShader(gl, "vshader");
> +    var fragmentShader = loadShader(gl, "fshader");
> +
> +    if (!vertexShader || !fragmentShader)
> +        return null;
> +
> +    // Create the program object
> +    gl.program = gl.createProgram();
> +
> +    if (!gl.program)
> +        return null;
> +
> +    // Attach our two shaders to the program
> +    gl.attachShader(gl.program, vertexShader);
> +    gl.attachShader(gl.program, fragmentShader);
> +
> +    // Bind attributes
> +    var attribs = [ "pos", "colorIn" ];
> +    for (var i in attribs)
> +        gl.bindAttribLocation(gl.program, i, attribs[i]);
> +
> +    // Link the program
> +    gl.linkProgram(gl.program);
> +
> +    // Check the link status
> +    var linked = gl.getProgramParameter(gl.program, gl.LINK_STATUS);
> +    if (!linked) {
> +        // something went wrong with the link
> +        var error = gl.getProgramInfoLog (gl.program);
> +        gl.console.log("Error in program linking:"+error);
> +
> +        gl.deleteProgram(gl.program);
> +        gl.deleteProgram(fragmentShader);
> +        gl.deleteProgram(vertexShader);
> +
> +        return null;
> +    }
> +
> +    gl.useProgram(gl.program);
> +
> +    gl.clearColor(clearColor[0], clearColor[1], clearColor[2], clearColor[3]);
> +    gl.clearDepth(clearDepth);
> +    gl.clearStencil(clearStencil);
> +    gl.enable(gl.DEPTH_TEST);
> +    gl.enable(gl.STENCIL_TEST);
> +    gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT | gl.STENCIL_BUFFER_BIT);
> +
> +    return gl;
> +}
> +
> +function drawAndReadPixel(gl, vertices, colors, x, y)
> +{
> +    var colorOffset = vertices.byteLength;
> +
> +    var vbo = gl.createBuffer();
> +    gl.bindBuffer(gl.ARRAY_BUFFER, vbo);
> +    gl.bufferData(gl.ARRAY_BUFFER, colorOffset + colors.byteLength, gl.STATIC_DRAW);
> +    gl.bufferSubData(gl.ARRAY_BUFFER, 0, vertices);
> +    gl.bufferSubData(gl.ARRAY_BUFFER, colorOffset, colors);
> +
> +    gl.vertexAttribPointer(0, 3, gl.FLOAT, false, 0, 0);
> +    gl.enableVertexAttribArray(0);
> +    gl.vertexAttribPointer(1, 4, gl.UNSIGNED_BYTE, true, 0, colorOffset);
> +    gl.enableVertexAttribArray(1);
> +
> +    gl.drawArrays(gl.TRIANGLES, 0, vertices.length / 3);
> +
> +    return gl.readPixels(x, y, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE);
> +}

Don't we have some common code to do this? If not we should. We will eventually have many tests using this technique.
Comment 10 Chris Marrin 2010-03-11 06:08:58 PST
Comment on attachment 50458 [details]
removed the whitespace modifications


> +function getWebGL(canvasName, contextAttribs, clearColor, clearDepth, clearStencil)
> +{
> +    var canvas = document.getElementById(canvasName);
> +    var gl = canvas.getContext("experimental-webgl", contextAttribs);
> +    if (!gl) {
> +        alert("No WebGL context found");
> +        return null;
> +    }
> +    var actualContextAttribs = gl.getContextAttributes();
> +    
> +    // Add a console
> +    gl.console = ("console" in window) ? window.console : { log: function() { } };
> +
> +    // create our shaders
> +    var vertexShader = loadShader(gl, "vshader");
> +    var fragmentShader = loadShader(gl, "fshader");
> +
> +    if (!vertexShader || !fragmentShader)
> +        return null;
> +
> +    // Create the program object
> +    gl.program = gl.createProgram();
> +
> +    if (!gl.program)
> +        return null;
> +
> +    // Attach our two shaders to the program
> +    gl.attachShader(gl.program, vertexShader);
> +    gl.attachShader(gl.program, fragmentShader);
> +
> +    // Bind attributes
> +    var attribs = [ "pos", "colorIn" ];
> +    for (var i in attribs)
> +        gl.bindAttribLocation(gl.program, i, attribs[i]);
> +
> +    // Link the program
> +    gl.linkProgram(gl.program);
> +
> +    // Check the link status
> +    var linked = gl.getProgramParameter(gl.program, gl.LINK_STATUS);
> +    if (!linked) {
> +        // something went wrong with the link
> +        var error = gl.getProgramInfoLog (gl.program);
> +        gl.console.log("Error in program linking:"+error);
> +
> +        gl.deleteProgram(gl.program);
> +        gl.deleteProgram(fragmentShader);
> +        gl.deleteProgram(vertexShader);
> +
> +        return null;
> +    }
> +
> +    gl.useProgram(gl.program);
> +
> +    gl.clearColor(clearColor[0], clearColor[1], clearColor[2], clearColor[3]);
> +    gl.clearDepth(clearDepth);
> +    gl.clearStencil(clearStencil);
> +    gl.enable(gl.DEPTH_TEST);
> +    gl.enable(gl.STENCIL_TEST);
> +    gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT | gl.STENCIL_BUFFER_BIT);
> +
> +    return gl;
> +}
> +
> +function drawAndReadPixel(gl, vertices, colors, x, y)
> +{
> +    var colorOffset = vertices.byteLength;
> +
> +    var vbo = gl.createBuffer();
> +    gl.bindBuffer(gl.ARRAY_BUFFER, vbo);
> +    gl.bufferData(gl.ARRAY_BUFFER, colorOffset + colors.byteLength, gl.STATIC_DRAW);
> +    gl.bufferSubData(gl.ARRAY_BUFFER, 0, vertices);
> +    gl.bufferSubData(gl.ARRAY_BUFFER, colorOffset, colors);
> +
> +    gl.vertexAttribPointer(0, 3, gl.FLOAT, false, 0, 0);
> +    gl.enableVertexAttribArray(0);
> +    gl.vertexAttribPointer(1, 4, gl.UNSIGNED_BYTE, true, 0, colorOffset);
> +    gl.enableVertexAttribArray(1);
> +
> +    gl.drawArrays(gl.TRIANGLES, 0, vertices.length / 3);
> +
> +    return gl.readPixels(x, y, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE);
> +}

Don't we have some common code to do this? If not we should. We will eventually have many tests using this technique.
Comment 11 Zhenyao Mo 2010-03-11 09:33:20 PST
Are you talking about the getWebGL or drawAndReadPixel?

If getWebGL, we can modify the existing code to include a parameter for setting context attributes.  Then almost all the tests need to be modified (although tiny modification).

I think we should open a new bug for this.

(In reply to comment #8)
> (From update of attachment 50458 [details])
> > +function getWebGL(canvasName, contextAttribs, clearColor, clearDepth, clearStencil)
> > +{
> > +    var canvas = document.getElementById(canvasName);
> > +    var gl = canvas.getContext("experimental-webgl", contextAttribs);
> > +    if (!gl) {
> > +        alert("No WebGL context found");
> > +        return null;
> > +    }
> > +    var actualContextAttribs = gl.getContextAttributes();
> > +    
> > +    // Add a console
> > +    gl.console = ("console" in window) ? window.console : { log: function() { } };
> > +
> > +    // create our shaders
> > +    var vertexShader = loadShader(gl, "vshader");
> > +    var fragmentShader = loadShader(gl, "fshader");
> > +
> > +    if (!vertexShader || !fragmentShader)
> > +        return null;
> > +
> > +    // Create the program object
> > +    gl.program = gl.createProgram();
> > +
> > +    if (!gl.program)
> > +        return null;
> > +
> > +    // Attach our two shaders to the program
> > +    gl.attachShader(gl.program, vertexShader);
> > +    gl.attachShader(gl.program, fragmentShader);
> > +
> > +    // Bind attributes
> > +    var attribs = [ "pos", "colorIn" ];
> > +    for (var i in attribs)
> > +        gl.bindAttribLocation(gl.program, i, attribs[i]);
> > +
> > +    // Link the program
> > +    gl.linkProgram(gl.program);
> > +
> > +    // Check the link status
> > +    var linked = gl.getProgramParameter(gl.program, gl.LINK_STATUS);
> > +    if (!linked) {
> > +        // something went wrong with the link
> > +        var error = gl.getProgramInfoLog (gl.program);
> > +        gl.console.log("Error in program linking:"+error);
> > +
> > +        gl.deleteProgram(gl.program);
> > +        gl.deleteProgram(fragmentShader);
> > +        gl.deleteProgram(vertexShader);
> > +
> > +        return null;
> > +    }
> > +
> > +    gl.useProgram(gl.program);
> > +
> > +    gl.clearColor(clearColor[0], clearColor[1], clearColor[2], clearColor[3]);
> > +    gl.clearDepth(clearDepth);
> > +    gl.clearStencil(clearStencil);
> > +    gl.enable(gl.DEPTH_TEST);
> > +    gl.enable(gl.STENCIL_TEST);
> > +    gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT | gl.STENCIL_BUFFER_BIT);
> > +
> > +    return gl;
> > +}
> > +
> > +function drawAndReadPixel(gl, vertices, colors, x, y)
> > +{
> > +    var colorOffset = vertices.byteLength;
> > +
> > +    var vbo = gl.createBuffer();
> > +    gl.bindBuffer(gl.ARRAY_BUFFER, vbo);
> > +    gl.bufferData(gl.ARRAY_BUFFER, colorOffset + colors.byteLength, gl.STATIC_DRAW);
> > +    gl.bufferSubData(gl.ARRAY_BUFFER, 0, vertices);
> > +    gl.bufferSubData(gl.ARRAY_BUFFER, colorOffset, colors);
> > +
> > +    gl.vertexAttribPointer(0, 3, gl.FLOAT, false, 0, 0);
> > +    gl.enableVertexAttribArray(0);
> > +    gl.vertexAttribPointer(1, 4, gl.UNSIGNED_BYTE, true, 0, colorOffset);
> > +    gl.enableVertexAttribArray(1);
> > +
> > +    gl.drawArrays(gl.TRIANGLES, 0, vertices.length / 3);
> > +
> > +    return gl.readPixels(x, y, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE);
> > +}
> Don't we have some common code to do this? If not we should. We will eventually
> have many tests using this technique.
Comment 12 Chris Marrin 2010-03-11 10:31:02 PST
Sorry for all the noise. The browser screwed up my review, which had a lot more stuff in it, and I was trying to recover. But yes, those are the functions I was referring to, and yes, I think it's reasonable to have a new bug on this.
Comment 13 Kenneth Russell 2010-03-11 14:32:06 PST
Comment on attachment 50458 [details]
removed the whitespace modifications

Generally looks good. One issue needs to be fixed in GraphicsContext3DMac.cpp.

> Index: WebCore/platform/graphics/mac/Canvas3DLayer.mm
> ===================================================================
> --- WebCore/platform/graphics/mac/Canvas3DLayer.mm	(revision 55815)
> +++ WebCore/platform/graphics/mac/Canvas3DLayer.mm	(working copy)
> @@ -70,8 +71,8 @@ -(CGLContextObj)copyCGLContextForPixelFo
>  
>  -(void)drawInCGLContext:(CGLContextObj)glContext pixelFormat:(CGLPixelFormatObj)pixelFormat forLayerTime:(CFTimeInterval)timeInterval displayTime:(const CVTimeStamp *)timeStamp
>  {
> -    CGLSetCurrentContext(m_contextObj);
> -    glFinish();
> +    m_context->prepareTexture();

See comment below in GraphicsContext3DMac.cpp.

> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================
> --- WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp	(revision 55820)
> +++ WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp	(working copy)
> @@ -205,25 +215,81 @@ void GraphicsContext3D::reshape(int widt
>      m_currentHeight = height;
>      
>      CGLSetCurrentContext(m_contextObj);
> -    
> +
> +    GLuint internalColorFormat, colorFormat, internalDepthStencilFormat;
> +    if (m_attrs.alpha) {
> +        internalColorFormat = GL_RGBA8;
> +        colorFormat = GL_RGBA;
> +    } else {
> +        internalColorFormat = GL_RGB8;
> +        colorFormat = GL_RGB;
> +    }
> +    if (m_attrs.stencil || m_attrs.depth) {
> +        // We don't allow the logic where stencil is requires and depth is not.

requires -> required

> @@ -243,6 +309,18 @@ void GraphicsContext3D::activeTexture(un
>      ::glActiveTexture(texture);
>  }
>  
> +void GraphicsContext3D::prepareTexture()
> +{
> +    if (m_attrs.antialias) {
> +        ensureContext(m_contextObj);
> +        ::glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, m_multisampleFBO);
> +        ::glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER_EXT, m_fbo);
> +        ::glBlitFramebufferEXT(0, 0, m_currentWidth, m_currentHeight, 0, 0, m_currentWidth, m_currentHeight, GL_COLOR_BUFFER_BIT, GL_LINEAR);
> +        ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_multisampleFBO);

We need to be careful here. When we leave this routine, if the user has bound their own FBO via bindFramebuffer, that is the one we need to restore at the end, not m_multisampleFBO. Also, the logic in bindFramebuffer needs to be updated to bind either m_multisampleFBO or m_fbo when a null framebuffer is passed in. More tracking logic is needed in bindFramebuffer.

> @@ -675,9 +753,25 @@ PassRefPtr<WebGLArray> GraphicsContext3D
>      // FIXME: Also, we should throw when an unacceptable value is passed
>      if (type != GL_UNSIGNED_BYTE || format != GL_RGBA)
>          return 0;
> -        
> +
> +    if (m_attrs.antialias) {
> +        ::glBindFramebufferEXT(GL_READ_FRAMEBUFFER_EXT, m_multisampleFBO);
> +        ::glBindFramebufferEXT(GL_DRAW_FRAMEBUFFER_EXT, m_fbo);
> +        ::glBlitFramebufferEXT(x, y, x + width, y + height, x, y, x + width, y + height, GL_COLOR_BUFFER_BIT, GL_LINEAR);
> +        ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_fbo);
> +    }
>      RefPtr<WebGLUnsignedByteArray> array = WebGLUnsignedByteArray::create(width * height * 4);
>      ::glReadPixels(x, y, width, height, format, type, (GLvoid*) array->data());
> +    if (m_attrs.antialias)
> +        ::glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_multisampleFBO);

This has the same problem as prepareTexture(). First, it isn't guaranteed that the user intended to read from the default frame buffer. If they bound another FBO via bindFramebuffer, then we shouldn't do the blit and binding. Second, we may need to restore to a different FBO than m_multisampleFBO at the end.

> Index: WebKit/chromium/src/GraphicsContext3D.cpp
> ===================================================================
> --- WebKit/chromium/src/GraphicsContext3D.cpp	(revision 55799)
> +++ WebKit/chromium/src/GraphicsContext3D.cpp	(working copy)
> @@ -124,16 +124,23 @@ public:
>                               unsigned long stride, unsigned long offset);
>      void viewportImpl(long x, long y, unsigned long width, unsigned long height);
>  
> +    void readPixels(long x, long y, unsigned long width, unsigned long height, unsigned long format, unsigned long type, void* buffer);
> +
>      void synthesizeGLError(unsigned long error);
>  
>  private:
>      GraphicsContext3D::Attributes m_attrs;
> -
> +    
>      unsigned int m_texture;
>      unsigned int m_fbo;
> -    unsigned int m_depthBuffer;
> +    unsigned int m_depthStencilBuffer;
>      unsigned int m_cachedWidth, m_cachedHeight;
>  
> +    // For multisampling
> +    unsigned int m_multisampleFBO;
> +    unsigned int m_multisampleDepthStencilBuffer;
> +    unsigned int m_multisampleColorBuffer;
> +
>      // For tracking which FBO is bound
>      unsigned int m_boundFBO;
>  
> @@ -351,7 +358,10 @@ GraphicsContext3DInternal::GraphicsConte
>      : m_attrs(attrs)
>      , m_texture(0)
>      , m_fbo(0)
> -    , m_depthBuffer(0)
> +    , m_depthStencilBuffer(0)
> +    , m_multisampleFBO(0)
> +    , m_multisampleDepthStencilBuffer(0)
> +    , m_multisampleColorBuffer(0)
>      , m_boundFBO(0)
>  #ifdef FLIP_FRAMEBUFFER_VERTICALLY
>      , m_scanline(0)
> @@ -375,14 +385,21 @@ GraphicsContext3DInternal::GraphicsConte
>  #error Must port to your platform
>  #endif
>  {
> -    // FIXME: we need to take into account the user's requested
> -    // context creation attributes, in particular stencil and
> -    // antialias, and determine which could and could not be honored
> -    // based on the capabilities of the OpenGL implementation.
> -    m_attrs.alpha = true;
> -    m_attrs.depth = true;
> -    m_attrs.stencil = false;
> -    m_attrs.antialias = false;
> +    // Take into account the user's requested context creation attributes, in
> +    // particular stencil and antialias, and determine which could and could
> +    // not be honored based on the capabilities of the OpenGL implementation.
> +    if (m_attrs.stencil) {
> +        if (GLEW_EXT_packed_depth_stencil) {
> +            if (!m_attrs.depth)
> +                m_attrs.depth = true;
> +        } else {
> +            m_attrs.stencil = false;
> +        }
> +    }
> +    if (m_attrs.antialias && !GLEW_EXT_framebuffer_multisample)
> +        m_attrs.antialias = false;
> +    // FIXME: instead of enforcing premultipliedAlpha = true, implement the
> +    // correct behavior when premultipliedAlpha = false is requested.
>      m_attrs.premultipliedAlpha = true;
>  
>  #if OS(WINDOWS)
> @@ -572,7 +589,15 @@ GraphicsContext3DInternal::~GraphicsCont
>  {
>      makeContextCurrent();
>  #ifndef RENDER_TO_DEBUGGING_WINDOW
> -    glDeleteRenderbuffersEXT(1, &m_depthBuffer);
> +    if (m_attrs.antialias) {
> +        glDeleteRenderbuffersEXT(1, &m_multisampleColorBuffer);
> +        if (m_attrs.depth || m_attrs.stencil)
> +            glDeleteRenderbuffersEXT(1, &m_multisampleDepthStencilBuffer);
> +        glDeleteFramebuffersEXT(1, &m_multisampleFBO);
> +    } else {
> +        if (m_attrs.depth || m_attrs.stencil)
> +            glDeleteRenderbuffersEXT(1, &m_depthStencilBuffer);
> +    }
>      glDeleteTextures(1, &m_texture);
>  #ifdef FLIP_FRAMEBUFFER_VERTICALLY
>      if (m_scanline)
> @@ -669,23 +694,86 @@ void GraphicsContext3DInternal::reshape(
>          m_texture = createTextureObject(target);
>          // Generate the framebuffer object
>          glGenFramebuffersEXT(1, &m_fbo);
> -        // Generate the depth buffer
> -        glGenRenderbuffersEXT(1, &m_depthBuffer);
> +        if (m_attrs.antialias) {
> +            // Generate the multisample framebuffer object
> +            glGenFramebuffersEXT(1, &m_multisampleFBO);
> +            // Generate the multisample color buffer
> +            glGenRenderbuffersEXT(1, &m_multisampleColorBuffer);
> +            if (m_attrs.depth || m_attrs.stencil) {
> +                // Generate the multisample depth/stencil buffer
> +                glGenRenderbuffersEXT(1, &m_multisampleDepthStencilBuffer);
> +            }
> +        } else {
> +            if (m_attrs.depth || m_attrs.stencil) {
> +                // Generate the depth/stencil buffer
> +                glGenRenderbuffersEXT(1, &m_depthStencilBuffer);
> +            }
> +        }
>      }
>  
> -    // Reallocate the color and depth buffers
> +    GLint internalColorFormat, colorFormat, internalDepthStencilFormat;
> +    if (m_attrs.alpha) {
> +        internalColorFormat = GL_RGBA8;
> +        colorFormat = GL_RGBA;
> +    } else {
> +        internalColorFormat = GL_RGB8;
> +        colorFormat = GL_RGB;
> +    }
> +    if (m_attrs.stencil || m_attrs.depth) {
> +        // We don't allow the logic where stencil is required and depth is not.
> +        // See GraphicsContext3DInternal constructor.
> +        if (m_attrs.stencil && m_attrs.depth)
> +            internalDepthStencilFormat = GL_DEPTH24_STENCIL8_EXT;
> +        else
> +            internalDepthStencilFormat = GL_DEPTH_COMPONENT;
> +    }
> +
> +    // Setup multisampling FBO
> +    if (m_attrs.antialias) {
> +        GLint sampleCount = 8;
> +        GLint maxSampleCount;
> +        glGetIntegerv(GL_MAX_SAMPLES_EXT, &maxSampleCount);
> +        if (sampleCount > maxSampleCount)
> +            sampleCount = maxSampleCount;

Use "sampleCount = std::min(sampleCount, maxSampleCount);".
Comment 14 Zhenyao Mo 2010-03-11 18:40:25 PST
Created attachment 50570 [details]
modified acording to Ken Russell review
Comment 15 Kenneth Russell 2010-03-11 18:57:35 PST
Comment on attachment 50570 [details]
modified acording to Ken Russell review 

Looks very good. Hopefully we can get this r+'d quickly.
Comment 16 Oliver Hunt 2010-03-11 19:17:02 PST
Comment on attachment 50570 [details]
modified acording to Ken Russell review 

r=me
Comment 17 WebKit Commit Bot 2010-03-12 09:14:22 PST
Comment on attachment 50570 [details]
modified acording to Ken Russell review 

Rejecting patch 50570 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--release']" exit_code: 1
Last 500 characters of output:
lement.cpp -o /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/HTMLAudioElement.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/GraphicsContext3DMac.o /Users/eseidel/Projects/CommitQueue/WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/662022
Comment 18 Zhenyao Mo 2010-03-12 09:34:34 PST
Created attachment 50603 [details]
fix a warning that my compiler fails to catch
Comment 19 Eric Seidel (no email) 2010-03-15 15:55:32 PDT
Comment on attachment 50570 [details]
modified acording to Ken Russell review 

Cleared Oliver Hunt's review+ from obsolete attachment 50570 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 20 Kenneth Russell 2010-03-15 17:25:19 PDT
Could we please get a review of the last patch? The only change is the fix of the uninitialized variable warning. Thanks.
Comment 21 Darin Fisher (:fishd, Google) 2010-03-16 11:08:25 PDT
Comment on attachment 50603 [details]
fix a warning that my compiler fails to catch

R=me based on Oliver's previous review.
Comment 22 Kenneth Russell 2010-03-16 11:53:56 PDT
Comment on attachment 50603 [details]
fix a warning that my compiler fails to catch

Clearing flags on attachment: 50603

Committed r56074: <http://trac.webkit.org/changeset/56074>
Comment 23 Kenneth Russell 2010-03-16 11:54:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Kenneth Russell 2010-03-17 12:51:03 PDT
This patch uncovered some OpenGL driver bugs on the build bots and after much discussion it is being reverted. It will be re-applied via the commit queue once heuristics have been developed that can detect the driver bugs and (likely) disable antialiasing / multisampling. See the following bugs:

https://bugs.webkit.org/show_bug.cgi?id=36189
https://bugs.webkit.org/show_bug.cgi?id=36194
https://bugs.webkit.org/show_bug.cgi?id=36200
https://bugs.webkit.org/show_bug.cgi?id=36233
Comment 25 Zhenyao Mo 2010-03-23 13:39:27 PDT
Last patch was canceled so now reopen this bug.
Comment 26 Zhenyao Mo 2010-03-23 19:20:32 PDT
Created attachment 51472 [details]
patch after Ken Russell's refac

Added code so that in Mac anti-alias is only on for NVIDIA cards.
Comment 27 Kenneth Russell 2010-03-30 16:35:11 PDT
Comment on attachment 51472 [details]
patch after Ken Russell's refac

Looks good. Thanks for postponing this checkin and sorry for taking so long to look at the updated patch. Only one comment needing fixing; also two tiny typos.

> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================
> --- WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp	(revision 56398)
> +++ WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp	(working copy)
> @@ -174,14 +175,46 @@ GraphicsContext3D::~GraphicsContext3D()
>  {
>      if (m_contextObj) {
>          CGLSetCurrentContext(m_contextObj);
> -        ::glDeleteRenderbuffersEXT(1, & m_depthBuffer);
>          ::glDeleteTextures(1, &m_texture);
> +        if (m_attrs.antialias) {
> +            ::glDeleteRenderbuffersEXT(1, &m_multisampleColorBuffer);
> +            if (m_attrs.stencil || m_attrs.depth)
> +                ::glDeleteRenderbuffersEXT(1, &m_multisampleDepthStencilBuffer);
> +            ::glDeleteFramebuffersEXT(1, &m_multisampleFBO);
> +        } else {
> +            if (m_attrs.stencil || m_attrs.depth)
> +                ::glDeleteRenderbuffersEXT(1, &m_depthStencilBuffer);
> +        }
>          ::glDeleteFramebuffersEXT(1, &m_fbo);
>          CGLSetCurrentContext(0);
>          CGLDestroyContext(m_contextObj);
>      }
>  }
>  
> +void GraphicsContext3D::validateAttributes()
> +{
> +    const char* extensions = reinterpret_cast<const char*>(::glGetString(GL_EXTENSIONS));
> +    if (m_attrs.stencil) {
> +        if (std::strstr(extensions, "GL_EXT_packed_depth_stencil")) {
> +            if (!m_attrs.depth)
> +                m_attrs.depth = true;
> +        } else
> +            m_attrs.stencil = false;
> +    }
> +    if (m_attrs.antialias) {
> +        bool isValidVendor = true;
> +        // Currentky in Mac we only turn on antialias if vendor is NVIDIA.

Currentky -> Currently

> Index: WebKit/chromium/src/GraphicsContext3D.cpp
> ===================================================================
> --- WebKit/chromium/src/GraphicsContext3D.cpp	(revision 56398)
> +++ WebKit/chromium/src/GraphicsContext3D.cpp	(working copy)
> @@ -812,6 +812,17 @@ PassRefPtr<WebGLArray> GraphicsContext3D
>      // FIXME: take into account pack alignment.
>      RefPtr<WebGLUnsignedByteArray> array = WebGLUnsignedByteArray::create(width * height * 4);
>      m_impl->readPixels(x, y, width, height, format, type, array->baseAddress());
> +#if PLATFORM(CG)
> +    WebKit::WebGraphicsContext3D::Attributes webAttributes = m_impl->getContextAttributes();
> +    if (!webAttributes.alpha) {
> +        // If alpha is off, by default glReadPixels should set the alpha to 255 instead of 0.
> +        // This is a hack until glReadPixels fixes its behavior.
> +        GLubyte* data = reinterpret_cast<GLubyte*>(array->baseAddress());
> +        unsigned byteLength = array->byteLength();
> +        for (unsigned i = 3; i < byteLength; i += 4)
> +            data[i] = 255;
> +    }
> +#endif
>      return array;
>  }
>  

Could you move this logic to WebGraphicsContext3DDefaultImpl::readPixels? We are going to want to implement this differently in the command buffer implementation of WebGraphicsContext3D. Note that you will need to expand out the macro currently used to define readPixels in WebGraphicsContext3DDefaultImpl.cpp.

> Index: WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp	(revision 56398)
> +++ WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp	(working copy)
> @@ -374,10 +375,40 @@ bool WebGraphicsContext3DDefaultImpl::in
>          s_initializedGLEW = true;
>      }
>  
> +    m_attributes = attributes;
> +    validateAttributes();
> +
>      m_initialized = true;
>      return true;
>  }
>  
> +void WebGraphicsContext3DDefaultImpl::validateAttributes()
> +{
> +    const char* extensions = reinterpret_cast<const char*>(glGetString(GL_EXTENSIONS));
> +
> +    if (m_attributes.stencil) {
> +        if (std::strstr(extensions, "GL_EXT_packed_depth_stencil")) {
> +            if (!m_attributes.depth)
> +                m_attributes.depth = true;
> +        } else
> +            m_attributes.stencil = false;
> +    }
> +    if (m_attributes.antialias) {
> +        bool isValidVendor = true;
> +#if PLATFORM(CG)
> +        // Currentky in Mac we only turn on antialias if vendor is NVIDIA.

Same typo
Comment 28 Zhenyao Mo 2010-03-30 18:12:19 PDT
Created attachment 52111 [details]
revised patch : responding to Ken Russell's review
Comment 29 Kenneth Russell 2010-03-30 18:18:45 PDT
Comment on attachment 52111 [details]
revised patch : responding to Ken Russell's review

Could you please regenerate the ChangeLogs to reflect the current state of the code? WebKit/chromium/src/GraphicsContext3D.cpp still shows up as modified, even though the patch no longer touches it.
Comment 30 Zhenyao Mo 2010-03-30 18:29:19 PDT
Created attachment 52114 [details]
revised patch : fixed the changeLog
Comment 31 Kenneth Russell 2010-03-30 18:52:34 PDT
Comment on attachment 52114 [details]
revised patch : fixed the changeLog

Looks good.
Comment 32 Darin Fisher (:fishd, Google) 2010-03-30 22:38:43 PDT
Comment on attachment 52114 [details]
revised patch : fixed the changeLog

> Index: WebKit/chromium/public/WebGraphicsContext3D.h
...
>      // This destructor needs to be public so that using classes can destroy instances if initialization fails.
> -    virtual ~WebGraphicsContext3D() {}
> +    virtual ~WebGraphicsContext3D();
...
> Index: WebKit/chromium/src/WebGraphicsContext3D.cpp
...
> +WebGraphicsContext3D::~WebGraphicsContext3D()
> +{
> +}

why this change?  please note that if this destructor needs to be exported
from WebKit that we'll need to add WEBKIT_API to its declaration, but I
think it would be better to leave it implemented inline in the header file.
Comment 33 Kenneth Russell 2010-03-31 01:19:43 PDT
(In reply to comment #32)
> (From update of attachment 52114 [details])
> > Index: WebKit/chromium/public/WebGraphicsContext3D.h
> ...
> >      // This destructor needs to be public so that using classes can destroy instances if initialization fails.
> > -    virtual ~WebGraphicsContext3D() {}
> > +    virtual ~WebGraphicsContext3D();
> ...
> > Index: WebKit/chromium/src/WebGraphicsContext3D.cpp
> ...
> > +WebGraphicsContext3D::~WebGraphicsContext3D()
> > +{
> > +}
> 
> why this change?  please note that if this destructor needs to be exported
> from WebKit that we'll need to add WEBKIT_API to its declaration, but I
> think it would be better to leave it implemented inline in the header file.

With Mo's changes we saw bizarre linker errors claiming that the vtbl of WebGraphicsContext3D was undefined. As far as I know the rule is that the translation unit containing the definition of the first virtual function for a class is the one into which the vtbl is emitted. Because this destructor was both virtual and inline I think that was breaking the compiler's heuristics about where to emit the vtbl. Moving the body to the .cpp file solved this error.

It is possible that the error was triggered due to another bug in the code at the time. Mo, can you try moving the body of the destructor back into the header and see whether it compiles and links properly?
Comment 34 Darin Fisher (:fishd, Google) 2010-03-31 07:54:34 PDT
I'd be shocked if the destructor being inlined caused problems since we have so many other virtual and inline destructors in the codebase.
Comment 35 Zhenyao Mo 2010-03-31 11:01:37 PDT
Moved the destructor inline and it linked fine.  I guess the original linking problem was caused by something else.  Will revise.
Comment 36 Zhenyao Mo 2010-03-31 11:03:40 PDT
Created attachment 52187 [details]
revised patch: responding to Darin Fisher's review.
Comment 37 Kenneth Russell 2010-03-31 11:18:40 PDT
(In reply to comment #36)
> Created an attachment (id=52187) [details]
> revised patch: responding to Darin Fisher's review.

Looks good to me.
Comment 38 WebKit Commit Bot 2010-03-31 15:15:11 PDT
Comment on attachment 52187 [details]
revised patch: responding to Darin Fisher's review.

Clearing flags on attachment: 52187

Committed r56872: <http://trac.webkit.org/changeset/56872>
Comment 39 WebKit Commit Bot 2010-03-31 15:15:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Eric Seidel (no email) 2010-03-31 17:34:02 PDT
This introduced 3 failures on Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r56872%20(7489)/results.html

This would have been automatically rolled out if Snow Leopard was a core builder, but since it isn't you're getting this manual warning from me. :)

Please fix or I'll roll this out later this evening/tomorrow morning.
Comment 41 Kenneth Russell 2010-03-31 17:47:19 PDT
(In reply to comment #40)
> This introduced 3 failures on Snow Leopard:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r56872%20(7489)/results.html
> 
> This would have been automatically rolled out if Snow Leopard was a core
> builder, but since it isn't you're getting this manual warning from me. :)
> 
> Please fix or I'll roll this out later this evening/tomorrow morning.

We're working on fixing these test failures right now. See bug 36910. The tests are actually incorrect; the new code is working. Please don't roll this out -- we've been trying to get it landed for a couple of weeks.
Comment 42 Eric Seidel (no email) 2010-03-31 17:50:21 PDT
Sorry, my comment sounded way more like a "threat" than it should have. :)  I have no intention of rolling it out since someone is working on it!  I just wanted to assure the rest of webkit.org that since the commit was under my name I would take responsibility for making sure the bots got un-broke.  Glad to hear you're on it!
Comment 43 Simon Fraser (smfr) 2010-05-28 17:03:33 PDT
This cause a bad performance and flickering regression on my hardware: bug 38560.