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

• Component: WebGL
• Status: RESOLVED FIXED

[Jarkko Sakkinen]

LayoutTest fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html fails.

[Jarkko Sakkinen]

I'm working on a fix for this. I plan to do also small refactoring:
- Get common code from GraphicsContext3DInternal constructor and GraphicsContext3D::reshape().
- Create bool reshape(int width, int height) that will be called by both of these functions in order to remove duplicate code.
So that I don't have to do same fixes into two places. This is probably OK?

[Jarkko Sakkinen]

Created attachment 86465 [details]
Create common reshaping function for main FBO

Does this work this way that one can create multiple patches for a single bug? :)

[Jarkko Sakkinen]

Created attachment 86508 [details]
Fixes for context attribute handling.

Why stencil handling is done how it is done (always enable stencil when depth is enabled). Look at http://www.opengl.org/wiki/GL_EXT_framebuffer_object: "NEVER EVER MAKE A STENCIL buffer. All GPUs and all drivers do not support an independent stencil buffer. If you need a stencil buffer, then you need to make a Depth=24, Stencil=8 buffer, also called D24S8. Please search for the example about GL_EXT_packed_depth_stencil on this page. "

For now for OpenGL ES 2.0 I've implemented only depth handling because I don't have N900 or any other ES 2.0 device to develop and test the fixes for that part so context attribute stencil is set to false there always.

[Benjamin Poulain]

No'am, could you have a look?

[Noam Rosenthal]

LGTM

[Jarkko Sakkinen]

Should I create bug about enabling stencil buffer with ES 2.0 (enhancement) with low priority? I don't think it is very important for the mobile ATM because it is rarely used feature in practice. It is, however, good to have this limitation in the backlog.

[Benjamin Poulain]

(In reply to comment #6)
> Should I create bug about enabling stencil buffer with ES 2.0 (enhancement) with low priority? I don't think it is very important for the mobile ATM because it is rarely used feature in practice. It is, however, good to have this limitation in the backlog.

Yes please.
It is better to have a bug open. If you know of a test failing because of it, you can also put the name of the test in the title.

[Jarkko Sakkinen]

I created bug for ES 2.0: https://bugs.webkit.org/show_bug.cgi?id=56939

[Benjamin Poulain]

Comment on attachment 86508 [details]
Fixes for context attribute handling.

View in context: https://bugs.webkit.org/attachment.cgi?id=86508&action=review

I made general comments to get me started reviewing this :)

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:451
> +    if (!reshapeMainFbo(1, 1)) {

Why do we create the Fbo of size (1, 1) at this stage?

It seems reasonable to me to have an invalid painter state until GraphicsContext3D::reshape() is called with a valid size. Or some initialization of reshapeMainFbo() is needed in the constructor?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:454
>          LOG_ERROR("GraphicsContext3D: Wasn't able to create the main framebuffer");
>          m_contextValid = false;
>      }

Shouldn't this code be part of reshapeMainFbo()?
Logging the error and setting m_contextValid = false; seems reasonable to do in the function if the allocation failed.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:482
> +    m_pixels = QImage(width, height, QImage::Format_ARGB32);

I got rid of m_pixels yesterday :)

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:494
> +    bindRenderbuffer(GraphicsContext3D::RENDERBUFFER, m_depthBuffer);

if !(m_attrs.depth) || QT_OPENGL_ES_2, genRenderbuffers() is not called. So I guess we should not bind the buffer either(?).

[Jarkko Sakkinen]

(In reply to comment #9)
> (From update of attachment 86508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86508&action=review
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:451
> > +    if (!reshapeMainFbo(1, 1)) {
> 
> Why do we create the Fbo of size (1, 1) at this stage?
> 
> It seems reasonable to me to have an invalid painter state until GraphicsContext3D::reshape() is called with a valid size. Or some initialization of reshapeMainFbo() is needed in the constructor?

I will verify whether GraphicsContext3D::reshape() is called before
anything that depends on valid context. 

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:454
> >          LOG_ERROR("GraphicsContext3D: Wasn't able to create the main framebuffer");
> >          m_contextValid = false;
> >      }
> 
> Shouldn't this code be part of reshapeMainFbo()?
> Logging the error and setting m_contextValid = false; seems reasonable to do in the function if the allocation failed.

You're probably right. I will verify this one too.

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:482
> > +    m_pixels = QImage(width, height, QImage::Format_ARGB32);
> 
> I got rid of m_pixels yesterday :)

Yeah, have to revise this patch against those changes :)

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:494
> > +    bindRenderbuffer(GraphicsContext3D::RENDERBUFFER, m_depthBuffer);
> 
> if !(m_attrs.depth) || QT_OPENGL_ES_2, genRenderbuffers() is not called. So I guess we should not bind the buffer either(?).

Oops, right. And handle for the renderbuffer should not be created either. Fixing this.

