Bug 78720 – [Qt] Use GraphicsContext3DOpenGLES.cpp when using OpenGL ES

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Simon Hausmann]

Currently the WebGL enabled build is broken against platforms that have OpenGL ES instead of desktop GL.

On these platforms we should try to use the new GraphicsContext3DOpenGLES.cpp that was introduced with bug #75462.

* WebCore/Target.pri needs to be changed to compile GraphicsContext3DQt.cpp, *OpenGLCommon and *OpenGLES.cpp when contains(QT_CONFIG, opengles2).

* The functions declared in GraphicsContext3DQt.cpp that are surrounded by #if defined(QT_OPENGL_ES_2) should be removed (their replacement should be in *OpenGLCommon.cpp and *OpenGLES.cpp)

[Roland Takacs]

Created attachment 133473 [details]
Patch

From now, GraphicsContext3DOpenGLES.cpp will be used on OpenGL ES supported platform.
Functions are removed from GraphicsContext3DQt that already implemented in GraphicsContext3DCommon.

"EXT" has been deleted from function and macro names ending with "EXT".
It was needed because OpenGLES does not support these (and functions and macros without "EXT" do the same things as the
ones ending with "EXT").

Extensions3DQt is removed beceause it has never been used.

The "m_depthStencilBuffer" member is added to Qt port because it uses "m_depthStencilBuffer" like the other platforms.

Furthermore some syntax errors have been fixed.

Unfortunately I could not test perfectly because the webgl does not work.
I get Segmentation Fault when I try to open a webgl example within QtTestbrowser. (On desktop)
I don't get segfault on my N9 but my device draws nothing, and says that this browser does not support webgl.

[Noam Rosenthal]

Comment on attachment 133473 [details]
Patch

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

Good work, see comments.

> Source/WebCore/ChangeLog:9
> +        No new tests, just a buildfix.
> +

I wouldn't call this a build fix. I'd say instead that it's tested by existing WebGL tests.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:947
> +#if defined(QT_OPENGL_ES_2) || USE(OPENGL_ES_2)

I e should add
#if defined(QT_OPENGL_ES_2)
#define WTF_USE_OPENGL_ES_2 1
#endif

to Platform.h instead.
Then we don't need to use ||.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:173
> +    ::glBindFramebuffer(GL_FRAMEBUFFER, m_fbo);

Wouldn't this break some existing desktop stuff, where EXT is needed?
I'd rather this be moved to a bindFramebuffer that's implemented differently in ES and desktop.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:212
> +#if defined(QT_OPENGL_ES_2) || USE(OPENGL_ES_2)

I'd rather we move all the ES specific functionality into the ES/desktop files, rather than have #ifdefs inside common. This applies to a lot of the following changes :)

[Martin Robinson]

Comment on attachment 133473 [details]
Patch

I've been hoping to see this change for a long time!

I agree with No'am comments.

[Roland Takacs]

Created attachment 134278 [details]
patch

The changes are implemented based on Noam advices.

[Noam Rosenthal]

