Bug 42405 – ANGLE integration for shader validator and compiler

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Paul Sawaya]

Need to integrate the ANGLE shader compiler into WebCore, which can validate GLSL shaders before they are compiled and executed.

[Paul Sawaya]

Will be used for shader validation (see Appendix A of http://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf). Also necessary for compiling shaders on windows.

[Paul Sawaya]

Created attachment 63388 [details]
Patch to use ANGLE to compile shaders in WebGLRenderingContext

[Chris Marrin]

Comment on attachment 63388 [details]
Patch to use ANGLE to compile shaders in WebGLRenderingContext

> Index: WebCore/WebCore.xcodeproj/project.pbxproj
> ===================================================================
> --- WebCore/WebCore.xcodeproj/project.pbxproj	(revision 64595)
> +++ WebCore/WebCore.xcodeproj/project.pbxproj	(working copy)
> @@ -5235,6 +5235,8 @@
>  		FABE72FE1059C21100D999DD /* MathMLNames.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FABE72FC1059C21100D999DD /* MathMLNames.cpp */; };
>  		FAC12CC41120DA6900DACC36 /* RenderMathMLSubSup.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FAC12CC21120DA6900DACC36 /* RenderMathMLSubSup.cpp */; };
>  		FAC12CC51120DA6900DACC36 /* RenderMathMLSubSup.h in Headers */ = {isa = PBXBuildFile; fileRef = FAC12CC31120DA6900DACC36 /* RenderMathMLSubSup.h */; };
> +		FBF281AF1208D35C00F7F812 /* ANGLEWebkitBridge.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FBF281AD1208D35C00F7F812 /* ANGLEWebkitBridge.cpp */; };
> +		FBF281B01208D35C00F7F812 /* ANGLEWebkitBridge.h in Headers */ = {isa = PBXBuildFile; fileRef = FBF281AE1208D35C00F7F812 /* ANGLEWebkitBridge.h */; };
>  		FE6FD4880F676E5700092873 /* Coordinates.h in Headers */ = {isa = PBXBuildFile; fileRef = FE6FD4850F676E5700092873 /* Coordinates.h */; settings = {ATTRIBUTES = (Private, ); }; };
>  		FE6FD48D0F676E9300092873 /* JSCoordinates.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FE6FD48B0F676E9300092873 /* JSCoordinates.cpp */; };
>  		FE6FD48E0F676E9300092873 /* JSCoordinates.h in Headers */ = {isa = PBXBuildFile; fileRef = FE6FD48C0F676E9300092873 /* JSCoordinates.h */; };
> @@ -11002,6 +11004,8 @@
>  		FABE72FC1059C21100D999DD /* MathMLNames.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = MathMLNames.cpp; sourceTree = "<group>"; };
>  		FAC12CC21120DA6900DACC36 /* RenderMathMLSubSup.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = RenderMathMLSubSup.cpp; sourceTree = "<group>"; };
>  		FAC12CC31120DA6900DACC36 /* RenderMathMLSubSup.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RenderMathMLSubSup.h; sourceTree = "<group>"; };
> +		FBF281AD1208D35C00F7F812 /* ANGLEWebkitBridge.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ANGLEWebkitBridge.cpp; sourceTree = "<group>"; };
> +		FBF281AE1208D35C00F7F812 /* ANGLEWebkitBridge.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ANGLEWebkitBridge.h; sourceTree = "<group>"; };
>  		FE49EF970DC51462004266E1 /* DashboardSupportCSSPropertyNames.in */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = DashboardSupportCSSPropertyNames.in; sourceTree = "<group>"; };
>  		FE6FD4850F676E5700092873 /* Coordinates.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = Coordinates.h; sourceTree = "<group>"; };
>  		FE6FD4860F676E5700092873 /* Coordinates.idl */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = Coordinates.idl; sourceTree = "<group>"; };
> @@ -15521,6 +15525,8 @@
>  				B27535490B053814002CE64F /* mac */,
>  				F4EAF4AB10C74268009100D3 /* opentype */,
>  				49E911B20EF86D27009D0CAF /* transforms */,
> +				FBF281AD1208D35C00F7F812 /* ANGLEWebkitBridge.cpp */,
> +				FBF281AE1208D35C00F7F812 /* ANGLEWebkitBridge.h */,
>  				A89943270B42338700D7C802 /* BitmapImage.cpp */,
>  				A89943260B42338700D7C802 /* BitmapImage.h */,
>  				B27535380B053814002CE64F /* Color.cpp */,
> @@ -19983,6 +19989,7 @@
>  				D39D009D11F907E6006041F2 /* SearchPopupMenuMac.h in Headers */,
>  				B61762541203374F00EF9114 /* IDBDatabaseBackendInterface.h in Headers */,
>  				B61762621203490800EF9114 /* IDBDatabaseBackendImpl.h in Headers */,
> +				FBF281B01208D35C00F7F812 /* ANGLEWebkitBridge.h in Headers */,
>  			);
>  			runOnlyForDeploymentPostprocessing = 0;
>  		};
> @@ -20042,7 +20049,6 @@
>  			isa = PBXProject;
>  			buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */;
>  			compatibilityVersion = "Xcode 2.4";
> -			developmentRegion = English;
>  			hasScannedForEncodings = 1;
>  			knownRegions = (
>  				English,
> @@ -22387,6 +22393,7 @@
>  				E1BE512D0CF6C512002EA959 /* XSLTUnicodeSort.cpp in Sources */,
>  				97DD4D860FDF4D6E00ECF9A4 /* XSSAuditor.cpp in Sources */,
>  				C046E1AC1208A9FE00BA2CF7 /* LocalizedStrings.cpp in Sources */,
> +				FBF281AF1208D35C00F7F812 /* ANGLEWebkitBridge.cpp in Sources */,
>  			);
>  			runOnlyForDeploymentPostprocessing = 0;
>  		};
> Index: WebCore/html/canvas/WebGLRenderingContext.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLRenderingContext.cpp	(revision 64595)
> +++ WebCore/html/canvas/WebGLRenderingContext.cpp	(working copy)
> @@ -29,6 +29,7 @@
>  
>  #include "WebGLRenderingContext.h"
>  
> +#include "ANGLEWebkitBridge.h"
>  #include "CheckedInt.h"
>  #include "CanvasPixelArray.h"
>  #include "Console.h"
> @@ -516,7 +517,10 @@ void WebGLRenderingContext::compileShade
>      UNUSED_PARAM(ec);
>      if (!validateWebGLObject(shader))
>          return;
> -    m_context->compileShader(shader ? shader->object() : 0);
> +    
> +    if (ANGLEWebkitBridge::validateShader(shader,getShaderSource(shader,ec).utf8().data()))
> +        m_context->compileShader(shader ? shader->object() : 0);
> +
>      cleanupAfterGraphicsCall(false);
>  }
>  
> @@ -1561,7 +1565,12 @@ String WebGLRenderingContext::getShaderI
>      if (!validateWebGLObject(shader))
>          return "";
>      WebGLStateRestorer(this, false);
> -    return m_context->getShaderInfoLog(shader ? shader->object() : 0);
> +    
> +    // If the shader failed validation, return the ANGLE validation log instead
> +    if (shader->passedValidation())
> +        return m_context->getShaderInfoLog(shader ? shader->object() : 0);
> +    else
> +        return shader->getValidatorLog();
>  }
>  
>  String WebGLRenderingContext::getShaderSource(WebGLShader* shader, ExceptionCode& ec)
> Index: WebCore/html/canvas/WebGLShader.cpp
> ===================================================================
> --- WebCore/html/canvas/WebGLShader.cpp	(revision 64595)
> +++ WebCore/html/canvas/WebGLShader.cpp	(working copy)
> @@ -38,8 +38,9 @@ PassRefPtr<WebGLShader> WebGLShader::cre
>  }
>  
>  WebGLShader::WebGLShader(WebGLRenderingContext* ctx, GraphicsContext3D::WebGLEnumType type)
> -    : CanvasObject(ctx)
> -    , m_type(type)
> +    : CanvasObject(ctx),
> +    m_type(type), 
> +    m_validated(false)
>  {
>      setObject(context()->graphicsContext3D()->createShader(type));
>  }
> Index: WebCore/html/canvas/WebGLShader.h
> ===================================================================
> --- WebCore/html/canvas/WebGLShader.h	(revision 64595)
> +++ WebCore/html/canvas/WebGLShader.h	(working copy)
> @@ -28,6 +28,8 @@
>  
>  #include "CanvasObject.h"
>  
> +#include "PlatformString.h"
> +
>  #include <wtf/PassRefPtr.h>
>  #include <wtf/RefCounted.h>
>  
> @@ -36,11 +38,20 @@ namespace WebCore {
>      class WebGLShader : public CanvasObject {
>      public:
>          virtual ~WebGLShader() { deleteObject(); }
> -        
> +
>          static PassRefPtr<WebGLShader> create(WebGLRenderingContext*, GraphicsContext3D::WebGLEnumType);
>  
>          GraphicsContext3D::WebGLEnumType getType() const { return m_type; }
>  
> +		bool passedValidation() { return m_validated; }		
> +		void setValidated(bool validated) { m_validated = validated; }
> +		
> +		String getValidatorLog() { return m_validationLog; }
> +		void setValidatorLog(String messages) { m_validationLog = messages; }
> +		
> +		String getTranslatedSource() { return m_translatedSource; }
> +		void setTranslatedSource(String source) { m_translatedSource = source; }
> +
>      private:
>          WebGLShader(WebGLRenderingContext*, GraphicsContext3D::WebGLEnumType);
>  
> @@ -49,6 +60,13 @@ namespace WebCore {
>          virtual bool isShader() const { return true; }
>  
>          GraphicsContext3D::WebGLEnumType m_type;
> +
> +		String m_validationLog;
> +		
> +		//Desktop GLSL source
> +		String m_translatedSource;
> +
> +		bool m_validated;
>      };
>      
>  } // namespace WebCore
> Index: WebCore/platform/graphics/ANGLEWebkitBridge.cpp
> ===================================================================
> --- WebCore/platform/graphics/ANGLEWebkitBridge.cpp	(revision 0)
> +++ WebCore/platform/graphics/ANGLEWebkitBridge.cpp	(revision 0)
> @@ -0,0 +1,80 @@
> +/*
> + *  ANGLEWebkitBridge.cpp
> + *  WebCore
> + *
> + *  Created by Paul Sawaya on 7/15/10.
> + *  Copyright 2010 Apple Inc. All rights reserved.
> + *
> + */
> +
> +#include "config.h"
> +
> +#if ENABLE(3D_CANVAS)
> +
> +#include "ANGLEWebkitBridge.h"
> +
> +#include "ActiveDOMObject.h"
> +#include "GraphicsContext3D.h"
> +#include <OpenGL/gl.h>
> +#include "ShaderLang.h"
> +#include "WebGLShader.h"
> +
> +#include <wtf/text/CString.h>
> +
> +namespace WebCore {
> +        
> +        
> +// Set up the per compile resources
> +static void generateResources(TBuiltInResource& resources)
> +{
> +    resources.maxVertexAttribs = 8;
> +    resources.maxVertexUniformVectors = 128;
> +    resources.maxVaryingVectors = 8;
> +    resources.maxVertexTextureImageUnits = 0;
> +    resources.maxCombinedTextureImageUnits = 8;
> +    resources.maxTextureImageUnits = 8;
> +    resources.maxFragmentUniformVectors = 16;
> +    resources.maxDrawBuffers = 1;
> +}
> +
> +bool ANGLEWebkitBridge::validateShader(WebGLShader* shader, const char* shaderSource)
> +{
> +    TBuiltInResource resources;
> +    EShLanguage shaderLang;
> +    ShHandle compiler;
> +
> +    generateResources(resources);
> +    
> +if (shader->getType() == GraphicsContext3D::VERTEX_SHADER)
> +        shaderLang = EShLangVertex;
> +    else if (shader->getType() == GraphicsContext3D::FRAGMENT_SHADER)
> +        shaderLang = EShLangFragment;
> +    else
> +        return false; // Invalid shader type
> +        
> +    ShInitialize();
> +    
> +    compiler = ShConstructCompiler(shaderLang, EShSpecWebGL);
> +    
> +    const char* const shaderSourceStrings[] = {shaderSource};
> +    
> +    bool validateSuccess = ShCompile(compiler, shaderSourceStrings, 1, EShOptNone, &resources, EDebugOpIntermediate);
> +    
> +    shader->setValidated(validateSuccess);
> +    
> +    if (validateSuccess) {
> +        shader->setTranslatedSource(ShGetObjectCode(compiler));
> +    }
> +    else {
> +        // TODO: drop in ShGetValidatorLog once new patches are accepted by ANGLE
> +        shader->setValidatorLog("Validation failed.");
> +    }
> +
> +    ShDestruct(compiler);
> +
> +    return validateSuccess;
> +}
> +
> +}
> +
> +#endif // ENABLE(3D_CANVAS)
> Index: WebCore/platform/graphics/ANGLEWebkitBridge.h
> ===================================================================
> --- WebCore/platform/graphics/ANGLEWebkitBridge.h	(revision 0)
> +++ WebCore/platform/graphics/ANGLEWebkitBridge.h	(revision 0)
> @@ -0,0 +1,28 @@
> +/*
> + *  ANGLEWebkitBridge.h
> + *  WebCore
> + *
> + *  Created by Paul Sawaya on 7/15/10.
> + *  Copyright 2010 Apple Inc. All rights reserved.
> + *
> + */
> +
> +#ifndef ANGLEWebkitBridge_h
> +#define ANGLEWebkitBridge_h
> +
> +#include "GraphicsContext3D.h"
> +#include "ShaderLang.h"
> +#include "WebGLShader.h"
> +
> +namespace WebCore {
> +
> +class ANGLEWebkitBridge {
> +public:
> +
> +	static bool validateShader(WebGLShader* shader, const char* shaderSource);
> +
> +};
> +
> +} //namespace WebCore
> +
> +#endif

WebCore/html/canvas/WebGLShader.cpp:43
 +      m_validated(false)
WebKit style is:
    : CanvasObject(ctx)
    , m_type(type)
    , m_validated(false)

WebCore/html/canvas/WebGLShader.h:41
 +  
Try to avoid spurious changes like deleting whitespace

WebCore/html/canvas/WebGLShader.h:48
 +  		
Since this flag indicates whether or not this shader has passed validation, maybe a better name would be m_valid. Then the calls would be isValid() and setValid(bool).

WebCore/html/canvas/WebGLShader.h:54
 +  
WebKit style is to not use "get". So the above calls would be validatorLog() and translatedSource().

WebCore/html/canvas/WebGLShader.h:65
 +  		
The calls should match the name of the variable. So it should be m_validatorLog, or the call names should be changed.

WebCore/platform/graphics/ANGLEWebkitBridge.cpp:18
 +  #include <OpenGL/gl.h>
This code is desktop GL specific? It shouldn't be. And I think GraphicsContext3D includes gl.h for Mac.

WebCore/platform/graphics/ANGLEWebkitBridge.cpp:69
 +          // TODO: drop in ShGetValidatorLog once new patches are accepted by ANGLE
WebKit style is to use FIXME rather than TODO

WebCore/platform/graphics/ANGLEWebkitBridge.h:13
 +  #include "GraphicsContext3D.h"
No need to include GraphicsContext3D.h in both the .cpp and .h files

[Kenneth Russell]

Comment on attachment 63388 [details]
Patch to use ANGLE to compile shaders in WebGLRenderingContext

WebCore/platform/graphics/ANGLEWebkitBridge.cpp:8
 +   */
This copyright notice is not in the correct form. Please copy one from one of the other files in this directory.


WebCore/platform/graphics/ANGLEWebkitBridge.cpp:48
 +  if (shader->getType() == GraphicsContext3D::VERTEX_SHADER)
Indentation is incorrect here.


WebCore/platform/graphics/ANGLEWebkitBridge.h:8
 +   */
Same issue with copyright notice.


WebCore/html/canvas/WebGLShader.h:46
 +  		bool passedValidation() { return m_validated; }		
Indentation is incorrect in this added block of code.


WebCore/html/canvas/WebGLShader.h:64
 +  		String m_validationLog;
Indentation is incorrect.


WebCore/platform/graphics/ANGLEWebkitBridge.cpp:16
 +  #include "ActiveDOMObject.h"
This seems like an unnecessary #include.


WebCore/platform/graphics/ANGLEWebkitBridge.cpp:37
 +      resources.maxDrawBuffers = 1;
These values need to be queried from the current OpenGL context. For this reason I think ANGLEWebKitBridge should probably be a class instantiated one per WebGLRenderingContext and kept as a member so you can cache these results. You will also be able to reuse the compiler objects. See GLES2DecoderImpl::Initialize, http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/service/gles2_cmd_decoder.cc?view=markup , and its instantiation of vertex_compiler_ and fragment_compiler_.

[Kenneth Russell]

Paul, I've upgraded the version of ANGLE in WebKit which involves a few minor API changes to the shader compiler. Apologies for any difficulty this causes you. Please consult the gles2_cmd_decoder.cc sources in the comment above for example usage.

[Kenneth Russell]

It would also be really helpful if, when you add your new sources, you could also add them to Chromium's build generation files. Here's a diff for your current patch:

Index: WebCore/WebCore.gypi
===================================================================
--- WebCore/WebCore.gypi	(revision 64680)
+++ WebCore/WebCore.gypi	(working copy)
@@ -2438,6 +2438,8 @@
             'platform/graphics/wx/PenWx.cpp',
             'platform/graphics/wx/SimpleFontDataWx.cpp',
             'platform/graphics/wx/TransformationMatrixWx.cpp',
+            'platform/graphics/ANGLEWebKitBridge.cpp',
+            'platform/graphics/ANGLEWebkitBridge.h',
             'platform/graphics/BitmapImage.cpp',
             'platform/graphics/BitmapImage.h',
             'platform/graphics/Color.cpp',

[Paul Sawaya]

Created attachment 63672 [details]
Made requested changes

[Kenneth Russell]

This looks really nice. Thanks for making the changes. I'm not a reviewer so can't r+ your patch, but otherwise would. Explicitly CC'ing a couple of reviewers. A couple of tiny issues: the year in the new copyright headers is wrong, and I don't know whether the naming convention of "validatorLog()/validatorLog(String)" would be preferred in WebKit, but it seems OK to me.

[Kenneth Russell]

Comment on attachment 63672 [details]
Made requested changes

WebCore/html/canvas/WebGLRenderingContext.cpp:552
 +          shaderSource(shader, shader->translatedSource(), ec);
Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.

[Paul Sawaya]

(In reply to comment #9)
> (From update of attachment 63672 [details])
> WebCore/html/canvas/WebGLRenderingContext.cpp:552
>  +          shaderSource(shader, shader->translatedSource(), ec);
> Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.

No problem. Where would be a good place to hand off the translated source to OpenGL on the desktop, then?

[Oliver Hunt]

(In reply to comment #9)
> (From update of attachment 63672 [details])
> WebCore/html/canvas/WebGLRenderingContext.cpp:552
>  +          shaderSource(shader, shader->translatedSource(), ec);
> Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.

Hmmm, i haven't looked at how ANGLE is expected to work but i would have thought that you tell it what the target platform is, and have it produce an appropriate translation.  It seems at the very least inefficient to pass the untranslated code to GraphicsContext3D as the underlying implementation would need to reparse + regenerate the code before passing to the underlying runtime.

[Kenneth Russell]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 63672 [details] [details])
> > WebCore/html/canvas/WebGLRenderingContext.cpp:552
> >  +          shaderSource(shader, shader->translatedSource(), ec);
> > Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.
> 
> No problem. Where would be a good place to hand off the translated source to OpenGL on the desktop, then?

I think it should work in the following manner. When WebGLRenderingContext::shaderSource() is called, the sources are stored in the WebGLShader object and not passed to GraphicsContext3D at all. When WebGLRenderingContext::compileShader() is called, it should run the validator on the cached source, and only if it is valid, pass through the source to a pair of calls to GraphicsContext3D::shaderSource() and compileShader(). Add a HashMap from Platform3DObject (shader ID) to String in GraphicsContext3DMac.mm. Upon a call to GraphicsContext3D::shaderSource, save off the source (GLES) string in this map. Later, in GraphicsContext3D::compileShader, run another instance of the compiler on that string and pass the result down to desktop GL.

Alternatively we could change GraphicsContext3D to combine the entry points into something like compileShaderFromSource(Platform3DObject, String), although to do that and not break any platform's build you'll have to modify the Chromium and Qt ports. That may end up being a much simpler solution, though.

[Kenneth Russell]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 63672 [details] [details])
> > WebCore/html/canvas/WebGLRenderingContext.cpp:552
> >  +          shaderSource(shader, shader->translatedSource(), ec);
> > Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.
> 
> Hmmm, i haven't looked at how ANGLE is expected to work but i would have thought that you tell it what the target platform is, and have it produce an appropriate translation.  It seems at the very least inefficient to pass the untranslated code to GraphicsContext3D as the underlying implementation would need to reparse + regenerate the code before passing to the underlying runtime.

WebGLRenderingContext should not have any notion of what underlying shader language is being used by the GraphicsContext3D implementation (desktop GLSL, HLSL for Direct3D, etc.). Yes, there is a certain amount of inefficiency, but doing the translation and not just validation in WebGLRenderingContext breaks a huge number of abstraction barriers.

[Oliver Hunt]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (From update of attachment 63672 [details] [details] [details])
> > > WebCore/html/canvas/WebGLRenderingContext.cpp:552
> > >  +          shaderSource(shader, shader->translatedSource(), ec);
> > > Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.
> > 
> > Hmmm, i haven't looked at how ANGLE is expected to work but i would have thought that you tell it what the target platform is, and have it produce an appropriate translation.  It seems at the very least inefficient to pass the untranslated code to GraphicsContext3D as the underlying implementation would need to reparse + regenerate the code before passing to the underlying runtime.
> 
> WebGLRenderingContext should not have any notion of what underlying shader language is being used by the GraphicsContext3D implementation (desktop GLSL, HLSL for Direct3D, etc.). Yes, there is a certain amount of inefficiency, but doing the translation and not just validation in WebGLRenderingContext breaks a huge number of abstraction barriers.

:-/ it's icky -- i don't like the idea of ever passing untranslated source anywhere near the actual backend, as you mentioned above a shader object may be best, maybe with a function akin to:

ShaderObject::getSystemSource(targetRuntime)

that would be called at the backend.

Could we also please stop using the term "passing through" to me at least it implies that the initial source may be passed through to the backend directly, which can never happen -- even if the backend were GL|ES2, we would need to send translated content to avoid exposing security vulnerabilities due to parser differences.

[Chris Marrin]

Comment on attachment 63672 [details]
Made requested changes

WebCore/html/canvas/WebGLRenderingContext.cpp:96
 +      return new WebGLRenderingContext(canvas, context.release(), angleCompiler.release());


I don't see why you're creating the compiler here and passing it in. Why not just create it in the ctor?



WebCore/html/canvas/WebGLRenderingContext.cpp:109
 +      , m_compiler(angleCompiler)


You should be able to say:

    , m_compiler(adoptPtr(new ANGLEWebKitBridge))



WebCore/html/canvas/WebGLShader.cpp:44
 +      m_valid(false)


WebKit style is:

    : WebGLObject(ctx)
    , m_type(type)
    , m_valid(false)



WebCore/platform/graphics/ANGLEWebkitBridge.h:38
 +      ANGLEWebkitBridge();


No space needed here



WebCore/platform/graphics/ANGLEWebkitBridge.h:46
 +      


or here

[Chris Marrin]

Comment on attachment 63672 [details]
Made requested changes

WebCore/html/canvas/WebGLRenderingContext.cpp:555
 +  


You shouldn't call shaderSource here. When the user gets the shader source, he should get what he passed in, not the translated source. This is the issue I was talking about before. We need separate validation and translation phases. GraphicsContext3D should take in GLSL ES source and translate it as needed for the current hardware. For now we should just be doing the translation phase.

[Chris Marrin]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 63672 [details] [details])
> > WebCore/html/canvas/WebGLRenderingContext.cpp:552
> >  +          shaderSource(shader, shader->translatedSource(), ec);
> > Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.
> 
> Hmmm, i haven't looked at how ANGLE is expected to work but i would have thought that you tell it what the target platform is, and have it produce an appropriate translation.  It seems at the very least inefficient to pass the untranslated code to GraphicsContext3D as the underlying implementation would need to reparse + regenerate the code before passing to the underlying runtime.