[Jarkko Sakkinen]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 86508 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=86508&action=review
> > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:451
> > > +    if (!reshapeMainFbo(1, 1)) {
> > 
> > Why do we create the Fbo of size (1, 1) at this stage?
> > 
> > It seems reasonable to me to have an invalid painter state until GraphicsContext3D::reshape() is called with a valid size. Or some initialization of reshapeMainFbo() is needed in the constructor?
> 
> I will verify whether GraphicsContext3D::reshape() is called before
> anything that depends on valid context. 

Of course this doesn't matter because there is proper FBO handle created in constructor. Draw calls will fail with error code. No reshape needed there. 

> 
> > 
> > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:454
> > >          LOG_ERROR("GraphicsContext3D: Wasn't able to create the main framebuffer");
> > >          m_contextValid = false;
> > >      }
> > 
> > Shouldn't this code be part of reshapeMainFbo()?
> > Logging the error and setting m_contextValid = false; seems reasonable to do in the function if the allocation failed.
> 
> You're probably right. I will verify this one too.

m_contextValid flag means whether GL symbols are resolved correctly. For reshaping error log should be enough (does not affect outside of canvas, drawing stop to work).

> 
> > 
> > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:482
> > > +    m_pixels = QImage(width, height, QImage::Format_ARGB32);
> > 
> > I got rid of m_pixels yesterday :)
> 
> Yeah, have to revise this patch against those changes :)
> 
> > 
> > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:494
> > > +    bindRenderbuffer(GraphicsContext3D::RENDERBUFFER, m_depthBuffer);
> > 
> > if !(m_attrs.depth) || QT_OPENGL_ES_2, genRenderbuffers() is not called. So I guess we should not bind the buffer either(?).
> 
> Oops, right. And handle for the renderbuffer should not be created either. Fixing this.

[Jarkko Sakkinen]

Created attachment 86921 [details]
Revised version based on comments

[Jarkko Sakkinen]

Created attachment 86922 [details]
Ooops, last patch had invalid git diff

[Benjamin Poulain]

Comment on attachment 86922 [details]
Ooops, last patch had invalid git diff

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 :)

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

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

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

[Jarkko Sakkinen]

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

[Jarkko Sakkinen]

Created attachment 86932 [details]
Comments fixed to be in one line.

Values for m_attrs should be read from QGLFormat. Those code bits and some others that are now cleared are kind of left-overs when the this backend implementation was in its starting points and the WebGL implementation in WebKit as a whole was shaky. This patch cleans most of the history clutter.

[Jarkko Sakkinen]

Created attachment 86933 [details]
Style and cosmetic fixes.

Comment on common implementation (summary of the flood in IRC):

If you want common implementation there must a driver class for these features (platform dependent):
- Symbol resolution (GC3DOGL assumes that symbols are resolved)
- Some calls differ whether compiled for OGL ES 2 or desktop GL. ES 2.0 flagging is not done in the GC3DOGL.
- Context handling
- Painting of canvas

These already cover so huge chunk of GC3DQt that I think it would not be a good idea to go for common. Or at least there has to be good answers where platform dependent parts are placed. I think this is something that should be considered in long-term but not for the first release at least :)

[Jarkko Sakkinen]

Comment on attachment 86933 [details]
Style and cosmetic fixes.

Removed commit queue flag until review is done :)

[Kenneth Rohde Christiansen]

Comment on attachment 86933 [details]
Style and cosmetic fixes.

View in context: https://bugs.webkit.org/attachment.cgi?id=86933&action=review

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:161
> +    // Used by GraphicsContext3D to check whether construction was done succesfully.

Not sure that comment brings much value, anyway it is called "successfully" with 3 s's in total.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:320
> +    // No need viewport of QGLWidget. We use to get a separate QGLContext.

I dont understand that comment

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:475
> +    if (((width == m_boundingRect.width()) && (height == m_boundingRect.height())))

Are all those ()'s needed?

[Jarkko Sakkinen]

(In reply to comment #19)
> (From update of attachment 86933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86933&action=review
> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:161
> > +    // Used by GraphicsContext3D to check whether construction was done succesfully.
> 
> Not sure that comment brings much value, anyway it is called "successfully" with 3 s's in total.

True, this misguiding comment. I'll remove it.

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:320
> > +    // No need viewport of QGLWidget. We use to get a separate QGLContext.
> 
> I dont understand that comment

I don't understand it either :) I'll revise this one.

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:475
> > +    if (((width == m_boundingRect.width()) && (height == m_boundingRect.height())))
> 
> Are all those ()'s needed?

No. I'll change this.

[Jarkko Sakkinen]

Created attachment 87032 [details]
Cosmetic fixes.

[Benjamin Poulain]

Comment on attachment 87032 [details]
Cosmetic fixes.

Looks good, I can't think of anything to complain about this time :)

[WebKit Commit Bot]

Comment on attachment 87032 [details]
Cosmetic fixes.

Rejecting attachment 87032 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8271810

[Benjamin Poulain]

Comment on attachment 87032 [details]
Cosmetic fixes.

The bot had a problem, re-cq+

[WebKit Commit Bot]

Comment on attachment 87032 [details]
Cosmetic fixes.

Clearing flags on attachment: 87032

Committed r82117: <http://trac.webkit.org/changeset/82117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.