was this tested (In reply to comment #4)
> Created an attachment (id=134278) [details]
> patch
> 
> The changes are implemented based on Noam advices.

Was this tested on actual ES hardware (e.g. N9)?

[Roland Takacs]

(In reply to comment #5)
> was this tested (In reply to comment #4)
> > Created an attachment (id=134278) [details] [details]
> > patch
> > 
> > The changes are implemented based on Noam advices.
> 
> Was this tested on actual ES hardware (e.g. N9)?

Of course, I sent the compiled webkit to my N9, but unfortunatelly it did not work. I wrote at my first comment why I could not to test.

The working is similar to Deskto on my N9.

My next task will fix this segfault error.

[Noam Rosenthal]

(In reply to comment #6)
> (In reply to comment #5)
> > was this tested (In reply to comment #4)
> > > Created an attachment (id=134278) [details] [details] [details]
> > > patch
> > > 
> > > The changes are implemented based on Noam advices.
> > 
> > Was this tested on actual ES hardware (e.g. N9)?
> 
> Of course, I sent the compiled webkit to my N9, but unfortunatelly it did not work. I wrote at my first comment why I could not to test.
> 
> The working is similar to Deskto on my N9.
> 
> My next task will fix this segfault error.

OK, let's wait with this until it works then.

[Noam Rosenthal]

> > My next task will fix this segfault error.
> 
> OK, let's wait with this until it works then.

btw - you're aware that this is only testable with WebKit-1 (QtTestBrowser) right now.

[Noam Rosenthal]

Comment on attachment 134278 [details]
patch

Clearing flags for now.

[Noam Rosenthal]

Any progress on this?

[Roland Takacs]

(In reply to comment #10)
> Any progress on this?

Sorry, I had a day-off then.

The patch is ready, but it does not work on N9 (since --3d-canvas have been replaces with --webgl).

When I run QtTestBrowser with these switches:

-graphicsbased -gl-viewport -tiled-backing-store -viewport-update-mode BoundingRect -webgl http://jsbin.com/ulazel

I get an error saying: "Your web browser does not support WebGL!"

Is it possible that a switch is missing (it used to work when --3d-canvas was active)?

[Noam Rosenthal]

(In reply to comment #11)
> (In reply to comment #10)
> > Any progress on this?
> 
> Sorry, I had a day-off then.
> 
> The patch is ready, but it does not work on N9 (since --3d-canvas have been replaces with --webgl).
> 
> When I run QtTestBrowser with these switches:
> 
> -graphicsbased -gl-viewport -tiled-backing-store -viewport-update-mode BoundingRect -webgl http://jsbin.com/ulazel
> 
> I get an error saying: "Your web browser does not support WebGL!"
> 
> Is it possible that a switch is missing (it used to work when --3d-canvas was active)?

I think the --3d-canvas switch for build-webkit has been replaced with --webgl.
Are you sure the code is getting built?

[Roland Takacs]

Created attachment 145574 [details]
patch

WebGL works on N9 with this patch

[Noam Rosenthal]

Comment on attachment 145574 [details]
patch

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

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:252
> +        GraphicsContext3D::clearDepth(1);

I don't think you need GraphicsContext3D::

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:274
> +        GraphicsContext3D::clearDepth(clearDepth);

ditto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:93
> -            ::glTexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_STENCIL_OES, width, height, 0, GL_DEPTH_STENCIL_OES, GL_UNSIGNED_INT_24_8_OES, 0);
> +            ::glTexImage2D(GL_TEXTURE_2D, 0, m_internalColorFormat, width, height, 0, colorFormat, pixelDataType, 0);

This seems wrong. m_internalColorFormat is equivalent to GL_DEPTH_STENCIL_OES?
I suggest we simply not allow this code-path if GL_DEPTH_STENCIL_OES is not defined, or something to that effect.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:236
> +#if !defined (QT_OPENGL_ES_2)

Let's switch to USE(OPENGL_ES_2) in this file while we're at it.

[Roland Takacs]

(In reply to comment #14)
> (From update of attachment 145574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145574&action=review
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:252
> > +        GraphicsContext3D::clearDepth(1);
> 
> I don't think you need GraphicsContext3D::
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:274
> > +        GraphicsContext3D::clearDepth(clearDepth);
> 
> ditto.

It is necessary to write 'GraphicsContext3D::' because there is a variable that name is 'clearDepth'. With 'GraphicsContext3D::' you can say to the compiler that it is a function and not the variable. 
 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:93
> > -            ::glTexImage2D(GL_TEXTURE_2D, 0, GL_DEPTH_STENCIL_OES, width, height, 0, GL_DEPTH_STENCIL_OES, GL_UNSIGNED_INT_24_8_OES, 0);
> > +            ::glTexImage2D(GL_TEXTURE_2D, 0, m_internalColorFormat, width, height, 0, colorFormat, pixelDataType, 0);
> 
> This seems wrong. m_internalColorFormat is equivalent to GL_DEPTH_STENCIL_OES?
> I suggest we simply not allow this code-path if GL_DEPTH_STENCIL_OES is not defined, or something to that effect.

You are right. I found the appropriate of GL_DEPTH_STENCIL_OES in GC3D.h that is GraphicsContext3D::DEPTH_STENCIL.
But the GL_UNSIGNED_INT_24_8_OES has not an appropriate type in GC3D.h that's why I inserted a new type into GC3D's macros. It is UNSIGNED_INT_24_8.

I hope it is a right way.

> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:236
> > +#if !defined (QT_OPENGL_ES_2)
> 
> Let's switch to USE(OPENGL_ES_2) in this file while we're at it.

fixed

[Roland Takacs]

Created attachment 145737 [details]
patch

[Noam Rosenthal]

Comment on attachment 145737 [details]
patch

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

Great! One nitpick.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:240
> +#if !USE(OPENGL_ES_2)
>      glBindFramebuffer(GL_READ_FRAMEBUFFER_EXT, m_context->m_multisampleFBO);
>      glBindFramebuffer(GL_DRAW_FRAMEBUFFER_EXT, m_context->m_fbo);
> +#endif
>      glBlitFramebuffer(0, 0, m_context->m_currentWidth, m_context->m_currentHeight, 0, 0, m_context->m_currentWidth, m_context->m_currentHeight, GL_COLOR_BUFFER_BIT, GL_LINEAR);

The #endif should encompass the blit as well, we can't blit if we don't bind READ/DRAW fbos.

[Roland Takacs]

Created attachment 145796 [details]
patch

[WebKit Review Bot]

Comment on attachment 145796 [details]
patch

Clearing flags on attachment: 145796

Committed r119552: <http://trac.webkit.org/changeset/119552>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.