Bug 64878 - [Qt] Adopt GraphicsContext3DOpenGL.cpp and ANGLE (part 2)
: [Qt] Adopt GraphicsContext3DOpenGL.cpp and ANGLE (part 2)
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Qt
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on: 64877
Blocks: 57261 64879
  Show dependency treegraph
 
Reported: 2011-07-20 10:12 PDT by Andrew Wason
Modified: 2011-07-25 12:21 PDT (History)
4 users (show)

See Also:


Attachments
adopt GraphicsContext3DOpenGL.cpp and ANGLE (14.55 KB, patch)
2011-07-21 11:19 PDT, Andrew Wason
noam: review-
Details | Formatted Diff | Diff
simplified ANGLE build (14.31 KB, patch)
2011-07-21 15:35 PDT, Andrew Wason
noam: review-
Details | Formatted Diff | Diff
fix ANGLE compile flags (15.13 KB, patch)
2011-07-24 18:15 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
remove unrelated glActiveTexture fix (14.50 KB, patch)
2011-07-25 08:46 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wason 2011-07-20 10:12:04 PDT
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 Andrew Wason 2011-07-21 11:19:59 PDT
Created attachment 101609 [details]
adopt GraphicsContext3DOpenGL.cpp and ANGLE
Comment 2 Noam Rosenthal 2011-07-21 12:04:53 PDT
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?
Comment 3 Andrew Wason 2011-07-21 14:10:05 PDT
(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 Andrew Wason 2011-07-21 15:35:10 PDT
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.
Comment 5 Noam Rosenthal 2011-07-24 09:15:57 PDT
> 
> 
> > > 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 Noam Rosenthal 2011-07-24 09:20:05 PDT
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.
Comment 7 Andrew Wason 2011-07-24 18:15:52 PDT
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.
Comment 8 Andrew Wason 2011-07-24 18:18:20 PDT
(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 Martin Robinson 2011-07-24 18:38:27 PDT
CCing Chris Marrin who probably has some more insightful comments than I can provide.
Comment 10 Noam Rosenthal 2011-07-24 19:32:11 PDT
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 Noam Rosenthal 2011-07-25 08:31:41 PDT
(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 Andrew Wason 2011-07-25 08:46:56 PDT
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.
Comment 13 Andrew Wason 2011-07-25 08:56:31 PDT
Moving the glActiveTexture fix to bug 65115
Comment 14 WebKit Review Bot 2011-07-25 12:21:03 PDT
Comment on attachment 101872 [details]
remove unrelated glActiveTexture fix

Clearing flags on attachment: 101872

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