Bug 44309 – Hook up ANGLE with chromium --in-process-webgl port

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

Currently test_shell is still using --in-process-webgl port.  In theory for webgl, we shouldn't need "#ifdef GL_ES" around precision modifier.  However, currently we still have to do so for chromium to pass the tests.

Once we hook up ANGEL with the port, we should be able to remove the "#ifdef GL_ES" both in khronos and in LayoutTests.

[Zhenyao Mo]

Created attachment 65965 [details]
patch

Refactor the ANGLEWebKitBridge class a little bit.  Also, fix some issues in the Mac port in ANGLE shader validation stuff.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:662:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:974:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 15 files


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

[Zhenyao Mo]

Created attachment 65968 [details]
revised patch: fix style issues

[WebKit Review Bot]

Attachment 65968 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3816186

[Zhenyao Mo]

Created attachment 65993 [details]
revised patch: fix the filename case issue in WebCore.gypi

[WebKit Review Bot]

Attachment 65993 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3816190

[Zhenyao Mo]

Created attachment 66078 [details]
revised patch: adapting to newer version of ANGLE interface

[Adam Barth]

Comment on attachment 66078 [details]
revised patch: adapting to newer version of ANGLE interface

View in context: https://bugs.webkit.org/attachment.cgi?id=66078&action=prettypatch

> WebCore/ChangeLog:-10
> -2010-08-31  Jian Li  <[e-mail]>
> [phone]  Zhenyao Mo  <[e-mail]>
>  
> -        Reviewed by Darin Fisher.
> +        Reviewed by NOBODY (OOPS!).
>  
> -        Improve BlobBuilder to combine adjacent strings.
> -        https://bugs.webkit.org/post_bug.cgi
> +        Hook up ANGLE with chromium --in-process-webgl port
> +        https://bugs.webkit.org/show_bug.cgi?id=44309
>  
> -        * fileapi/BlobBuilder.cpp:
> -        (WebCore::addTwoCStrings):
> -        (WebCore::BlobBuilder::append):
This part of the diff looks bogus.

[Zhenyao Mo]

Will revise and fix this change log issue once the newer version of ANGLE lands.  Cancel the review request for now.

[Zhenyao Mo]

Created attachment 68767 [details]
revised patch

So I didn't use AngleWebKitBridge because the Angle under webkit is out-of-sync again.  Besides, AngleWebKitBridge isn't very efficient and I feel optimizing it should be a separate patch.

Per discussion with alokp@google, we don't call ShFinalize per canvas exit, but per process exit, so I will add it to renderer exit code in a chromium patch.  This patch alone should work because the angle resource will be released anyway when the renderer process exits.

[Kenneth Russell]

Comment on attachment 68767 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68767&action=review

This mostly looks good; I'm marking it r- because of a few relatively minor issues. Please correct me if I'm wrong about the need for the null checks, but I'm quite sure strlen(NULL) crashes rather than returning 0.

The introduction of an ANGLE dependency on WebCore means that API changes to the shader validator are going to be fairly difficult to handle. I'm going to notify people about this.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:689
> +    int compileStatus;
> +    glGetShaderiv(shader, GL_COMPILE_STATUS, &compileStatus);
> +    // ASSERT that ANGLE generated GLSL will be accepted by OpenGL
> +    ASSERT(compileStatus == GL_TRUE);

It would be safer to handle the case that the OpenGL driver rejects the shader for some reason. While it's unlikely, even though we're configuring the ANGLE shader validator with the OpenGL driver's parameters like the maximum number of uniforms, the OpenGL driver might count things differently than ANGLE and therefore fail to compile the shader because it exceeds resource constraints. (Although it's more likely that the OpenGL driver will eliminate unused uniforms or varyings and therefore use fewer resources than ANGLE counts.) I think this block is OK in debug mode but the whole thing should be in an #ifndef NDEBUG.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:958
> +            *value = static_cast<int>(entry.isValid);

Per above, I think this should delegate to glGetShaderiv if entry.isValid.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:962
> +                *value = strlen(entry.log);

Guard against the case that entry.log is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:969
> +            *value = strlen(entry.source);

Guard against the case that entry.source is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:987
> +            WebString res = WebString::fromUTF8(entry.log, strlen(entry.log));

Guard against the case that entry.log is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1014
> +        WebString res = WebString::fromUTF8(entry.source, strlen(entry.source));

Guard against the case that entry.source is NULL.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1152
> +        glShaderSource(shader, 1, &string, &length);

