Summary: | WebGL LayoutTests fail on buildbot | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Marrin <cmarrin> | ||||||||||||
Component: | WebGL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | eric, jorlow, simon.fraser | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Chris Marrin
2009-10-13 18:57:35 PDT
Created attachment 41177 [details]
Patch
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.
Created attachment 41960 [details]
Patch
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. 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. Created attachment 41967 [details]
Replacement patch
Comment on attachment 41967 [details]
Replacement patch
Looks like you addressed Simon’s comments, but not mine.
Created attachment 41976 [details]
replacement patch
This addresses Simon and Darin's comments
Comment on attachment 41976 [details] replacement patch > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 50169) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,34 @@ > +2009-10-27 Chris Marrin <cmarrin@apple.com> > + > + 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>. 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. Created attachment 41983 [details]
replacement patch
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 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. > +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; > +} Jeremy: it got fixed before the commit: PassOwnPtr<GraphicsContext3D> GraphicsContext3D::create() { OwnPtr<GraphicsContext3D> context(new GraphicsContext3D()); return context->m_contextObj ? context.release() : 0; } (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. :-) 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. :( Fixing the test bustage via bug 30840. |