Bug 30349 – WebGL LayoutTests fail on buildbot

• Component: WebGL
• Status: RESOLVED FIXED

[Chris Marrin]

Looks like it is failing because it is not able to get the desired pixel format. This may be due to the monitor being plugged in, or because the card is old or something. I need to make the code more robust in the face of a pixel format failure, make it succeed in more cases (on this buildbot if possible) and somehow avoid failing if the target machine can't run the webgl tests.

[Simon Fraser (smfr)]

<rdar://problem/7301008>

[Chris Marrin]

Created attachment 41177 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 41177 [details]
Patch

I don't like having to add this notion of validity everywhere. I think it would be better to just not be able to construct a GraphicsContext3D if the 3d is not available.

[Chris Marrin]

Created attachment 41960 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 41960 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================

> +        (WebCore::CanvasRenderingContext3D::create):
> +        (WebCore::CanvasRenderingContext3D::CanvasRenderingContext3D):
> +        (WebCore::CanvasRenderingContext3D::beginPaint):
> +        (WebCore::CanvasRenderingContext3D::endPaint):
> +        (WebCore::CanvasRenderingContext3D::reshape):
> +        (WebCore::CanvasRenderingContext3D::sizeInBytes):
> +        (WebCore::CanvasRenderingContext3D::activeTexture):
> +        (WebCore::CanvasRenderingContext3D::attachShader):
> +        (WebCore::CanvasRenderingContext3D::bindAttribLocation):
> +        (WebCore::CanvasRenderingContext3D::bindBuffer):
> +        (WebCore::CanvasRenderingContext3D::bindFramebuffer):
> +        (WebCore::CanvasRenderingContext3D::bindRenderbuffer):
> +        (WebCore::CanvasRenderingContext3D::bindTexture):
> +        (WebCore::CanvasRenderingContext3D::blendColor):
> +        (WebCore::CanvasRenderingContext3D::blendEquation):
> +        (WebCore::CanvasRenderingContext3D::blendEquationSeparate):
> +        (WebCore::CanvasRenderingContext3D::blendFunc):
> +        (WebCore::CanvasRenderingContext3D::blendFuncSeparate):
> +        (WebCore::CanvasRenderingContext3D::bufferData):
> +        (WebCore::CanvasRenderingContext3D::bufferSubData):
> +        (WebCore::CanvasRenderingContext3D::checkFramebufferStatus):
> +        (WebCore::CanvasRenderingContext3D::clear):
> +        (WebCore::CanvasRenderingContext3D::clearColor):
> +        (WebCore::CanvasRenderingContext3D::clearDepth):
> +        (WebCore::CanvasRenderingContext3D::clearStencil):
> +        (WebCore::CanvasRenderingContext3D::colorMask):
> +        (WebCore::CanvasRenderingContext3D::compileShader):
> +        (WebCore::CanvasRenderingContext3D::copyTexImage2D):
> +        (WebCore::CanvasRenderingContext3D::copyTexSubImage2D):
> +        (WebCore::CanvasRenderingContext3D::cullFace):
> +        (WebCore::CanvasRenderingContext3D::depthFunc):
> +        (WebCore::CanvasRenderingContext3D::depthMask):
> +        (WebCore::CanvasRenderingContext3D::depthRange):
> +        (WebCore::CanvasRenderingContext3D::detachShader):
> +        (WebCore::CanvasRenderingContext3D::disable):
> +        (WebCore::CanvasRenderingContext3D::disableVertexAttribArray):
> +        (WebCore::CanvasRenderingContext3D::drawArrays):
> +        (WebCore::CanvasRenderingContext3D::drawElements):
> +        (WebCore::CanvasRenderingContext3D::enable):
> +        (WebCore::CanvasRenderingContext3D::enableVertexAttribArray):
> +        (WebCore::CanvasRenderingContext3D::finish):
> +        (WebCore::CanvasRenderingContext3D::flush):
> +        (WebCore::CanvasRenderingContext3D::framebufferRenderbuffer):
> +        (WebCore::CanvasRenderingContext3D::framebufferTexture2D):
> +        (WebCore::CanvasRenderingContext3D::frontFace):
> +        (WebCore::CanvasRenderingContext3D::generateMipmap):
> +        (WebCore::CanvasRenderingContext3D::getActiveAttrib):
> +        (WebCore::CanvasRenderingContext3D::getActiveUniform):
> +        (WebCore::CanvasRenderingContext3D::getAttribLocation):
> +        (WebCore::CanvasRenderingContext3D::getBoolean):
> +        (WebCore::CanvasRenderingContext3D::getBooleanv):
> +        (WebCore::CanvasRenderingContext3D::getBufferParameteri):
> +        (WebCore::CanvasRenderingContext3D::getBufferParameteriv):
> +        (WebCore::CanvasRenderingContext3D::getError):
> +        (WebCore::CanvasRenderingContext3D::getFloat):
> +        (WebCore::CanvasRenderingContext3D::getFloatv):
> +        (WebCore::CanvasRenderingContext3D::getFramebufferAttachmentParameteri):
> +        (WebCore::CanvasRenderingContext3D::getFramebufferAttachmentParameteriv):
> +        (WebCore::CanvasRenderingContext3D::getInteger):
> +        (WebCore::CanvasRenderingContext3D::getIntegerv):
> +        (WebCore::CanvasRenderingContext3D::getProgrami):
> +        (WebCore::CanvasRenderingContext3D::getProgramiv):
> +        (WebCore::CanvasRenderingContext3D::getProgramInfoLog):
> +        (WebCore::CanvasRenderingContext3D::getRenderbufferParameteri):
> +        (WebCore::CanvasRenderingContext3D::getRenderbufferParameteriv):
> +        (WebCore::CanvasRenderingContext3D::getShaderi):
> +        (WebCore::CanvasRenderingContext3D::getShaderiv):
> +        (WebCore::CanvasRenderingContext3D::getShaderInfoLog):
> +        (WebCore::CanvasRenderingContext3D::getShaderSource):
> +        (WebCore::CanvasRenderingContext3D::getString):
> +        (WebCore::CanvasRenderingContext3D::getTexParameterf):
> +        (WebCore::CanvasRenderingContext3D::getTexParameterfv):
> +        (WebCore::CanvasRenderingContext3D::getTexParameteri):
> +        (WebCore::CanvasRenderingContext3D::getTexParameteriv):
> +        (WebCore::CanvasRenderingContext3D::getUniformf):
> +        (WebCore::CanvasRenderingContext3D::getUniformfv):
> +        (WebCore::CanvasRenderingContext3D::getUniformi):
> +        (WebCore::CanvasRenderingContext3D::getUniformiv):
> +        (WebCore::CanvasRenderingContext3D::getUniformLocation):
> +        (WebCore::CanvasRenderingContext3D::getVertexAttribf):
> +        (WebCore::CanvasRenderingContext3D::getVertexAttribfv):
> +        (WebCore::CanvasRenderingContext3D::getVertexAttribi):
> +        (WebCore::CanvasRenderingContext3D::getVertexAttribiv):
> +        (WebCore::CanvasRenderingContext3D::getVertexAttribOffset):
> +        (WebCore::CanvasRenderingContext3D::hint):
> +        (WebCore::CanvasRenderingContext3D::isBuffer):
> +        (WebCore::CanvasRenderingContext3D::isEnabled):
> +        (WebCore::CanvasRenderingContext3D::isFramebuffer):
> +        (WebCore::CanvasRenderingContext3D::isProgram):
> +        (WebCore::CanvasRenderingContext3D::isRenderbuffer):
> +        (WebCore::CanvasRenderingContext3D::isShader):
> +        (WebCore::CanvasRenderingContext3D::isTexture):
> +        (WebCore::CanvasRenderingContext3D::lineWidth):
> +        (WebCore::CanvasRenderingContext3D::linkProgram):
> +        (WebCore::CanvasRenderingContext3D::pixelStorei):
> +        (WebCore::CanvasRenderingContext3D::polygonOffset):
> +        (WebCore::CanvasRenderingContext3D::readPixels):
> +        (WebCore::CanvasRenderingContext3D::releaseShaderCompiler):
> +        (WebCore::CanvasRenderingContext3D::renderbufferStorage):
> +        (WebCore::CanvasRenderingContext3D::sampleCoverage):
> +        (WebCore::CanvasRenderingContext3D::scissor):
> +        (WebCore::CanvasRenderingContext3D::shaderSource):
> +        (WebCore::CanvasRenderingContext3D::stencilFunc):
> +        (WebCore::CanvasRenderingContext3D::stencilFuncSeparate):
> +        (WebCore::CanvasRenderingContext3D::stencilMask):
> +        (WebCore::CanvasRenderingContext3D::stencilMaskSeparate):
> +        (WebCore::CanvasRenderingContext3D::stencilOp):
> +        (WebCore::CanvasRenderingContext3D::stencilOpSeparate):
> +        (WebCore::CanvasRenderingContext3D::texImage2D):
> +        (WebCore::CanvasRenderingContext3D::texParameterf):
> +        (WebCore::CanvasRenderingContext3D::texParameteri):
> +        (WebCore::CanvasRenderingContext3D::texSubImage2D):
> +        (WebCore::CanvasRenderingContext3D::uniform1f):
> +        (WebCore::CanvasRenderingContext3D::uniform1fv):
> +        (WebCore::CanvasRenderingContext3D::uniform1i):
> +        (WebCore::CanvasRenderingContext3D::uniform1iv):
> +        (WebCore::CanvasRenderingContext3D::uniform2f):
> +        (WebCore::CanvasRenderingContext3D::uniform2fv):
> +        (WebCore::CanvasRenderingContext3D::uniform2i):
> +        (WebCore::CanvasRenderingContext3D::uniform2iv):
> +        (WebCore::CanvasRenderingContext3D::uniform3f):
> +        (WebCore::CanvasRenderingContext3D::uniform3fv):
> +        (WebCore::CanvasRenderingContext3D::uniform3i):
> +        (WebCore::CanvasRenderingContext3D::uniform3iv):
> +        (WebCore::CanvasRenderingContext3D::uniform4f):
> +        (WebCore::CanvasRenderingContext3D::uniform4fv):
> +        (WebCore::CanvasRenderingContext3D::uniform4i):
> +        (WebCore::CanvasRenderingContext3D::uniform4iv):
> +        (WebCore::CanvasRenderingContext3D::uniformMatrix2fv):
> +        (WebCore::CanvasRenderingContext3D::uniformMatrix3fv):
> +        (WebCore::CanvasRenderingContext3D::uniformMatrix4fv):
> +        (WebCore::CanvasRenderingContext3D::useProgram):
> +        (WebCore::CanvasRenderingContext3D::validateProgram):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib1f):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib1fv):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib2f):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib2fv):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib3f):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib3fv):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib4f):
> +        (WebCore::CanvasRenderingContext3D::vertexAttrib4fv):
> +        (WebCore::CanvasRenderingContext3D::vertexAttribPointer):
> +        (WebCore::CanvasRenderingContext3D::viewport):