The else clause should be an error condition, and synthesize either INVALID_VALUE or INVALID_OPERATION (probably the latter).

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1358
> +    getIntegerv(0x8DFB, // MAX_VERTEX_UNIFORM_VECTORS

Please define "const int MAX_VERTEX_UNIFORM_VECTORS=0x8DFB" at the top of the file and use it here and in getIntegerv.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1361
> +    getIntegerv(0x8DFC, // MAX_VARYING_VECTORS

Same here.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1370
> +    getIntegerv(0x8DFD, // MAX_FRAGMENT_UNIFORM_VECTORS

Same here.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.h:330
> +    class ShaderSourceEntry {

If all members are supposed to be public then this should be a struct, not a class. Note that a struct can have constructors and destructors.

[Alok Priyadarshi]

Comment on attachment 68767 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68767&action=review

> WebKit/chromium/WebKit.gyp:79
> +                '<(chromium_src_dir)/third_party/angle/src/build_angle.gyp:translator_common',

It should depend on translator_glsl or translator_hlsl. translator_common is abstract with no backend.

> WebKit/chromium/WebKit.gyp:88
> +                '<(chromium_src_dir)/third_party/angle/include/GLSLANG',

I think it should just be "<(chromium_src_dir)/third_party/angle/include". ShaderLang.h should be included as GLSLANG/ShaderLang.h.

>> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:689
>> +    ASSERT(compileStatus == GL_TRUE);
> 
> It would be safer to handle the case that the OpenGL driver rejects the shader for some reason. While it's unlikely, even though we're configuring the ANGLE shader validator with the OpenGL driver's parameters like the maximum number of uniforms, the OpenGL driver might count things differently than ANGLE and therefore fail to compile the shader because it exceeds resource constraints. (Although it's more likely that the OpenGL driver will eliminate unused uniforms or varyings and therefore use fewer resources than ANGLE counts.) I think this block is OK in debug mode but the whole thing should be in an #ifndef NDEBUG.

I agree with Ken. ASSERT is too string here. Logging the info that ANGLE generated GLSL failed to compile would useful though.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1351
> +    if (!ShInitialize())

Even though ShInitialize is a no-op if called multiple times, it will be better to call it only once.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1382
> +    if (m_fragmentCompiler) {

Is there something equivalent to AtExitManager in webkit? You could register ShFinalize in that case.

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1425
> +    ShGetInfo(compiler, SH_OBJECT_CODE_LENGTH, &length);

I am just curious what you do with the translated source. Does webkit use desktop OpenGL? I thought it just marshaled all calls to command buffer which exposes a GLES 2.0 interface.

[Zhenyao Mo]

Comment on attachment 68767 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68767&action=review

The rest I will revise.

>> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1351
>> +    if (!ShInitialize())
> 
> Even though ShInitialize is a no-op if called multiple times, it will be better to call it only once.

In order to call it only once, we need to either do it on the chromium side (in the initializing of the process), or keep an static flag to track whether it's been initialized.  Since it's no-op, I prefer to keep the ShInitialize here for safeguard purpose.

>> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1382
>> +    if (m_fragmentCompiler) {
> 
> Is there something equivalent to AtExitManager in webkit? You could register ShFinalize in that case.

The process class is in chromium, and I will register ShFinalize there in another chromium patch.

>> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1425
>> +    ShGetInfo(compiler, SH_OBJECT_CODE_LENGTH, &length);
> 
> I am just curious what you do with the translated source. Does webkit use desktop OpenGL? I thought it just marshaled all calls to command buffer which exposes a GLES 2.0 interface.

No, WebGraphicsContext3DDefaultImpl is used for in-process-webgl and it's an alternative to command buffer; it is a wrapper around desktop gl.

[Zhenyao Mo]

Comment on attachment 68767 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68767&action=review

>> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:1152
>> +        glShaderSource(shader, 1, &string, &length);
> 
> The else clause should be an error condition, and synthesize either INVALID_VALUE or INVALID_OPERATION (probably the latter).

It's hard to decide whether to generate INVALID_VALUE or INVALID_OPERATION here; also, in the rare case where deleteShader is called when it's still attached to a program, the ShaderSourceEntry will be removed, yet glShaderSource call should still succeed.  For these two reasons, I pass down the call to gl driver.

[Zhenyao Mo]

Created attachment 68955 [details]
revised patch: responding to kbr and alokp reviews

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:48:  IMPLEMENTATION_COLOR_READ_FORMAT is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:49:  IMPLEMENTATION_COLOR_READ_TYPE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:50:  MAX_VERTEX_UNIFORM_VECTORS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:51:  MAX_VARYING_VECTORS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:52:  MAX_FRAGMENT_UNIFORM_VECTORS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 6 files


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

[Kenneth Russell]

Comment on attachment 68955 [details]
revised patch: responding to kbr and alokp reviews

View in context: https://bugs.webkit.org/attachment.cgi?id=68955&action=review

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:52
> +const unsigned int MAX_FRAGMENT_UNIFORM_VECTORS = 0x8DFD;

To make the style bot happy I guess we need to name these ImplementationColorReadFormat, etc. You can add a comment at the end of the line indicating it's the same as GL_IMPLEMENTATION_COLOR_READ_FORMAT.

[Zhenyao Mo]

Created attachment 68960 [details]
revised patch: style fix

It's really important to make the style bot happy.

[Kenneth Russell]

Comment on attachment 68960 [details]
revised patch: style fix

View in context: https://bugs.webkit.org/attachment.cgi?id=68960&action=review

> WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:48
> +enum GLES2EnumType {

This could just be an unnamed enum type.

[Zhenyao Mo]

Committed r68434: <http://trac.webkit.org/changeset/68434>