Bug 78720 - [Qt] Use GraphicsContext3DOpenGLES.cpp when using OpenGL ES
Summary: [Qt] Use GraphicsContext3DOpenGLES.cpp when using OpenGL ES
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Roland Takacs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-15 09:25 PST by Simon Hausmann
Modified: 2012-06-05 20:04 PDT (History)
5 users (show)

See Also:


Attachments
Patch (56.98 KB, patch)
2012-03-23 06:39 PDT, Roland Takacs
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
patch (67.45 KB, patch)
2012-03-28 06:12 PDT, Roland Takacs
no flags Details | Formatted Diff | Diff
patch (52.43 KB, patch)
2012-06-04 06:17 PDT, Roland Takacs
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
patch (53.87 KB, patch)
2012-06-05 02:29 PDT, Roland Takacs
noam: review+
noam: commit-queue-
Details | Formatted Diff | Diff
patch (53.88 KB, patch)
2012-06-05 07:56 PDT, Roland Takacs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2012-02-15 09:25:27 PST
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)
Comment 1 Roland Takacs 2012-03-23 06:39:19 PDT
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.
Comment 2 Noam Rosenthal 2012-03-23 07:07:52 PDT
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 :)
Comment 3 Martin Robinson 2012-03-23 08:58:56 PDT
Comment on attachment 133473 [details]
Patch

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

I agree with No'am comments.
Comment 4 Roland Takacs 2012-03-28 06:12:37 PDT
Created attachment 134278 [details]
patch

The changes are implemented based on Noam advices.
Comment 5 Noam Rosenthal 2012-03-28 06:33:28 PDT
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)?
Comment 6 Roland Takacs 2012-03-28 07:17:01 PDT
(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.
Comment 7 Noam Rosenthal 2012-03-28 08:35:28 PDT
(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.
Comment 8 Noam Rosenthal 2012-04-05 07:50:33 PDT
> > 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.
Comment 9 Noam Rosenthal 2012-04-19 16:47:58 PDT
Comment on attachment 134278 [details]
patch

Clearing flags for now.
Comment 10 Noam Rosenthal 2012-05-09 06:10:59 PDT
Any progress on this?
Comment 11 Roland Takacs 2012-05-24 06:06:21 PDT
(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)?
Comment 12 Noam Rosenthal 2012-05-24 06:32:28 PDT
(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?
Comment 13 Roland Takacs 2012-06-04 06:17:51 PDT
Created attachment 145574 [details]
patch

WebGL works on N9 with this patch
Comment 14 Noam Rosenthal 2012-06-04 06:57:37 PDT
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.
Comment 15 Roland Takacs 2012-06-05 02:29:13 PDT
(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
Comment 16 Roland Takacs 2012-06-05 02:29:56 PDT
Created attachment 145737 [details]
patch
Comment 17 Noam Rosenthal 2012-06-05 06:40:09 PDT
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.
Comment 18 Roland Takacs 2012-06-05 07:56:46 PDT
Created attachment 145796 [details]
patch
Comment 19 WebKit Review Bot 2012-06-05 20:04:47 PDT
Comment on attachment 145796 [details]
patch

Clearing flags on attachment: 145796

Committed r119552: <http://trac.webkit.org/changeset/119552>
Comment 20 WebKit Review Bot 2012-06-05 20:04:55 PDT
All reviewed patches have been landed.  Closing bug.