No need for this huge list.


> Index: WebCore/html/canvas/CanvasRenderingContext3D.cpp
> ===================================================================

> +CanvasRenderingContext3D::CanvasRenderingContext3D(HTMLCanvasElement* canvas, GraphicsContext3D* context)
>      : CanvasRenderingContext(canvas)
> +    , m_context(context)
>      , m_needsUpdate(true)
>      , m_markedCanvasDirty(false)
>  {

I think an ASSERT(context) would make the contract clearer.

> Index: WebCore/html/canvas/CanvasRenderingContext3D.h
> ===================================================================

>  
> -        GraphicsContext3D* graphicsContext3D() { return &m_context; }
> +        GraphicsContext3D* graphicsContext3D() { return m_context.get(); }

As I mentioned in previous reviews, this getter should be "const".

> Index: WebCore/platform/graphics/mac/Canvas3DLayer.mm
> ===================================================================
> --- WebCore/platform/graphics/mac/Canvas3DLayer.mm	(revision 50115)
> +++ WebCore/platform/graphics/mac/Canvas3DLayer.mm	(working copy)
> @@ -50,6 +50,8 @@ using namespace WebCore;
>  
>  -(CGLPixelFormatObj)copyCGLPixelFormatForDisplayMask:(uint32_t)mask
>  {
> +    UNUSED_PARAM(mask);
> +    /*
>      CGLPixelFormatAttribute attribs[] =
>      {
>          (CGLPixelFormatAttribute) kCGLPFAAccelerated,
> @@ -63,6 +65,9 @@ using namespace WebCore;
>   
>      CGLChoosePixelFormat(attribs, &pixelFormatObj, &numPixelFormats);
>      return pixelFormatObj;
> +    */

Why are you commenting out this code? If it shouldn't be there, remove it.

> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================

> +static void setPixelFormat(CGLPixelFormatAttribute* attribs, int colorBits, int depthBits, 
> +                           bool accelerated, bool supersample, bool closest)
> +{
> +    *attribs++ = kCGLPFAColorSize;
> +    *attribs++ = static_cast<CGLPixelFormatAttribute>(colorBits);
> +    *attribs++ = kCGLPFADepthSize;
> +    *attribs++ = static_cast<CGLPixelFormatAttribute>(depthBits);

I don't like this at all. This method just blindly assumes that the caller has allocated enough space.

You could use a Vector<CGLPixelFormatAttribute>, which would be much cleaner.

> @@ -176,7 +200,7 @@ void GraphicsContext3D::endPaint()
>  
>  void GraphicsContext3D::reshape(int width, int height)
>  {
> -    if (width == m_currentWidth && height == m_currentHeight)
> +    if (width == m_currentWidth && height == m_currentHeight || !m_contextObj)
>          return;

How do you ever get here with m_contextObj == 0 ?

>      m_currentWidth = width;
> @@ -208,6 +232,9 @@ void GraphicsContext3D::reshape(int widt
>  
>  static inline void ensureContext(CGLContextObj context)
>  {
> +    if (!context)
> +        return;
> +        
>      CGLContextObj currentContext = CGLGetCurrentContext();
>      if (currentContext != context)
>          CGLSetCurrentContext(context);

r- to clean up the setPixelFormat() stuff, but in general it looks ok.

[Darin Adler]

Comment on attachment 41960 [details]
Patch

I think that CanvasRenderContext.h needs a comment explaining that ref/deref just ref/deref the canvas.

I think that m_canvas in CanvasRenderContext should be private, not protected.

> +CanvasRenderingContext3D* CanvasRenderingContext3D::create(HTMLCanvasElement* canvas)

This function should return a PassOwnPtr<CanvasRenderingContext3D>.

> +        static GraphicsContext3D* create();

This function should return a PassOwnPtr<GraphicsContext3D>.

>  -(CGLPixelFormatObj)copyCGLPixelFormatForDisplayMask:(uint32_t)mask
>  {
> +    UNUSED_PARAM(mask);
> +    /*
>      CGLPixelFormatAttribute attribs[] =
>      {
>          (CGLPixelFormatAttribute) kCGLPFAAccelerated,
> @@ -63,6 +65,9 @@ using namespace WebCore;
>   
>      CGLChoosePixelFormat(attribs, &pixelFormatObj, &numPixelFormats);
>      return pixelFormatObj;
> +    */
> +    
> +    return CGLRetainPixelFormat(CGLGetPixelFormat(m_contextObj));
>  }

Normally we do not leave commented-out code in WebKit. Could you just delete this instead?

> +static void setPixelFormat(CGLPixelFormatAttribute* attribs, int colorBits, int depthBits, 
> +                           bool accelerated, bool supersample, bool closest)

Normally in WebKit we don't line up the second line with the parenthesis like this.

> +    *attribs++ = static_cast<CGLPixelFormatAttribute>(0);

I'm surprised this cast is helpful or

> +GraphicsContext3D* GraphicsContext3D::create()
> +{
> +    GraphicsContext3D* context = new GraphicsContext3D();
> +    if (context->m_contextObj)
> +        return context;
> +    delete context;
> +    return 0;
> +}

It would be slightly better to use OwnPtr inside this function. Also no need for the parentheses in the new expression.

>  void GraphicsContext3D::reshape(int width, int height)
>  {
> -    if (width == m_currentWidth && height == m_currentHeight)
> +    if (width == m_currentWidth && height == m_currentHeight || !m_contextObj)
>          return;

Is this needed? You made the create function return 0 if m_contextObj is 0. How can this ever be called with an m_contextObj of zero?

>  static inline void ensureContext(CGLContextObj context)
>  {
> +    if (!context)
> +        return;

Same question. Does this get called with a context of 0? From where?

Seems on the right track, but review- because I think you should be using PassOwnPtr.

[Chris Marrin]

Created attachment 41967 [details]
Replacement patch

[Darin Adler]

Comment on attachment 41967 [details]
Replacement patch

Looks like you addressed Simon’s comments, but not mine.

[Chris Marrin]

Created attachment 41976 [details]
replacement patch

This addresses Simon and Darin's comments

[Simon Fraser (smfr)]

Comment on attachment 41976 [details]
replacement patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 50169)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,34 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +        Make WebGL context failure more robust
> +        https://bugs.webkit.org/show_bug.cgi?id=30349
> +
> +        The buildbot failure has already been fixed and checked in. This

This is not a useful comment in a changelog; there isn't enough context. Just say what the current patch is doing.

> +void CanvasRenderingContext2D::drawImage(HTMLCanvasElement* passedCanvas, const FloatRect& srcRect,

passedCanvas is vague. Is it the source or the destination?


> Index: WebCore/html/canvas/CanvasRenderingContext3D.cpp
> ===================================================================
> --- WebCore/html/canvas/CanvasRenderingContext3D.cpp	(revision 50115)
> +++ WebCore/html/canvas/CanvasRenderingContext3D.cpp	(working copy)
> @@ -42,12 +42,23 @@
>  
>  namespace WebCore {
>  
> -CanvasRenderingContext3D::CanvasRenderingContext3D(HTMLCanvasElement* canvas)
> -    : CanvasRenderingContext(canvas)
> +CanvasRenderingContext3D* CanvasRenderingContext3D::create(HTMLCanvasElement* canvas)

Darin said:
> This function should return a PassOwnPtr<CanvasRenderingContext3D>.

[Darin Adler]

Comment on attachment 41976 [details]
replacement patch

My comments are similar to Simon's.

> -void CanvasRenderingContext2D::drawImage(HTMLCanvasElement* canvas, const FloatRect& srcRect,
> +void CanvasRenderingContext2D::drawImage(HTMLCanvasElement* passedCanvas, const FloatRect& srcRect,

I think sourceCanvas or srcCanvas would be clearer.

> +CanvasRenderingContext3D* CanvasRenderingContext3D::create(HTMLCanvasElement* canvas)

I suggested changing the return type of this function to PassOwnPtr. Did you decide not to? Maybe you confused this comment with the similar one about GraphicsContext3D::create?

> +    PassOwnPtr<GraphicsContext3D> context = GraphicsContext3D::create();

This local variable should be OwnPtr, not PassOwnPtr.

> +CanvasRenderingContext3D::CanvasRenderingContext3D(HTMLCanvasElement* passedCanvas, PassOwnPtr<GraphicsContext3D> context)
> +    : CanvasRenderingContext(passedCanvas)

You could call it targetCanvas. But here passedCanvas seems OK.

> +        // Ref and deref the m_canvas

We normally put a period on a comment like this one.

> -    if (width == m_currentWidth && height == m_currentHeight)
> +    if (width == m_currentWidth && height == m_currentHeight || !m_contextObj)
>          return;

I believe you added this because reshape is called in the constructor. I asked you this in my last review comments, and you didn't answer or change the code. But I think I figured the answer out myself.

>  static inline void ensureContext(CGLContextObj context)
>  {
> +    if (!context)
> +        return;

I asked why you needed to add this, but you didn't say.

[Chris Marrin]

Created attachment 41983 [details]
replacement patch

[Simon Fraser (smfr)]

Comment on attachment 41983 [details]
replacement patch

> Index: WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
> ===================================================================

> +static void setPixelFormat(Vector<CGLPixelFormatAttribute>& attribs, int colorBits, int depthBits, bool accelerated, bool supersample, bool closest)
> +{
> +    attribs.clear();
> +    
> +    attribs.append(kCGLPFAColorSize);
> +    attribs.append(static_cast<CGLPixelFormatAttribute>(colorBits));
> +    attribs.append(kCGLPFADepthSize);
> +    attribs.append(static_cast<CGLPixelFormatAttribute>(depthBits));

So much better!

> +PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create()
> +{
> +    PassOwnPtr<GraphicsContext3D> context = new GraphicsContext3D();

I think you should use an OwnPtr on the stack here.

> +    setPixelFormat(attribs, 32, 32, true, true, false);

> +        setPixelFormat(attribs, 32, 32, true, false, false);

> +            setPixelFormat(attribs, 32, 16, true, false, false);

> +                setPixelFormat(attribs, 32, 16, false, false, true);

This would be even more readable if we had enum values rather than 'true', 'false' here. But OK for now (since the called method is just above).

r=me

[Chris Marrin]

Sending        WebCore/ChangeLog
Sending        WebCore/html/HTMLCanvasElement.cpp
Sending        WebCore/html/canvas/CanvasRenderingContext.h
Sending        WebCore/html/canvas/CanvasRenderingContext2D.cpp
Sending        WebCore/html/canvas/CanvasRenderingContext3D.cpp
Sending        WebCore/html/canvas/CanvasRenderingContext3D.h
Sending        WebCore/platform/graphics/GraphicsContext3D.h
Sending        WebCore/platform/graphics/mac/Canvas3DLayer.mm
Sending        WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp
Transmitting file data .........
Committed revision 50181.

[Jeremy Orlow]

> +PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create()
> +{
> +    PassOwnPtr<GraphicsContext3D> context = new GraphicsContext3D();

If these were RefPtr's, it seems like http://webkit.org/coding/RefPtr.html would suggest this should be a OwnPtr not a PassOwnPtr.  I've never used a PassOwnPtr though, so maybe I'm off base?

> +    return context->m_contextObj ? context : 0;
> +}

[Simon Fraser (smfr)]

Jeremy: it got fixed before the commit:

PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create()
{
    OwnPtr<GraphicsContext3D> context(new GraphicsContext3D());
    return context->m_contextObj ? context.release() : 0;
}

[Jeremy Orlow]

(In reply to comment #16)
> Jeremy: it got fixed before the commit:
> 
> PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create()
> {
>     OwnPtr<GraphicsContext3D> context(new GraphicsContext3D());
>     return context->m_contextObj ? context.release() : 0;
> }

Gotcha.  Sorry for the noise.  :-)

[Eric Seidel (no email)]

This change broke a security test:
http/tests/security/canvas-remote-read-remote-image.html

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r50184%20(6527)/http/tests/security/canvas-remote-read-remote-image-pretty-diff.html

I'll see if I can see the easy fix in the patch.  If not, then I'll start a rollout since Chris isn't around. :(

[Simon Fraser (smfr)]

Fixing the test bustage via bug 30840.