Bug 64878 – [Qt] Adopt GraphicsContext3DOpenGL.cpp and ANGLE (part 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Andrew Wason]

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.

[Andrew Wason]

Created attachment 101609 [details]
adopt GraphicsContext3DOpenGL.cpp and ANGLE

[Noam Rosenthal]

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?

[Andrew Wason]

(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.

[Andrew Wason]

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.

[Noam Rosenthal]

> 
> 
> > > 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)

[Noam Rosenthal]

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.

[Andrew Wason]

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.

[Andrew Wason]

(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

[Martin Robinson]

CCing Chris Marrin who probably has some more insightful comments than I can provide.

[Noam Rosenthal]

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.

[Noam Rosenthal]

(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.

[Andrew Wason]

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.

[Andrew Wason]

Moving the glActiveTexture fix to bug 65115

[WebKit Review Bot]

Comment on attachment 101872 [details]
remove unrelated glActiveTexture fix

Clearing flags on attachment: 101872

Committed r91694: <http://trac.webkit.org/changeset/91694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.