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.
Created attachment 87059 [details] New symbol resolver
Created attachment 87060 [details] Last patch had one header missing.
Created attachment 87061 [details] Fixed ChangeLog entry.
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.
Created attachment 87062 [details] Fixed typo in ChangeLog and style issue
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.
Created attachment 87137 [details] Cleaned up symbol resolving code to have single point where used OpenGL calls can be defined
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.
Created attachment 87182 [details] Adapted to upstream changes
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
Created attachment 87396 [details] Clean up based on experiences with GraphicsContext3DOpenGL. Fixed ChangeLog entry.
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.
Created attachment 87496 [details] clean ups
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 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 100156 [details] updated Jarkkos patch Update patch to make m_procsAreValid a bool in response to earlier comment.
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.
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
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.
This seems reasonable to me. Do you plan to now use the shared version of GraphicsContext3D in the opengl directory?
(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
(In reply to comment #34) > Yes, that will be done in bug 57261 That's great news!
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.
Almost there, sorry for nitpicking but it's a lot of code :) Thanks for working on this.
(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.
Created attachment 101383 [details] use OpenGLShims resolver implementation Update patch based on review comments.
Created attachment 101389 [details] update ChangeLog
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 Clearing flags on attachment: 101389 Committed r91363: <http://trac.webkit.org/changeset/91363>
All reviewed patches have been landed. Closing bug.
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.)