Right, but I think at this point, we have to do it that way to avoid layering violations. GraphicsContext3D is a OpenGL ES 2.0 based API, so it is expecting GLSL ES code.

Later we might want to move the validation back down into GraphicsContext3D and have a flag to turn validation and translation on or off. We would also need a way to get back the validation info log. This will allow us to do the validation and translation in one pass, avoid layering violations and still allow internal users to turn all this off for efficiency. Turning it off would of course mean  that the client would have to make sure to send down shaders in the appropriate hardware specific language.

[Chris Marrin]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 63672 [details] [details] [details])
> > > WebCore/html/canvas/WebGLRenderingContext.cpp:552
> > >  +          shaderSource(shader, shader->translatedSource(), ec);
> > > Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.
> > 
> > No problem. Where would be a good place to hand off the translated source to OpenGL on the desktop, then?
> 
> I think it should work in the following manner. When WebGLRenderingContext::shaderSource() is called, the sources are stored in the WebGLShader object and not passed to GraphicsContext3D at all. When WebGLRenderingContext::compileShader() is called, it should run the validator on the cached source, and only if it is valid, pass through the source to a pair of calls to GraphicsContext3D::shaderSource() and compileShader(). Add a HashMap from Platform3DObject (shader ID) to String in GraphicsContext3DMac.mm. Upon a call to GraphicsContext3D::shaderSource, save off the source (GLES) string in this map. Later, in GraphicsContext3D::compileShader, run another instance of the compiler on that string and pass the result down to desktop GL.
> 
> Alternatively we could change GraphicsContext3D to combine the entry points into something like compileShaderFromSource(Platform3DObject, String), although to do that and not break any platform's build you'll have to modify the Chromium and Qt ports. That may end up being a much simpler solution, though.

