Bug 70989 – Attribute and Uniform variable names need translation in shader

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

The WebGL layout tests introduced in Bug 70981 are currently failing on Mac. It looks like an upgrade of the copy of ANGLE in WebKit, along with the use of the long identifier name mapping flag, will fix these failures. They're being skipped for the moment.

[Joseph Pecoraro]

<rdar://problem/10516646>

[Dean Jackson]

I was hoping just passing SH_MAP_LONG_VARIABLE_NAMES to the ANGLE compile step would be enough, but it seems Ken is right - we'd need to update ANGLE. Unfortunately that looks like a bit of a pain.

[Dean Jackson]

Adding just this is enough to avoid the ASSERT on debug builds:

bool validateSuccess = ShCompile(compiler, shaderSourceStrings, 1, SH_OBJECT_CODE | SH_MAP_LONG_VARIABLE_NAMES | SH_ATTRIBUTES_UNIFORMS);

But then the test fails because it can't find the attribute.

[Dean Jackson]

I'm going to submit a patch to fix the ANGLE translation so the shaders will compile on Mac. The tests will still fail, but I'll check in expected results for Mac.

Meanwhile, the real fix is: https://bugs.webkit.org/show_bug.cgi?id=81717

[Dean Jackson]

Created attachment 132925 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132925 [details]
Patch

Attachment 132925 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12071067

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/uniform-location-length-limits.html
fast/canvas/webgl/uniform-location-length-limits.html
platform/chromium/virtual/gpu/fast/canvas/webgl/attrib-location-length-limits.html
fast/canvas/webgl/attrib-location-length-limits.html

[Zhenyao Mo]

Comment on attachment 132925 [details]
Patch

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

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:91
> +    bool validateSuccess = ShCompile(compiler, shaderSourceStrings, 1, SH_OBJECT_CODE | SH_MAP_LONG_VARIABLE_NAMES | SH_ATTRIBUTES_UNIFORMS);

By turning this one, you need to manage the mapping on webkit side, say, user queries a uniform location using the original long name, you need to map it to short name before sending the gl calls to underlying driver.  Otherwise you won't get the location.

[Dean Jackson]

