Bug 33416 – Hook up WebGLContextAttributes to OpenGL context creation code

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Zhenyao Mo]

Created attachment 50438 [details]
patch

[Zhenyao Mo]

Created attachment 50441 [details]
modified patch

[WebKit Review Bot]

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.

[Zhenyao Mo]

Created attachment 50442 [details]
fixed style problems

[Chris Marrin]

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.

[David Levin]

Comment on attachment 50442 [details]
fixed style problems

r- based on Chris's comment, which I agree with.

[Zhenyao Mo]

Created attachment 50458 [details]
removed the whitespace modifications

[Chris Marrin]

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.

[Chris Marrin]

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.

[Chris Marrin]

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.

[Zhenyao Mo]

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.

[Chris Marrin]

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.

[Kenneth Russell]

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);".

[Zhenyao Mo]

Created attachment 50570 [details]
modified acording to Ken Russell review

[Kenneth Russell]

Comment on attachment 50570 [details]
modified acording to Ken Russell review 

Looks very good. Hopefully we can get this r+'d quickly.

[Oliver Hunt]

Comment on attachment 50570 [details]
modified acording to Ken Russell review 

r=me

[WebKit Commit Bot]

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

[Zhenyao Mo]

Created attachment 50603 [details]
fix a warning that my compiler fails to catch

[Eric Seidel (no email)]

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.

[Kenneth Russell]

Could we please get a review of the last patch? The only change is the fix of the uninitialized variable warning. Thanks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50603 [details]
fix a warning that my compiler fails to catch

R=me based on Oliver's previous review.

[Kenneth Russell]

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>

[Kenneth Russell]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

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

[Zhenyao Mo]

Last patch was canceled so now reopen this bug.

[Zhenyao Mo]

Created attachment 51472 [details]
patch after Ken Russell's refac

Added code so that in Mac anti-alias is only on for NVIDIA cards.

[Kenneth Russell]

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

[Zhenyao Mo]

Created attachment 52111 [details]
revised patch : responding to Ken Russell's review

[Kenneth Russell]

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.

[Zhenyao Mo]

Created attachment 52114 [details]
revised patch : fixed the changeLog

[Kenneth Russell]

Comment on attachment 52114 [details]
revised patch : fixed the changeLog

Looks good.

[Darin Fisher (:fishd, Google)]

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.

[Kenneth Russell]

(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?

[Darin Fisher (:fishd, Google)]

I'd be shocked if the destructor being inlined caused problems since we have so many other virtual and inline destructors in the codebase.

[Zhenyao Mo]

Moved the destructor inline and it linked fine.  I guess the original linking problem was caused by something else.  Will revise.

[Zhenyao Mo]

Created attachment 52187 [details]
revised patch: responding to Darin Fisher's review.

[Kenneth Russell]

(In reply to comment #36)
> Created an attachment (id=52187) [details]
> revised patch: responding to Darin Fisher's review.

Looks good to me.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Kenneth Russell]

(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.

[Eric Seidel (no email)]

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!

[Simon Fraser (smfr)]

This cause a bad performance and flickering regression on my hardware: bug 38560.