GraphicsContext3D should expect valid GLSL ES code and should do the translation itself as needed. This adds an extra compiler pass, but it keeps the API clean and we can deal with optimizations later. I realize this means instantiating yet another ANGLECompiler, but I think it's more important to keep the API clean at this point.

For this patch, it should remain just a validator patch, not a translator patch. We should open a new bug for the translation.

We should open yet another bug for the optimization. Here's how I think that should work:

1) Remove validation from WebGLRenderingContext. 

2) Change GraphicsContext3D::compileShader to:

    bool compileShader(Platform3DObject, bool validateAndTranslate, String& validationInfoLog)

3) Put the validation and translation logic in GraphicsContext3D::compileShader.

[Kenneth Russell]

(In reply to comment #18)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (From update of attachment 63672 [details] [details] [details] [details])
> > > > WebCore/html/canvas/WebGLRenderingContext.cpp:552
> > > >  +          shaderSource(shader, shader->translatedSource(), ec);
> > > > Actually, sorry, I have to point out one more problem. At this level the translated shader source must not be used -- the incoming OpenGL ES 2.0 sources must be passed through to the GraphicsContext3D unmodified. This is because some GraphicsContext3D implementations may be running on OpenGL ES 2.0 directly, not desktop GL. All that should be done at the WebGLRenderingContext level is initial validation.
> > > 
> > > No problem. Where would be a good place to hand off the translated source to OpenGL on the desktop, then?
> > 
> > I think it should work in the following manner. When WebGLRenderingContext::shaderSource() is called, the sources are stored in the WebGLShader object and not passed to GraphicsContext3D at all. When WebGLRenderingContext::compileShader() is called, it should run the validator on the cached source, and only if it is valid, pass through the source to a pair of calls to GraphicsContext3D::shaderSource() and compileShader(). Add a HashMap from Platform3DObject (shader ID) to String in GraphicsContext3DMac.mm. Upon a call to GraphicsContext3D::shaderSource, save off the source (GLES) string in this map. Later, in GraphicsContext3D::compileShader, run another instance of the compiler on that string and pass the result down to desktop GL.
> > 
> > Alternatively we could change GraphicsContext3D to combine the entry points into something like compileShaderFromSource(Platform3DObject, String), although to do that and not break any platform's build you'll have to modify the Chromium and Qt ports. That may end up being a much simpler solution, though.
> 
> GraphicsContext3D should expect valid GLSL ES code and should do the translation itself as needed. This adds an extra compiler pass, but it keeps the API clean and we can deal with optimizations later. I realize this means instantiating yet another ANGLECompiler, but I think it's more important to keep the API clean at this point.
> 
> For this patch, it should remain just a validator patch, not a translator patch. We should open a new bug for the translation.
> 
> We should open yet another bug for the optimization. Here's how I think that should work:
> 
> 1) Remove validation from WebGLRenderingContext. 
> 
> 2) Change GraphicsContext3D::compileShader to:
> 
>     bool compileShader(Platform3DObject, bool validateAndTranslate, String& validationInfoLog)
> 
> 3) Put the validation and translation logic in GraphicsContext3D::compileShader.