(In reply to comment #7)
> (From update of attachment 132925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132925&action=review
> 
> > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:91
> > +    bool validateSuccess = ShCompile(compiler, shaderSourceStrings, 1, SH_OBJECT_CODE | SH_MAP_LONG_VARIABLE_NAMES | SH_ATTRIBUTES_UNIFORMS);
> 
> By turning this one, you need to manage the mapping on webkit side, say, user queries a uniform location using the original long name, you need to map it to short name before sending the gl calls to underlying driver.  Otherwise you won't get the location.

Ah, thanks for explaining. I was hoping to leave that to a later patch. For now I just wanted to avoid the crasher.

I'll add the management. Could you point me to where Chromium does it?

[Zhenyao Mo]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 132925 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=132925&action=review
> > 
> > > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:91
> > > +    bool validateSuccess = ShCompile(compiler, shaderSourceStrings, 1, SH_OBJECT_CODE | SH_MAP_LONG_VARIABLE_NAMES | SH_ATTRIBUTES_UNIFORMS);
> > 
> > By turning this one, you need to manage the mapping on webkit side, say, user queries a uniform location using the original long name, you need to map it to short name before sending the gl calls to underlying driver.  Otherwise you won't get the location.
> 
> Ah, thanks for explaining. I was hoping to leave that to a later patch. For now I just wanted to avoid the crasher.
> 
> I'll add the management. Could you point me to where Chromium does it?

goto cs.chromium.org and search for shader_translator.cc and program_manager.cc.

For program_manager.cc, look at Update().

[Dean Jackson]

Created attachment 166758 [details]
Patch

[Dean Jackson]

Adding Alex and Max because this touches CSS custom filter code.

[Dean Jackson]

*** Bug 94034 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 166758 [details]
Patch

Attachment 166758 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14135360

[Dean Jackson]

Comment on attachment 166758 [details]
Patch

Obviously fails on chromium.

[Dean Jackson]

Created attachment 166762 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166762 [details]
Patch

Attachment 166762 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14137334

[Peter Beverloo (cr-android ews)]

Comment on attachment 166762 [details]
Patch

Attachment 166762 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14132389

[Dean Jackson]

Created attachment 166787 [details]
Patch

[Dean Jackson]

Trying to get Chromium to compile. I'm not sure what will happen to Chrome since I've changed the name of a method in Extensions3D.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 166787 [details]
Patch

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

> Source/Platform/chromium/public/WebGraphicsContext3D.h:408
> +    virtual WebString translateShaderANGLE(WebGLId shader) { return WebString(); }

this change will break things since we have implementations of this code in the chromium repo.  To change any public Chromium API (see the directory name - it has chromium/public in it) you generally speaking need to do a 2-sided roll, one to implement the new name of the function in implementations of WebGraphicsContext3D and then one on the WebKit side to change the exposed interface name.

For this I'd recommend simply calling getTranslatedShaderSourceANGLE() in Extensions3DChromium.  I don't think there's an especially compelling reason to rename this.  Honestly, as far as WebKit style goes I think the "get" is warranted since this is definitely *not* a simple getter.

[Dean Jackson]

Created attachment 166789 [details]
Patch

[Dean Jackson]

(In reply to comment #21)

> Honestly, as far as WebKit style goes I think the "get" is warranted since this is definitely *not* a simple getter.

You're right. I'm keeping the old name.

[Max Vujovic]

Nice patch! Just a couple of small comments.

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

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:33
> +using namespace std;

Nit: I don't think you need "using namespace std;", since you use "std::min" below.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:44
> +static bool getVariableInfo(const ShHandle compiler, ShShaderInfo variableType, Vector<ANGLEShaderSymbol>& symbols)

We seem to be using the names "variable" and "symbol" interchangeably here. Perhaps we should pick one?

e.g.
(1) getVariableInfo(..., Vector<ANGLEShaderVariable>& variables);
(2) getSymbolInfo(..., Vector<ANGLEShaderSymbol>& symbols);

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:61
> +    if (numVariables < 0)

Good.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:65
> +    if (maxNameLength <= 1)

Good.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:69
> +    if (maxMappedNameLength <= 1)

Good.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:91
> +        if (nameLength <= 0)

Good.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:94
> +        symbol.mappedName = String::fromUTF8(mappedNameBuffer.get(), std::min(maxMappedNameLength, nameLength));

maxMappedNameLength counts the null terminator.
nameLength does not count the null terminator.

I believe String::fromUTF8 expects a length that does not include the null terminator. Then, this would be:
std::min(maxMappedNameLength - 1, nameLength)

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:75
> +    bool compileShaderSource(const char* shaderSource, ANGLEShaderType, String& translatedShaderSource, String& shaderValidationLog, Vector<ANGLEShaderSymbol>& symbols, int extraCompileOptions = 0);

This is a better interface. Nice :)

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:100
> +        if (it->symbolType != SHADER_SYMBOL_TYPE_UNIFORM)

In GLSL [1], samplers must be uniforms, so this check isn't required. If the sampler isn't a uniform, ANGLE should have generated a validation error and returned early. However, having this check come first is a little more efficient if the symbol is not a uniform, since we don't have to check the symbol against all the different sampler types in isSampler().

If you decide to keep the check, I think it would be better to push it into isSampler().

[1]: "[Samplers] can only be declared as function parameters or uniforms", Section 4.1.7: Samplers, http://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf

[Darin Adler]

Comment on attachment 166789 [details]
Patch

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

review- only because of my mappedName buffer overrun concern. Otherwise looks good, although I have various suggestions for improvements.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:44
> +static bool getVariableInfo(const ShHandle compiler, ShShaderInfo variableType, Vector<ANGLEShaderSymbol>& symbols)

The const in const ShHandle here has no practical effect and should be omitted.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:73
> +    OwnArrayPtr<char> nameBuffer = adoptArrayPtr(new char[maxNameLength]);
> +    OwnArrayPtr<char> mappedNameBuffer = adoptArrayPtr(new char[maxMappedNameLength]);

A higher performance idiom for this is:

    Vector<char, 256> nameBuffer(maxNameLength);

Then you use nameBuffer.data().

Where you replace 256 with a number that is likely to be large enough to hold most variable names but not so large that it makes the function use an inappropriate amount of stack. That technique avoids heap allocation/free.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:94
> +        symbol.name = String::fromUTF8(nameBuffer.get(), nameLength);
> +        symbol.mappedName = String::fromUTF8(mappedNameBuffer.get(), std::min(maxMappedNameLength, nameLength));

This code will put a symbol containing null strings into the vector if there are illegal UTF-8 sequences in either name. Can that happen? If so, what is the right way to handle that? Leave out the symbol entirely? Are these strings really UTF-8? I’m concerned about translating strings from UTF-8 to UTF-16 and back if they aren’t actually UTF-8 strings. If they are ASCII then it’s more efficient to treat them as Latin-1 than UTF-8; they’ll stay 8-bit strings and not be encoded and decoded. If the characters can be arbitrary bytes that might not be UTF-8 sequences, then we can either treat them as Latin-1, which guarantees an accurate round trip and has no such concept as an illegal sequence, or alternatively we might want to use something other than String to store them.

I am puzzled by the std::min(maxMappedNameLength, nameLength) expression. What is the guarantee that the mapped name did not overrun the buffer, if our code is the code that’s clipping the mapped name's length to the maximum? If the mapped name can come out of the shader functions with a longer length, then mappedNameBuffer needs to be larger. This is non-obvious enough that it might require a comment.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:974
> +        VariableInfo(GC3Denum type, int size, String& mappedName)

Should be const String& rather than String&.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1000
> +        ShaderVariableMap attribMap;

Can we spell out the word “attribute” instead of using the abbreviation “attrib”?

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1003
> +            , isValid(0)

Should be false rather than 0.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1011
> +    String getMappedVariableName(Platform3DObject program, ShShaderInfo variableType, const String& name);

Normally we don’t name such functions with the word “get”. We reserve the word “get” for functions that have no return value and use out arguments instead. We’d typically just call this mappedVariableName.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:99
> +        // We're only checking uniforms

Comments should answer the question “why” rather than restate what the code does.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:174
> +    for (Vector<ANGLEShaderSymbol>::iterator it = variables.begin(); it != variables.end(); ++it) {

Normally we use indexing, rather than iterators, to iterate vectors.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:185
> +        switch (it->symbolType) {
> +        case SHADER_SYMBOL_TYPE_ATTRIBUTE:
> +            entry.attribMap.set(it->name, variableInfo);
> +            break;
> +        case SHADER_SYMBOL_TYPE_UNIFORM:
> +            entry.uniformMap.set(it->name, variableInfo);
> +            break;
> +        default:
> +            ASSERT_NOT_REACHED();
> +        }

I think we should have a ShaderSourceEntry member function that takes a symbol type and returns a reference to the appropriate map. Then we can get rid of this switch statement, and other ones like it in at least one other place. I’d call the member function variableMap, and then it would be like this:

    entry.variableMap(it->symbolType).set(it->name, variableInfo);

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:674
> +#if PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(QT) || PLATFORM(EFL) || PLATFORM(BLACKBERRY)

This #if looks bad. Is there some way to avoid this long list of platforms?

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:677
> +    String mappedName = String(name);

There is no need for the String() around the word “name” here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:688
> +        ShaderSourceEntry entry = result->second;

Would be more efficient to use a local variable of type const ShaderSourceEntry& here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:698
> +        if (variableType == SH_ACTIVE_ATTRIBUTES) {
> +            variableResult = entry.attribMap.find(name);
> +            if (variableResult == entry.attribMap.end())
> +                continue;
> +        } else {
> +            variableResult = entry.uniformMap.find(name);
> +            if (variableResult == entry.uniformMap.end())
> +                continue;
> +        }

It would be better factoring here to use something that points to either attribMap or uniformMap, that way you could have find call and check against continue also be part of the shared code. If you take my suggestion to add a member function, then you get this:

    const ShaderVariableMap& variableMap = result->second.variableMap(variableType);
    ShaderVariableMap::const_iterator variableEntry = variableMap.find(name);
    if (variableEntry != variableMap.end())
        return variableEntry->second.mappedName;

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:701
> +        mappedName = variableResult->second.mappedName;
> +        break;

I think using a return would be clearer here. Then we wouldn’t even need the mappedName local variable.

[Max Vujovic]

Comment on attachment 166789 [details]
Patch

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

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:158
> +    bool validateSuccess = ShCompile(compiler, shaderSourceStrings, 1, SH_OBJECT_CODE | SH_ATTRIBUTES_UNIFORMS | SH_MAP_LONG_VARIABLE_NAMES | extraCompileOptions);

Can we pass in SH_MAP_LONG_VARIABLE_NAMES in extraCompileOptions instead? In custom filters, we do a first ANGLE validation pass for shader rewriting. If we don't shorten the names in this first pass, we can reuse your mapping code during the second ANGLE pass in GC3D.

[Dean Jackson]

Thanks for the reviews everyone! New patch coming up.

[Dean Jackson]

(In reply to comment #24)

> Nice patch! Just a couple of small comments.

I've updated everything you suggested. Some comments.

> > +static bool getVariableInfo(const ShHandle compiler, ShShaderInfo variableType, Vector<ANGLEShaderSymbol>& symbols)
> 
> We seem to be using the names "variable" and "symbol" interchangeably here. Perhaps we should pick one?

I went with "symbol" everywhere.

> > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:94
> > +        symbol.mappedName = String::fromUTF8(mappedNameBuffer.get(), std::min(maxMappedNameLength, nameLength));
> 
> maxMappedNameLength counts the null terminator.
> nameLength does not count the null terminator.
> 
> I believe String::fromUTF8 expects a length that does not include the null terminator. Then, this would be:

It turns out I didn't even need this code. See my answer to Darin below.

> > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:100
> > +        if (it->symbolType != SHADER_SYMBOL_TYPE_UNIFORM)
> 
> In GLSL [1], samplers must be uniforms, so this check isn't required. If the sampler isn't a uniform, ANGLE should have generated a validation error and returned early. However, having this check come first is a little more efficient if the symbol is not a uniform, since we don't have to check the symbol against all the different sampler types in isSampler().
> 
> If you decide to keep the check, I think it would be better to push it into isSampler().

Done.

[Dean Jackson]

(In reply to comment #25)

> review- only because of my mappedName buffer overrun concern. Otherwise looks good, although I have various suggestions for improvements.

Addressed everything you suggested. Thanks for the detailed feedback. I have some comments below.


> > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:73
> > +    OwnArrayPtr<char> nameBuffer = adoptArrayPtr(new char[maxNameLength]);
> > +    OwnArrayPtr<char> mappedNameBuffer = adoptArrayPtr(new char[maxMappedNameLength]);
> 
> A higher performance idiom for this is:
> 
>     Vector<char, 256> nameBuffer(maxNameLength);
> 
> Then you use nameBuffer.data().
> 
> Where you replace 256 with a number that is likely to be large enough to hold most variable names but not so large that it makes the function use an inappropriate amount of stack. That technique avoids heap allocation/free.

Excellent. Symbols are not allowed to be over 256 characters otherwise validation would have failed before here, so this is perfect.

> > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:94
> > +        symbol.name = String::fromUTF8(nameBuffer.get(), nameLength);
> > +        symbol.mappedName = String::fromUTF8(mappedNameBuffer.get(), std::min(maxMappedNameLength, nameLength));
> 
> This code will put a symbol containing null strings into the vector if there are illegal UTF-8 sequences in either name. Can that happen? If so, what is the right way to handle that? Leave out the symbol entirely? Are these strings really UTF-8? I’m concerned about translating strings from UTF-8 to UTF-16 and back if they aren’t actually UTF-8 strings. If they are ASCII then it’s more efficient to treat them as Latin-1 than UTF-8; they’ll stay 8-bit strings and not be encoded and decoded. If the characters can be arbitrary bytes that might not be UTF-8 sequences, then we can either treat them as Latin-1, which guarantees an accurate round trip and has no such concept as an illegal sequence, or alternatively we might want to use something other than String to store them.
> 
> I am puzzled by the std::min(maxMappedNameLength, nameLength) expression. What is the guarantee that the mapped name did not overrun the buffer, if our code is the code that’s clipping the mapped name's length to the maximum? If the mapped name can come out of the shader functions with a longer length, then mappedNameBuffer needs to be larger. This is non-obvious enough that it might require a comment.

It turns out I was doing more than necessary (and it was wrong). The OpenGL ES Shading Language spec requires all content to be ASCII (actually a subset). WebGL *does* allow UTF-8, but only in comments. Therefore, no symbol that gets this far could be anything but ASCII. There are tests in the WebGL conformance suite exercising this.

Furthermore, the ShGetActive* methods produce null-terminated strings for nameBuffer and mappedNameBuffer. So I was able to replace the code above with just String(nameBuffer.data())

I've written a comment explaining this. I'd like to see a little more testing of this in the ANGLE project.

> > +        ShaderVariableMap attribMap;
> 
> Can we spell out the word “attribute” instead of using the abbreviation “attrib”?

OpenGL uses the term "attrib" for some reason in its API, but it looks like we often write it in full in WebKit. I've changed this to "attribute".

> I think we should have a ShaderSourceEntry member function that takes a symbol type and returns a reference to the appropriate map. Then we can get rid of this switch statement, and other ones like it in at least one other place. I’d call the member function variableMap, and then it would be like this:
> 
>     entry.variableMap(it->symbolType).set(it->name, variableInfo);

Done.

> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:674
> > +#if PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(QT) || PLATFORM(EFL) || PLATFORM(BLACKBERRY)
> 
> This #if looks bad. Is there some way to avoid this long list of platforms?

It was unnecessary. I believe that file is only compiled on those platforms.


> It would be better factoring here to use something that points to either attribMap or uniformMap, that way you could have find call and check against continue also be part of the shared code. If you take my suggestion to add a member function, then you get this:
> 
>     const ShaderVariableMap& variableMap = result->second.variableMap(variableType);
>     ShaderVariableMap::const_iterator variableEntry = variableMap.find(name);
>     if (variableEntry != variableMap.end())
>         return variableEntry->second.mappedName;

Done.

> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:701
> > +        mappedName = variableResult->second.mappedName;
> > +        break;
> 
> I think using a return would be clearer here. Then we wouldn’t even need the mappedName local variable.

Done, and yes.

Thanks again.

[Dean Jackson]

(In reply to comment #26)
> (From update of attachment 166789 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166789&action=review
> 
> > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:158
> > +    bool validateSuccess = ShCompile(compiler, shaderSourceStrings, 1, SH_OBJECT_CODE | SH_ATTRIBUTES_UNIFORMS | SH_MAP_LONG_VARIABLE_NAMES | extraCompileOptions);
> 
> Can we pass in SH_MAP_LONG_VARIABLE_NAMES in extraCompileOptions instead? In custom filters, we do a first ANGLE validation pass for shader rewriting. If we don't shorten the names in this first pass, we can reuse your mapping code during the second ANGLE pass in GC3D.

I tried to address this by having the ANGLEWebKitBridge know if it is being used for GC3D or CustomFilters at construction time, but that didn't quite work out. Instead I'm now passing the extra options in from the GC3D use (via Extensions3D).

[Dean Jackson]

Created attachment 167012 [details]
Patch

[Build Bot]

Comment on attachment 167012 [details]
Patch

Attachment 167012 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14123926

New failing tests:
css3/filters/custom/custom-filter-no-element-texture-access.html

[Max Vujovic]

Comment on attachment 167012 [details]
Patch

Thanks for the updates!

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

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:59
> +        return symbolType == SHADER_SYMBOL_TYPE_UNIFORM

I think you want:
symbolType == SHADER_SYMBOL_TYPE_UNIFORM && (dataType == SH_SAMPLER_2D || dataType == ...)

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:89
> +    bool vertexShaderValid = validator->compileShaderSource(originalVertexShader.utf8().data(), SHADER_TYPE_VERTEX, m_validatedVertexShader, vertexShaderLog, symbols);

The bot failure is because we're not passing in SH_ATTRIBUTES_UNIFORMS anymore, which causes the ANGLE symbol query to fail. I think it makes sense to always use this flag in compileShaderSource since that method returns a vector of symbols now. On the ANGLE side, all this flag does is walk the shader's intermediate parse tree, collect all the attributes and uniforms in a vector, and make them available via the ANGLE API. It doesn't affect the validation policy in any way, so it should be fine in all usages of compileShaderSource.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:163
> +    int extraCompileOptions = SH_ATTRIBUTES_UNIFORMS | SH_MAP_LONG_VARIABLE_NAMES;

If we push SH_ATTRIBUTES_UNIFORMS back into compileShaderSource, we can remove it here.

[WebKit Review Bot]

Comment on attachment 167012 [details]
Patch

Attachment 167012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14131895

New failing tests:
css3/filters/custom/custom-filter-no-element-texture-access.html

[Dean Jackson]

Comment on attachment 167012 [details]
Patch

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

>> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:59
>> +        return symbolType == SHADER_SYMBOL_TYPE_UNIFORM
> 
> I think you want:
> symbolType == SHADER_SYMBOL_TYPE_UNIFORM && (dataType == SH_SAMPLER_2D || dataType == ...)

Duh! You're right :(

>> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:89
>> +    bool vertexShaderValid = validator->compileShaderSource(originalVertexShader.utf8().data(), SHADER_TYPE_VERTEX, m_validatedVertexShader, vertexShaderLog, symbols);
> 
> The bot failure is because we're not passing in SH_ATTRIBUTES_UNIFORMS anymore, which causes the ANGLE symbol query to fail. I think it makes sense to always use this flag in compileShaderSource since that method returns a vector of symbols now. On the ANGLE side, all this flag does is walk the shader's intermediate parse tree, collect all the attributes and uniforms in a vector, and make them available via the ANGLE API. It doesn't affect the validation policy in any way, so it should be fine in all usages of compileShaderSource.

Indeed. I've made this change.

[Dean Jackson]

Created attachment 167068 [details]
Patch

[Dean Jackson]

Committed r130417: <http://trac.webkit.org/changeset/130417>