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.
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.
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.
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 on attachment 87137[details]
Cleaned up symbol resolving code to have single point where used OpenGL calls can be defined
Rebasing to upstream changes..
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.
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 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?
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.
Created attachment 98541[details]
updated Jarkkos patch
Updated Jarkkos patch to take into account changes in trunk, including new TextureMapper stuff.
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.
(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.
Created attachment 100527[details]
use OpenGLShims resolver implementation
Use the OpenGLShims resolver implementation. Also make TextureMapperGL use the resolver.
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 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
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 :)
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 :)
(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?
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.
> 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.
(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.
(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 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.
(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.
> 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.
(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?
> ../../../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 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 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.
(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.)
2011-03-27 00:30 PDT, Jarkko Sakkinen
2011-03-27 00:32 PDT, Jarkko Sakkinen
2011-03-27 00:35 PDT, Jarkko Sakkinen
2011-03-27 00:55 PDT, Jarkko Sakkinen
2011-03-28 06:48 PDT, Jarkko Sakkinen
2011-03-28 12:51 PDT, Jarkko Sakkinen
2011-03-29 12:41 PDT, Jarkko Sakkinen
2011-03-30 00:22 PDT, Jarkko Sakkinen
2011-03-30 03:46 PDT, Jarkko Sakkinen
2011-06-24 14:26 PDT, Andrew Wason
2011-07-08 13:46 PDT, Andrew Wason
2011-07-11 12:31 PDT, Andrew Wason
2011-07-12 11:21 PDT, Andrew Wason
2011-07-12 14:31 PDT, Andrew Wason
2011-07-13 09:02 PDT, Andrew Wason
2011-07-18 14:53 PDT, Andrew Wason
2011-07-19 14:16 PDT, Andrew Wason
2011-07-19 14:35 PDT, Andrew Wason