This is certainly one way of doing it and would be fine from my standpoint, but it will still require a HashMap from shader to sources to be maintained in the GraphicsContext3D implementation. It would be easier to "buffer up" the calls at the WebGLRenderingContext layer and change the GraphicsContext3D entry point to something like compileShaderFromSource(Platform3DObject, const String& source, ...). I don't think we should add a validationInfoLog parameter to compileShader. Any validation log should simply be passed back via getShaderInfoLog.

[Chris Marrin]

(In reply to comment #19)
> ...
> > We should open yet another bug for the optimization. Here's how I think that should work:
> > 
> > 1) Remove validation from WebGLRenderingContext. 
> > 
> > 2) Change GraphicsContext3D::compileShader to:
> > 
> >     bool compileShader(Platform3DObject, bool validateAndTranslate, String& validationInfoLog)
> > 
> > 3) Put the validation and translation logic in GraphicsContext3D::compileShader.
> 
> This is certainly one way of doing it and would be fine from my standpoint, but it will still require a HashMap from shader to sources to be maintained in the GraphicsContext3D implementation. It would be easier to "buffer up" the calls at the WebGLRenderingContext layer and change the GraphicsContext3D entry point to something like compileShaderFromSource(Platform3DObject, const String& source, ...). I don't think we should add a validationInfoLog parameter to compileShader. Any validation log should simply be passed back via getShaderInfoLog.

