[Webkit-unassigned] [Bug 56825] [Qt] fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html fails

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56825





--- Comment #15 from Jarkko Sakkinen <jarkko.j.sakkinen at gmail.com>  2011-03-25 06:30:58 PST ---
(In reply to comment #14)
> (From update of attachment 86922 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86922&action=review
> 
> quick review
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:313
> > +    else
> > +        m_glWidget = new QGLWidget();
> 
> Shouldn't we get rid of this? Just set m_valid = false and return?
> If the viewport does not have QGLWidget -> too bad for you, you won't enjoy webgl :)
> 

No. For that case I think the best compromise is to allow creation of WebGL context but disallow painting of it. In the paint method there should be check that we are painting on a QImage before allowing software fallback. Anyway, let's leave fixing of this issue to that bug that I created :)

> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:-325
> > -    m_attrs.alpha = format.alpha();
> 
> Why can you skip checking the alpha from the GL surface?
> You use m_attrs.alpha in reshape(), but as far as I can see, it is initialized with the default value.

Default values for enabling/disabling are defined:
- Second parameter given by getContext call from JavaScript
- GraphicsContext3D::Attributes default constructor 

This was invalid code. That's why the test case failed (it uses
getContext() to enable/disable alpha).

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:-327
> > -    m_attrs.depth = format.depth();
> > -    m_attrs.stencil = format.stencil();
> 
> Ditto :)
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:322
> > +    // No need for widgets viewport. We use QGLWidget only to get a separate
> > +    // OpenGL context.
> 
> The comment can be on one line, we don't limit lines at 80 characters in WebKit.

Roger.

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:428
> > +        m_valid = false;
> 
> This assignement is useless, the condition to enter this branch is
> if (!m_valid)
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:507
> > +    // Construct canvas FBO
> 
> Comments must be sentence, so they must end with a full stop.
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:512
> > +#if !defined(QT_OPENGL_ES_2)
> 
> Do you have to #ifdef here, it looks like m_attrs.stencil is always gonna be false for QT_OPENGL_ES_2 because of the way it is initialized.
> 
> I am not aware of the problem of the stencil buffer with OpenGL ES 2. So my question is: would the following call make sense if stencil problems were fixed of GL ES 2?

There's no problem with ES 2.0 but stencil buffer has to be made as
separate renderbuffer there. http://www.khronos.org/opengles/sdk/docs/man/xhtml/glRenderbufferStorage.xml I created separate bug for fixing the same test for ES 2 because I don't have a N900 that I could use to test this.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.