Bug 57154 - [Qt] Make OpenGL symbol resolver transparent
Summary: [Qt] Make OpenGL symbol resolver transparent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 57261 65465
  Show dependency treegraph
 
Reported: 2011-03-26 09:51 PDT by Jarkko Sakkinen
Modified: 2011-08-01 06:20 PDT (History)
5 users (show)

See Also:


Attachments
New symbol resolver (61.43 KB, patch)
2011-03-27 00:30 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Last patch had one header missing. (82.24 KB, patch)
2011-03-27 00:32 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Fixed ChangeLog entry. (82.37 KB, patch)
2011-03-27 00:35 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Fixed typo in ChangeLog and style issue (82.37 KB, patch)
2011-03-27 00:55 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Cleaned up symbol resolving code to have single point where used OpenGL calls can be defined (69.19 KB, patch)
2011-03-28 06:48 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Adapted to upstream changes (74.45 KB, patch)
2011-03-28 12:51 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
Clean up based on experiences with GraphicsContext3DOpenGL. Fixed ChangeLog entry. (78.51 KB, patch)
2011-03-29 12:41 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
some more cleanup (77.83 KB, patch)
2011-03-30 00:22 PDT, Jarkko Sakkinen
no flags Details | Formatted Diff | Diff
clean ups (75.28 KB, patch)
2011-03-30 03:46 PDT, Jarkko Sakkinen
hausmann: review-
Details | Formatted Diff | Diff
updated Jarkkos patch (78.66 KB, patch)
2011-06-24 14:26 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
updated Jarkkos patch (78.63 KB, patch)
2011-07-08 13:46 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
updated Jarkkos patch (78.37 KB, patch)
2011-07-11 12:31 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
use OpenGLShims resolver implementation (81.74 KB, patch)
2011-07-12 11:21 PDT, Andrew Wason
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
use OpenGLShims resolver implementation (82.47 KB, patch)
2011-07-12 14:31 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
use OpenGLShims resolver implementation (84.10 KB, patch)
2011-07-13 09:02 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
use OpenGLShims resolver implementation (70.41 KB, patch)
2011-07-18 14:53 PDT, Andrew Wason
noam: review-
Details | Formatted Diff | Diff
use OpenGLShims resolver implementation (70.38 KB, patch)
2011-07-19 14:16 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
update ChangeLog (70.47 KB, patch)
2011-07-19 14:35 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jarkko Sakkinen 2011-03-26 09:51:03 PDT
In order to move GraphicsContext3D Qt backend to use GraphicsContext3DOpenGL symbol resolver must mask the use of function pointers so that GraphicsContext3DOpenGL.cpp can be compiled.
Comment 1 Jarkko Sakkinen 2011-03-27 00:30:19 PDT
Created attachment 87059 [details]
New symbol resolver
Comment 2 Jarkko Sakkinen 2011-03-27 00:32:37 PDT
Created attachment 87060 [details]
Last patch had one header missing.
Comment 3 Jarkko Sakkinen 2011-03-27 00:35:10 PDT
Created attachment 87061 [details]
Fixed ChangeLog entry.
Comment 4 WebKit Review Bot 2011-03-27 00:36:34 PDT
Attachment 87061 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/qt/GraphicsContext3DInternalQt.h:23:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Jarkko Sakkinen 2011-03-27 00:55:36 PDT
Created attachment 87062 [details]
Fixed typo in ChangeLog and style issue
Comment 6 Jarkko Sakkinen 2011-03-27 10:22:37 PDT
I wrote experimental patch (which I'm going to submit "when it is ready enough" to upstream. I'll do some fine-tunings to this patch based on lessons learned writing that.
Comment 7 Jarkko Sakkinen 2011-03-27 23:23:07 PDT
Widened scope of this patch after successful experiment with GraphicsContext3DOpenGL. 

I would like to do this in two phases:
- Patch A: symbol resolver 
- Patch B: GraphicsContext3DOpenGL

NOTE! Migration will done for *desktop* only in this bugs scope. ES 2.0 will still use old implementation.
Comment 8 Jarkko Sakkinen 2011-03-28 06:48:20 PDT
Created attachment 87137 [details]
Cleaned up symbol resolving code to have single point where used OpenGL calls can be defined
Comment 9 Jarkko Sakkinen 2011-03-28 10:53:54 PDT
Comment on attachment 87137 [details]
Cleaned up symbol resolving code to have single point where used OpenGL calls can be defined

Rebasing to upstream changes..
Comment 10 Jarkko Sakkinen 2011-03-28 12:49:17 PDT
Created separate bug for GraphicsContext3DOpenGL.cpp:
https://bugs.webkit.org/show_bug.cgi?id=57261

Symbol resolver changes are self-contained and improve quality no matter what direction is taken.
Comment 11 Jarkko Sakkinen 2011-03-28 12:51:36 PDT
Created attachment 87182 [details]
Adapted to upstream changes
Comment 12 Jarkko Sakkinen 2011-03-29 08:57:53 PDT
I'll explain the basic idea in the symbol resolver so that is easier to follow the patch.

This is simple and stupid symbol resolver. OpenGL symbols are defined in GraphicsContext3DOpenGLQt.h. This is a data file that is used in class definition and constructor to define and initialize OpenGL symbols that need to be resolved.

Definition is done as follows:
#define GL_PROC(ReturnValue, Proc, Params) \
    typedef ReturnValue (APIENTRY* Proc##Type) Params; \
    Proc##Type Proc;
#include "GraphicsContext3DOpenGLQt.h"

Initialization is done as follows:
#if defined (QT_OPENGL_ES_2)
#define GL_PROC(ReturnValue, Proc, Params) Proc = ::Proc;
#else
#define GL_PROC(ReturnValue, Proc, Params) Proc = reinterpret_cast<GraphicsContext3D::Proc##Type>(m_internal->getProcAddress(#Proc));
#endif
Comment 13 Jarkko Sakkinen 2011-03-29 12:41:35 PDT
Created attachment 87396 [details]
Clean up based on experiences with GraphicsContext3DOpenGL. Fixed ChangeLog entry.
Comment 14 Jarkko Sakkinen 2011-03-30 00:22:29 PDT
Created attachment 87475 [details]
some more cleanup

I know the patch is large but if you look at it most of it is just removal of code.
Comment 15 Jarkko Sakkinen 2011-03-30 03:46:25 PDT
Created attachment 87496 [details]
clean ups
Comment 16 Simon Hausmann 2011-04-26 15:59:52 PDT
Comment on attachment 87496 [details]
clean ups

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

> Source/WebCore/platform/graphics/GraphicsContext3D.h:890
> +    int m_procsAreValid;

Shouldn't this be a bool instead of an int?
Comment 17 Simon Hausmann 2011-04-26 16:08:22 PDT
Comment on attachment 87496 [details]
clean ups

As a general direction I'd rather like to see us re-use cairo/OpenGLShisms.cpp/h for example
and use opengl/GraphicsContext3DOpenGL.cpp and leave Qt specific bits in GraphicsContext3DQt.cpp,
like the Gtk folks do.

On that note, why do we need to resolve the GL symbols at run-time?
Comment 18 Jarkko Sakkinen 2011-05-08 09:52:32 PDT
Simon, as a side-note I have also bug 57261 for that migration. We need to modify symbol resolver to be on intelligent before doing that.

On Desktop OpenGL we always will need a symbol resolver to get those symbols that are not readily available in standard OpenGL header files. Note that Qt does similar things itself in painting subsystem.

I looked at the OpenGLShims implementation and it is in fact a symbol resolver. Code is direct derivative * of code in GraphicsContext3DQt but it is more limited. Symbols resolved are taken from global variable. Symbols returned by getProcAddress are context dependent. Therefore stable implementation cannot be based on a global variable.

* Can be seen by reading the code and from having a Tieto copyright.
Comment 19 Andrew Wason 2011-06-24 14:26:32 PDT
Created attachment 98541 [details]
updated Jarkkos patch

Updated Jarkkos patch to take into account changes in trunk, including new TextureMapper stuff.
Comment 20 Andrew Wason 2011-07-08 13:46:38 PDT
Created attachment 100156 [details]
updated Jarkkos patch

Update patch to make m_procsAreValid a bool in response to earlier comment.
Comment 21 Andrew Wason 2011-07-11 12:31:30 PDT
Created attachment 100349 [details]
updated Jarkkos patch

Some additional cleanup realized while working on bug 57261

Move the destruction of members owned by GraphicsContext3D from ~GraphicsContext3DInternal to ~GraphicsContext3D.

Define proc members for QT_OPENGL_ES_2 too - so we can always access them via the context - this removes extra conditional code from other classes that need to access the procs via m_context.
Comment 22 Andrew Wason 2011-07-12 08:06:48 PDT
(In reply to comment #18)
> I looked at the OpenGLShims implementation and it is in fact a symbol resolver.
> Code is direct derivative * of code in GraphicsContext3DQt but it is more limited.
> Symbols resolved are taken from global variable. Symbols returned by getProcAddress are
> context dependent. Therefore stable implementation cannot be based on a global variable.


It looks to me like GL function pointers are context *independent* - so we should be able to use cairo/OpenGLShims.cpp/h

"Final ARB-approved version. - Specify that GL function pointers are context independent."
http://www.opengl.org/registry/specs/ARB/get_proc_address.txt

This is good, because it looks like we will also need to use the resolver in opengl/TextureMapperGL.cpp and it doesn't have access to GraphicsContext3D where we are currently storing the proc addresses.
Comment 23 Andrew Wason 2011-07-12 11:21:40 PDT
Created attachment 100527 [details]
use OpenGLShims resolver implementation

Use the OpenGLShims resolver implementation. Also make TextureMapperGL use the resolver.
Comment 24 Early Warning System Bot 2011-07-12 11:32:43 PDT
Comment on attachment 100527 [details]
use OpenGLShims resolver implementation

Attachment 100527 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9022217
Comment 25 Andrew Wason 2011-07-12 14:31:43 PDT
Created attachment 100562 [details]
use OpenGLShims resolver implementation

Fix patch so it builds when webgl is not enabled - since TextureMapperGL needs the shims too.
Comment 26 Andrew Wason 2011-07-12 15:12:37 PDT
Comment on attachment 100562 [details]
use OpenGLShims resolver implementation

#defining non-EXT versions of the *EXT procs is not going to work, will rework this patch
Comment 27 Jarkko Sakkinen 2011-07-13 06:00:14 PDT
Latest patch looks good to me at least. Nice to see that this code is being updated as I don't have free time these days to contribute to WebKit. Thank you :)
Comment 28 Andrew Wason 2011-07-13 09:02:15 PDT
Created attachment 100678 [details]
use OpenGLShims resolver implementation

Update OpenGLShims.h macros so the *EXT macros expand correctly.

Sorry for all the false starts, but I think this patch is really ready for review now :)
Comment 29 Noam Rosenthal 2011-07-16 20:06:45 PDT
(In reply to comment #28)
> Created an attachment (id=100678) [details]
> use OpenGLShims resolver implementation
> 
> Update OpenGLShims.h macros so the *EXT macros expand correctly.
> 
> Sorry for all the false starts, but I think this patch is really ready for review now :)

It's a pretty big patch; Hard to review...
Could you break it down a bit, e.g. let us review the changes to OpenGLShims first, then the changes to GraphicsContext3D, and then TextureMapper?
Comment 30 Andrew Wason 2011-07-18 14:53:37 PDT
Created attachment 101209 [details]
use OpenGLShims resolver implementation

(In reply to comment #29)
> It's a pretty big patch; Hard to review...
> Could you break it down a bit

The patch had some additional changes not directly related to adopting the shims resolver - it was also removing things from GraphicsContext3DInternal and using the corresponding members in GraphicsContext3D instead - holdovers from jarkkos original patch.

I'm going to move those changes to separate patches for bug 57261 where they belong, and this patch now only addresses changing the symbol resolver.

So this patch is still kind of big, but very straightforward. It just deletes a bunch of typedefs, proc member declarations and assignments that were the old resolver. Then it changes each call to the old symbol resolver to use the new one (e.g. m_internal->bindFramebuffer becomes glBindFramebuffer etc.) - so just a bunch of single line diffs where it's very clear what is changing.

I think this will be much easier to review.
Comment 31 Noam Rosenthal 2011-07-18 18:33:13 PDT
> So this patch is still kind of big, but very straightforward. It just deletes a bunch of typedefs, proc member declarations and assignments that were the old resolver. Then it changes each call to the old symbol resolver to use the new one (e.g. m_internal->bindFramebuffer becomes glBindFramebuffer etc.) - so just a bunch of single line diffs where it's very clear what is changing.
> 
> I think this will be much easier to review.

I'm having a hard time when the patch contains stuff in the Cairo directory, even though it looks rather trivial.
The Qt stuff looks OK, if you could get Martin Robinson or someone familiar with Cairo OpenGLShims to review changes to those files, perhaps in an additional bug, I'd be happy to review the Qt specific stuff.
Comment 32 Andrew Wason 2011-07-19 09:52:08 PDT
(In reply to comment #31)
> I'm having a hard time when the patch contains stuff in the Cairo directory,
> even though it looks rather trivial.
> The Qt stuff looks OK, if you could get Martin Robinson or someone familiar with
> Cairo OpenGLShims to review changes to those files, perhaps in an additional bug,
> I'd be happy to review the Qt specific stuff.


Martin, would you be able to review the OpenGLShims.h/cpp part of the patch? The changes are pretty straightforward.
Comment 33 Martin Robinson 2011-07-19 09:57:51 PDT
This seems reasonable to me. Do you plan to now use the shared version of GraphicsContext3D in the opengl directory?
Comment 34 Andrew Wason 2011-07-19 10:57:28 PDT
(In reply to comment #33)
> This seems reasonable to me.

Thanks for looking at this.

> Do you plan to now use the shared version of GraphicsContext3D in the opengl directory?

Yes, that will be done in bug 57261
Comment 35 Martin Robinson 2011-07-19 10:59:43 PDT
(In reply to comment #34)

> Yes, that will be done in bug 57261

That's great news!
Comment 36 Noam Rosenthal 2011-07-19 12:54:21 PDT
Comment on attachment 101209 [details]
use OpenGLShims resolver implementation

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

> Source/WebCore/WebCore.pro:3619
> +    CONFIG += opengl-shims

Indentation seems a bit off.

> Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:20
> +#if ENABLE(WEBGL) || (PLATFORM(QT) && USE(TEXTURE_MAPPER_GL))

This is a bit awkward. Maybe add a define QT_OPENGL_SHIMS inside the contains(CONFIG...) clause? Otherwise we repeat the build-time logic that decides whether to use it twice.

> Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:34
> +#define ASSIGN_FUNCTION_TABLE_ENTRY(FunctionName, success) \

Strange to use uppercase for FunctionName and lowercase for success.

> Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:46
> +    return QGLContext::currentContext()->getProcAddress(QLatin1String(procName));

This doesn't buy you anything. getProcAddress(procName) is essentially the same thing and is somewhat more readable.
Comment 37 Noam Rosenthal 2011-07-19 12:55:14 PDT
Almost there, sorry for nitpicking but it's a lot of code :)
Thanks for working on this.
Comment 38 Andrew Wason 2011-07-19 13:27:55 PDT
(In reply to comment #36)
> > Source/WebCore/WebCore.pro:3619
> > +    CONFIG += opengl-shims
> 
> Indentation seems a bit off.

Actually, the existing indentation of the INCLUDEPATH directly above my CONFIG
was wrong, so I fixed that when I added the new CONFIG. Both these should line
up with the SOURCES further above (that you can't see in the diff),
not with the filenames themselves that are the indented value of SOURCES.

> > Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:20
> > +#if ENABLE(WEBGL) || (PLATFORM(QT) && USE(TEXTURE_MAPPER_GL))
> 
> This is a bit awkward. Maybe add a define QT_OPENGL_SHIMS inside the contains(CONFIG...) clause?
> Otherwise we repeat the build-time logic that decides whether to use it twice.

I'll add QT_OPENGL_SHIMS.

> > Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:34
> > +#define ASSIGN_FUNCTION_TABLE_ENTRY(FunctionName, success) \
> 
> Strange to use uppercase for FunctionName and lowercase for success.

I agree, but the existing definition of ASSIGN_FUNCTION_TABLE_ENTRY already used those names
(see a few lines below in the diff).
Should I fix the existing definition and my new definition, or leave my new definition
consistent with the existing one?

> > Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:46
> > +    return QGLContext::currentContext()->getProcAddress(QLatin1String(procName));
> 
> This doesn't buy you anything. getProcAddress(procName) is essentially the
> same thing and is somewhat more readable.

I'll remove QLatin1String.
Comment 39 Noam Rosenthal 2011-07-19 13:32:58 PDT
> Actually, the existing indentation of the INCLUDEPATH directly above my CONFIG
> was wrong, so I fixed that when I added the new CONFIG. Both these should line
> up with the SOURCES further above (that you can't see in the diff),
> not with the filenames themselves that are the indented value of SOURCES.
>
OK. 

> I agree, but the existing definition of ASSIGN_FUNCTION_TABLE_ENTRY already used those names
> (see a few lines below in the diff).
> Should I fix the existing definition and my new definition, or leave my new definition
> consistent with the existing one?
Leave it consistent, that's ok.

> I'll remove QLatin1String.
I think with this and the #define we're good.
Comment 40 Andrew Wason 2011-07-19 13:41:41 PDT
(In reply to comment #39)
> > I'll remove QLatin1String.
> I think with this and the #define we're good.

Hmm, actually I get an error when I remove it:

../../../Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:42: error: 'QString::QString(const char*)' is deprecated

procName is const char*, not WTF::String, so I think I need to keep QLatin1String?
Comment 41 Noam Rosenthal 2011-07-19 13:47:54 PDT
> ../../../Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:42: error: 'QString::QString(const char*)' is deprecated
> 
> procName is const char*, not WTF::String, so I think I need to keep QLatin1String?

you can use QString::fromAscii or QString::fromLatin1.
Comment 42 Andrew Wason 2011-07-19 14:16:23 PDT
Created attachment 101383 [details]
use OpenGLShims resolver implementation

Update patch based on review comments.
Comment 43 Andrew Wason 2011-07-19 14:35:54 PDT
Created attachment 101389 [details]
update ChangeLog
Comment 44 Noam Rosenthal 2011-07-19 14:41:22 PDT
Comment on attachment 101389 [details]
update ChangeLog

The responses to my comments were satisfactory and Martin Robinson reviewed the Cairo stuff. I think this looks good!
Comment 45 WebKit Review Bot 2011-07-20 07:52:37 PDT
Comment on attachment 101389 [details]
update ChangeLog

Clearing flags on attachment: 101389

Committed r91363: <http://trac.webkit.org/changeset/91363>
Comment 46 WebKit Review Bot 2011-07-20 07:52:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Benjamin Poulain 2011-08-01 04:12:05 PDT
Comment on attachment 101389 [details]
update ChangeLog

Since this patch, it seems it is no longuer possible to compile trunk with OpenGL on ARM.

> Source/WebCore/WebCore.pro:3676
> +    HEADERS += platform/graphics/cairo/OpenGLShims.h
> +    SOURCES += platform/graphics/cairo/OpenGLShims.cpp

And may I suggest to move those files out of cairo and in platform/opengl? :-D
This is uber confusing to have cairo files being compiled with Qt, I was convinced that was a mistake until I saw this commit.
Comment 48 Benjamin Poulain 2011-08-01 04:19:35 PDT
(In reply to comment #41)
> > ../../../Source/WebCore/platform/graphics/cairo/OpenGLShims.cpp:42: error: 'QString::QString(const char*)' is deprecated
> > 
> > procName is const char*, not WTF::String, so I think I need to keep QLatin1String?
> 
> you can use QString::fromAscii or QString::fromLatin1.

(It is almost never correct to use QString::fromAscii() from inside a library since it relies on the default encoding, not on a ASCII codec.)