The shader source has to be buffered somewhere. The way Paul has it now they're buffered in GraphicsContext3D in a HashMap as you suggest. I'd prefer the main calls work like they do in GL: source sent in shaderSource, compiled in compileSource. I suppose we could also put the info log entry returned from the validator into the HashMap rather than returning it. That would be a bit cleaner. I'll talk to Paul about it.

[Paul Sawaya]

Created attachment 64080 [details]
Stores data about shader (log, translated source, if valid) in HashMap instead of WebGLShader object

[Chris Marrin]

Comment on attachment 64080 [details]
Stores data about shader (log, translated source, if valid) in HashMap instead of WebGLShader object

> @@ -998,11 +1062,12 @@ void GraphicsContext3D::shaderSource(Pla
>      ASSERT(shader);
>      
>      ensureContext(m_contextObj);
> -    const CString& cs = string.utf8();
> -    const char* s = cs.data();
> -    
> -    int length = string.length();
> -    ::glShaderSource((GLuint) shader, 1, &s, &length);
> +
> +    ShaderSourceEntry entry;
> +
> +    entry.source = string;
> +
> +    m_shaderSourceMap.set(shader, entry);
>  }
>  
>  void GraphicsContext3D::stencilFunc(unsigned long func, long ref, unsigned long mask)
> @@ -1352,20 +1417,35 @@ void GraphicsContext3D::getShaderiv(Plat
>  String GraphicsContext3D::getShaderInfoLog(Platform3DObject shader)
>  {
>      ASSERT(shader);
>      ensureContext(m_contextObj);
>      GLint length;
>      ::glGetShaderiv((GLuint) shader, GL_INFO_LOG_LENGTH, &length);
> -    
> -    GLsizei size;
> -    GLchar* info = (GLchar*) fastMalloc(length);
> -    if (!info)
> -        return "";
> -        
> -    ::glGetShaderInfoLog((GLuint) shader, length, &size, info);
> -    String s(info);
> -    fastFree(info);
> -    return s;
> +
> +    HashMap<Platform3DObject, ShaderSourceEntry>::iterator result = m_shaderSourceMap.find(shader);
> +
> +    if (result == m_shaderSourceMap.end())
> +         return "";
> +
> +     ShaderSourceEntry entry = result->second;
> +
> +     if (entry.isValid) {
> +         GLint length;
> +         ::glGetShaderiv((GLuint) shader, GL_INFO_LOG_LENGTH, &length);


Do you need to get INFO_LOG_LENGTH twice?


> +
> +         GLsizei size;
> +         GLchar* info = (GLchar*) fastMalloc(length);
> +         if (!info)
> +             return "";
> +
> +         ::glGetShaderInfoLog((GLuint) shader, length, &size, info);
> +         String s(info);
> +         fastFree(info);
> +         return s;
> +     }
> +     else {
> +         return entry.log;
> +     }
>  }
>  
>  String GraphicsContext3D::getShaderSource(Platform3DObject shader)

WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:1447
 +       }


