This is part 2 of a 3 part migration for bug 57261 to migrate to GraphicsContext3DOpenGL.cpp for Qt. Part 2 adopts GraphicsContext3DOpenGL.cpp and ANGLE, but does not yet implement antialiasing.
Created attachment 101609 [details] adopt GraphicsContext3DOpenGL.cpp and ANGLE
Comment on attachment 101609 [details] adopt GraphicsContext3DOpenGL.cpp and ANGLE View in context: https://bugs.webkit.org/attachment.cgi?id=101609&action=review Comments inline. > Source/WebCore/WebCore.pro:3641 > + *g++* { > + QMAKE_CXXFLAGS += -Wno-unused-variable > + QMAKE_CXXFLAGS += -Wno-missing-noreturn > + QMAKE_CXXFLAGS += -Wno-unused-function > + QMAKE_CXXFLAGS += -Wno-parentheses > + QMAKE_CXXFLAGS += -Wno-reorder > + # g++ < 4.3 doesn't support -Wno-type-limits > + system( $$QMAKE_CXX --version | grep -e "\\<[4-9]\\.[3-9]\\.[0-9]" ) { > + QMAKE_CXXFLAGS += -Wno-type-limits > + } Please explain :) > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:32 > +#if ENABLE(WEBGL) && !defined(QT_OPENGL_ES_2) So, does this mean we use GraphicsContext3DQt for ES, and GraphicsContext3DOpenGL for desktop? Maybe we should make this decision in the pro file rather than pollute the #ifdef space, especially in files shared by other ports. > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:336 > + // Always set to 1 for OpenGL ES. Shouldn't we allow more if we're not in ES?
(In reply to comment #2) > > > Source/WebCore/WebCore.pro:3641 > > + *g++* { > > + QMAKE_CXXFLAGS += -Wno-unused-variable > > + QMAKE_CXXFLAGS += -Wno-missing-noreturn > > + QMAKE_CXXFLAGS += -Wno-unused-function > > + QMAKE_CXXFLAGS += -Wno-parentheses > > + QMAKE_CXXFLAGS += -Wno-reorder > > + # g++ < 4.3 doesn't support -Wno-type-limits > > + system( $$QMAKE_CXX --version | grep -e "\\<[4-9]\\.[3-9]\\.[0-9]" ) { > > + QMAKE_CXXFLAGS += -Wno-type-limits > > + } > > Please explain :) These are the g++ warnings that need to be disabled for ANGLE to build without generating warnings. The nasty bit at the end is because g++ 4.3 generates an additional warning, and the flag to disable that is 4.3 specific. Let me check if all these are still needed since ANGLE has been updated since jarkko worked on this (he came up with most of that list) > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:32 > > +#if ENABLE(WEBGL) && !defined(QT_OPENGL_ES_2) > > So, does this mean we use GraphicsContext3DQt for ES, and GraphicsContext3DOpenGL for desktop? > Maybe we should make this decision in the pro file rather than pollute the #ifdef space, especially in files shared by other ports. Yes, all these changes are just for desktop. But I can't figure out how to determine if we are ES2 or desktop in the pro file - QT_OPENGL_ES_2 I believe would be defined in <Qt/qconfig.h> and that file is generated as part of the Qt configure/build process. If I could determine we are ES2 in the pro, then I can just not include the file in SOURCES in that case. > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:336 > > + // Always set to 1 for OpenGL ES. > > Shouldn't we allow more if we're not in ES? Hmm, this whole ANGLE init block (including that comment) was copied from GraphicsContext3DGtk.cpp and GraphicsContext3DMac.mm (which initialize it identically). I think the "ES" in the comment refers to the WebGL spec being based on ES 2.0 which only supports 1 drawing buffer - i.e. it's a requirement of the WebGL spec. But actually that whole ANGLE initialization block in GraphicsContext3DQt.cpp needs to be wrapped in #if !defined(QT_OPENGL_ES_2) because we're only using ANGLE for desktop.
Created attachment 101652 [details] simplified ANGLE build Removed some flags/defines from WebCore.pro that aren't needed by the latest ANGLE. Only initialize ANGLE if not ES2.
> > > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:32 > > > +#if ENABLE(WEBGL) && !defined(QT_OPENGL_ES_2) > > > > So, does this mean we use GraphicsContext3DQt for ES, and GraphicsContext3DOpenGL for desktop? > > Maybe we should make this decision in the pro file rather than pollute the #ifdef space, especially in files shared by other ports. > > Yes, all these changes are just for desktop. But I can't figure out how to determine if we are ES2 or desktop in the pro file - QT_OPENGL_ES_2 I believe would be defined in <Qt/qconfig.h> and that file is generated as part of the Qt configure/build process. If I could determine we are ES2 in the pro, then I can just not include the file in SOURCES in that case. contains(QT_CONFIG, opengles2)
Comment on attachment 101652 [details] simplified ANGLE build View in context: https://bugs.webkit.org/attachment.cgi?id=101652&action=review > Source/WebCore/WebCore.pro:3637 > + *g++* { > + QMAKE_CXXFLAGS += -Wno-unused-variable > + QMAKE_CXXFLAGS += -Wno-missing-noreturn > + QMAKE_CXXFLAGS += -Wno-unused-function > + QMAKE_CXXFLAGS += -Wno-reorder > + } Sorry, I can't let an overarching disabling of warnings in... We have to figure out a way to diable these for ANGLE files only, or to live with those warnings being outputted when we build the ANGLE files. > Source/WebCore/WebCore.pro:3736 > + HEADERS += \ > + $$ANGLE_DIR/src/compiler/BaseTypes.h \ > + $$ANGLE_DIR/src/compiler/Common.h \ > + $$ANGLE_DIR/src/compiler/ConstantUnion.h \ > + $$ANGLE_DIR/src/compiler/debug.h \ > + $$ANGLE_DIR/src/compiler/ExtensionBehavior.h \ > + $$ANGLE_DIR/src/compiler/ForLoopUnroll.h \ > + $$ANGLE_DIR/src/compiler/glslang.h \ > + $$ANGLE_DIR/src/compiler/glslang_tab.h \ > + $$ANGLE_DIR/src/compiler/InfoSink.h \ > + $$ANGLE_DIR/src/compiler/InitializeDll.h \ > + $$ANGLE_DIR/src/compiler/InitializeGlobals.h \ > + $$ANGLE_DIR/src/compiler/Initialize.h \ > + $$ANGLE_DIR/src/compiler/InitializeParseContext.h \ > + $$ANGLE_DIR/src/compiler/intermediate.h \ > + $$ANGLE_DIR/src/compiler/localintermediate.h \ > + $$ANGLE_DIR/src/compiler/MMap.h \ > + $$ANGLE_DIR/src/compiler/MapLongVariableNames.h \ > + $$ANGLE_DIR/src/compiler/osinclude.h \ > + $$ANGLE_DIR/src/compiler/OutputESSL.h \ > + $$ANGLE_DIR/src/compiler/OutputGLSL.h \ > + $$ANGLE_DIR/src/compiler/OutputGLSLBase.h \ > + $$ANGLE_DIR/src/compiler/OutputHLSL.h \ > + $$ANGLE_DIR/src/compiler/ParseHelper.h \ > + $$ANGLE_DIR/src/compiler/PoolAlloc.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/atom.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/compile.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/cpp.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/memory.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/parser.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/preprocess.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/scanner.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/slglobals.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/symbols.h \ > + $$ANGLE_DIR/src/compiler/preprocessor/tokens.h \ > + $$ANGLE_DIR/src/compiler/QualifierAlive.h \ > + $$ANGLE_DIR/src/compiler/RemoveTree.h \ > + $$ANGLE_DIR/src/compiler/SearchSymbol.h \ > + $$ANGLE_DIR/src/compiler/ShHandle.h \ > + $$ANGLE_DIR/src/compiler/SymbolTable.h \ > + $$ANGLE_DIR/src/compiler/TranslatorESSL.h \ > + $$ANGLE_DIR/src/compiler/TranslatorGLSL.h \ > + $$ANGLE_DIR/src/compiler/TranslatorHLSL.h \ > + $$ANGLE_DIR/src/compiler/Types.h \ > + $$ANGLE_DIR/src/compiler/UnfoldSelect.h \ > + $$ANGLE_DIR/src/compiler/util.h \ > + $$ANGLE_DIR/src/compiler/ValidateLimitations.h \ > + $$ANGLE_DIR/src/compiler/VariableInfo.h \ > + $$ANGLE_DIR/src/compiler/VersionGLSL.h \ > + > + SOURCES += \ > + $$ANGLE_DIR/src/compiler/CodeGenGLSL.cpp \ > + $$ANGLE_DIR/src/compiler/CodeGenHLSL.cpp \ > + $$ANGLE_DIR/src/compiler/Compiler.cpp \ > + $$ANGLE_DIR/src/compiler/debug.cpp \ > + $$ANGLE_DIR/src/compiler/glslang_lex.cpp \ > + $$ANGLE_DIR/src/compiler/glslang_tab.cpp \ > + $$ANGLE_DIR/src/compiler/ForLoopUnroll.cpp \ > + $$ANGLE_DIR/src/compiler/InfoSink.cpp \ > + $$ANGLE_DIR/src/compiler/Initialize.cpp \ > + $$ANGLE_DIR/src/compiler/InitializeDll.cpp \ > + $$ANGLE_DIR/src/compiler/Intermediate.cpp \ > + $$ANGLE_DIR/src/compiler/intermOut.cpp \ > + $$ANGLE_DIR/src/compiler/IntermTraverse.cpp \ > + $$ANGLE_DIR/src/compiler/ossource_posix.cpp \ > + $$ANGLE_DIR/src/compiler/MapLongVariableNames.cpp \ > + $$ANGLE_DIR/src/compiler/OutputESSL.cpp \ > + $$ANGLE_DIR/src/compiler/OutputGLSL.cpp \ > + $$ANGLE_DIR/src/compiler/OutputGLSLBase.cpp \ > + $$ANGLE_DIR/src/compiler/OutputHLSL.cpp \ > + $$ANGLE_DIR/src/compiler/parseConst.cpp \ > + $$ANGLE_DIR/src/compiler/ParseHelper.cpp \ > + $$ANGLE_DIR/src/compiler/PoolAlloc.cpp \ > + $$ANGLE_DIR/src/compiler/preprocessor/atom.c \ > + $$ANGLE_DIR/src/compiler/preprocessor/cpp.c \ > + $$ANGLE_DIR/src/compiler/preprocessor/cppstruct.c \ > + $$ANGLE_DIR/src/compiler/preprocessor/memory.c \ > + $$ANGLE_DIR/src/compiler/preprocessor/scanner.c \ > + $$ANGLE_DIR/src/compiler/preprocessor/symbols.c \ > + $$ANGLE_DIR/src/compiler/preprocessor/tokens.c \ > + $$ANGLE_DIR/src/compiler/QualifierAlive.cpp \ > + $$ANGLE_DIR/src/compiler/RemoveTree.cpp \ > + $$ANGLE_DIR/src/compiler/SearchSymbol.cpp \ > + $$ANGLE_DIR/src/compiler/ShaderLang.cpp \ > + $$ANGLE_DIR/src/compiler/SymbolTable.cpp \ > + $$ANGLE_DIR/src/compiler/TranslatorESSL.cpp \ > + $$ANGLE_DIR/src/compiler/TranslatorGLSL.cpp \ > + $$ANGLE_DIR/src/compiler/TranslatorHLSL.cpp \ > + $$ANGLE_DIR/src/compiler/UnfoldSelect.cpp \ > + $$ANGLE_DIR/src/compiler/util.cpp \ > + $$ANGLE_DIR/src/compiler/ValidateLimitations.cpp \ > + $$ANGLE_DIR/src/compiler/VariableInfo.cpp \ > + $$ANGLE_DIR/src/compiler/VersionGLSL.cpp \ > + All this should be under !CONTAINS(QT_CONFIG, opengles2) > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:32 > +#if ENABLE(WEBGL) && !defined(QT_OPENGL_ES_2) See previous comment. We can put this in the .pro file under contains(QT_CONFIG, opengles2) > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:344 > + ::glActiveTexture(GL_TEXTURE0); Please ask Martin Robinson to review this.
Created attachment 101843 [details] fix ANGLE compile flags (In reply to comment #6) > > Sorry, I can't let an overarching disabling of warnings in... > We have to figure out a way to diable these for ANGLE files only, > or to live with those warnings being outputted when we build the ANGLE files. I defined a new compiler in QMAKE_EXTRA_COMPILERS, which is just QMAKE_CXX but with the extra warnings turned off and is used only for the ANGLE source files. > All this should be under !CONTAINS(QT_CONFIG, opengles2) > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:32 > > +#if ENABLE(WEBGL) && !defined(QT_OPENGL_ES_2) > > See previous comment. We can put this in the .pro file under > contains(QT_CONFIG, opengles2) Both are done.
(In reply to comment #6) > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:344 > > + ::glActiveTexture(GL_TEXTURE0); > > Please ask Martin Robinson to review this. Martin, could you review this part? Previously this was glActiveTexture(0) which is invalid and generated GL error GL_INVALID_ENUM
CCing Chris Marrin who probably has some more insightful comments than I can provide.
I'm fine with all the Qt parts, the only thing I'm not comfortable reviewing is the change in initialization of (In reply to comment #8) > (In reply to comment #6) > > > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:344 > > > + ::glActiveTexture(GL_TEXTURE0); > > > > Please ask Martin Robinson to review this. > > Martin, could you review this part? Previously this was glActiveTexture(0) which is invalid and generated GL error GL_INVALID_ENUM This is the only change to non-Qt-specific code; I'm fine with the Qt-specific changes after the latest patch.
(In reply to comment #8) > (In reply to comment #6) > > > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:344 > > > + ::glActiveTexture(GL_TEXTURE0); > > > > Please ask Martin Robinson to review this. > > Martin, could you review this part? Previously this was glActiveTexture(0) which is invalid and generated GL error GL_INVALID_ENUM Also one option is to submit this as a different patch, it seems somewhat parallel to the rest of this patch.
Created attachment 101872 [details] remove unrelated glActiveTexture fix (In reply to comment #11) > > Also one option is to submit this as a different patch, > it seems somewhat parallel to the rest of this patch. Yes, it's orthogonal. I've removed it and will submit it under another bug.
Moving the glActiveTexture fix to bug 65115
Comment on attachment 101872 [details] remove unrelated glActiveTexture fix Clearing flags on attachment: 101872 Committed r91694: <http://trac.webkit.org/changeset/91694>
All reviewed patches have been landed. Closing bug.