Bug 64878

Summary: [Qt] Adopt GraphicsContext3DOpenGL.cpp and ANGLE (part 2)
Product: WebKit Reporter: Andrew Wason <rectalogic@rectalogic.com>
Component: WebKit QtAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin@apple.com, mrobinson@webkit.org, noam@webkit.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 64877    
Bug Blocks: 57261, 64879    
Attachments:
Description Flags
adopt GraphicsContext3DOpenGL.cpp and ANGLE
noam: review-
simplified ANGLE build
noam: review-
fix ANGLE compile flags
none
remove unrelated glActiveTexture fix none

Description From 2011-07-20 10:12:04 PST
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.
------- Comment #1 From 2011-07-21 11:19:59 PST -------
Created an attachment (id=101609) [details]
adopt GraphicsContext3DOpenGL.cpp and ANGLE
------- Comment #2 From 2011-07-21 12:04:53 PST -------
(From update of attachment 101609 [details])
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?
------- Comment #3 From 2011-07-21 14:10:05 PST -------
(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.
------- Comment #4 From 2011-07-21 15:35:10 PST -------
Created an attachment (id=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.
------- Comment #5 From 2011-07-24 09:15:57 PST -------
> 
> 
> > > 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 #6 From 2011-07-24 09:20:05 PST -------
(From update of attachment 101652 [details])
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.
------- Comment #7 From 2011-07-24 18:15:52 PST -------
Created an attachment (id=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.
------- Comment #8 From 2011-07-24 18:18:20 PST -------
(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
------- Comment #9 From 2011-07-24 18:38:27 PST -------
CCing Chris Marrin who probably has some more insightful comments than I can provide.
------- Comment #10 From 2011-07-24 19:32:11 PST -------
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.
------- Comment #11 From 2011-07-25 08:31:41 PST -------
(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.
------- Comment #12 From 2011-07-25 08:46:56 PST -------
Created an attachment (id=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.
------- Comment #13 From 2011-07-25 08:56:31 PST -------
Moving the glActiveTexture fix to bug 65115
------- Comment #14 From 2011-07-25 12:21:03 PST -------
(From update of attachment 101872 [details])
Clearing flags on attachment: 101872

Committed r91694: <http://trac.webkit.org/changeset/91694>
------- Comment #15 From 2011-07-25 12:21:08 PST -------
All reviewed patches have been landed.  Closing bug.