The above lines are indented 5, they should be indented 4.


r+ with the above fixes

Also, this really needs testing. I really want this checked in, so existing tests will do for now. Please submit a new bug for tests. You should add tests that show the appropriate message return for a failed validation.

[Chris Marrin]

Comment on attachment 64080 [details]
Stores data about shader (log, translated source, if valid) in HashMap instead of WebGLShader object

I changed this to r-. After trying to apply the patch to my tree I noticed that it fails to patch the project file, probably because it is out of date. It will be easier for you to fix this on your machine. Just svn update, which will probably result in a conflict on the project file. Fix that conflict, fix the other issues I noted in my review and then submit a new patch. If it's clean, we can have the commit-queue land it, which will be much easier.

[Kenneth Russell]

Comment on attachment 64080 [details]
Stores data about shader (log, translated source, if valid) in HashMap instead of WebGLShader object

WebCore/platform/graphics/ANGLEWebkitBridge.cpp:2
 +   * Copyright (C) 2009 Apple Inc. All rights reserved.
Copyright year should be 2010.


WebCore/platform/graphics/ANGLEWebkitBridge.cpp:44
 +      printf("done with compiler constructing.\n");
printf should be removed.


WebCore/platform/graphics/ANGLEWebkitBridge.h:2
 +   * Copyright (C) 2009 Apple Inc. All rights reserved.
Copyright year.


WebCore/platform/graphics/GraphicsContext3D.h:817
 +      HashMap<Platform3DObject, ShaderSourceEntry> m_shaderSourceMap;
You could typedef HashMap<Platform3DObject, ShaderSourceEntry> to something like ShaderSourceMap for conciseness.


WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:664
 +      ShaderSourceEntry entry;
I think you can use:
ShaderSourceEntry& entry = result->second;
to avoid the copying and re-setting of the entry in the map.


WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:1429
 +       ShaderSourceEntry entry = result->second;
I think you can use const ShaderSourceEntry& entry = ...


WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:1427
 +           return "";
Indentation is off.


WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:1445
 +       else {
"else" should be on same line as }.

[Paul Sawaya]

Created attachment 64381 [details]
Changes to WebCore for ANGLE

[Paul Sawaya]

Created attachment 64382 [details]
Changes to LayoutTests for ANGLE

[WebKit Review Bot]

Attachment 64381 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/ANGLEWebkitBridge.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 64381 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3767145

[Chris Marrin]

Comment on attachment 64381 [details]
Changes to WebCore for ANGLE

Did you redo the xcodeproj file? It is still failing to patch on TOT.

Please fix the style issues pointed out by the style bot

WebCore/platform/graphics/GraphicsContext3D.h:29
 +  #include "ANGLEWebkitBridge.h"
This include is causing the Qt build to fail. You can fix that by adding:

    class ANGLEWebkitBridge;

You are already including the .h file in the Mac specific .mm file.

Also, the proper case is ANGLWebKitBridge. Please change to match.

[Chris Marrin]

Comment on attachment 64381 [details]
Changes to WebCore for ANGLE

r+ after making requested changes

[Chris Marrin]

Comment on attachment 64382 [details]
Changes to LayoutTests for ANGLE

r+

[Paul Sawaya]

Resolved in http://trac.webkit.org/changeset/65446

[WebKit Review Bot]

http://trac.webkit.org/changeset/65446 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release