Changeset 221831 in webkit


Ignore:
Timestamp:
Sep 9, 2017 3:54:13 PM (7 years ago)
Author:
dino@apple.com
Message:

gl.detachShader breaks shader program
​https://bugs.webkit.org/show_bug.cgi?id=137689
<rdar://problem/34025056>

Reviewed by Sam Weinig.

Source/WebCore:

It should be possible to compile shaders, attach them to a program,
link the program, detach the shaders, delete the shaders, and then
ask for the uniform and attribute locations. That is, once you've
linked, the shaders can be thrown away.

We were using the attached shaders to look up uniform locations, so
we now keep around a separate map that remembers what shaders were
attached when the program links.

This fixes the bug, but the whole area is still a bit messy. For one,
we're keeping around all the shader information even after it is
no longer used.
See ​https://bugs.webkit.org/show_bug.cgi?id=98204

Test: fast/canvas/webgl/detachShader-before-accessing-uniform.html

  • platform/graphics/GraphicsContext3D.h: Add another map to remember

what shaders were used when a program was linked.

  • platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:

(WebCore::GraphicsContext3D::mappedSymbolInShaderSourceMap): New helper
to look up a name in our source maps.
(WebCore::GraphicsContext3D::mappedSymbolName): Use the helper, and look
at linked shaders if there are no attached shaders.
(WebCore::GraphicsContext3D::originalSymbolInShaderSourceMap): Does the
reverse of the above.
(WebCore::GraphicsContext3D::originalSymbolName):
(WebCore::GraphicsContext3D::linkProgram): Add to the new map.
(WebCore::GraphicsContext3D::deleteProgram): Delete the program from
our shader entries.

LayoutTests:

Test that detaches and deletes shaders after linking, but before
asking for uniform locations.

  • fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt: Added.
  • fast/canvas/webgl/detachShader-before-accessing-uniform.html: Added.
Location:
trunk
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r221830 r221831  
     12017-09-08  Dean Jackson  <dino@apple.com>
     2
     3        gl.detachShader breaks shader program
     4        https://bugs.webkit.org/show_bug.cgi?id=137689
     5        <rdar://problem/34025056>
     6
     7        Reviewed by Sam Weinig.
     8
     9        Test that detaches and deletes shaders after linking, but before
     10        asking for uniform locations.
     11
     12        * fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt: Added.
     13        * fast/canvas/webgl/detachShader-before-accessing-uniform.html: Added.
     14
    1152017-09-09  Per Arne Vollan  <pvollan@apple.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r221822 r221831  
     12017-09-08  Dean Jackson  <dino@apple.com>
     2
     3        gl.detachShader breaks shader program
     4        https://bugs.webkit.org/show_bug.cgi?id=137689
     5        <rdar://problem/34025056>
     6
     7        Reviewed by Sam Weinig.
     8
     9        It should be possible to compile shaders, attach them to a program,
     10        link the program, detach the shaders, delete the shaders, and then
     11        ask for the uniform and attribute locations. That is, once you've
     12        linked, the shaders can be thrown away.
     13
     14        We were using the attached shaders to look up uniform locations, so
     15        we now keep around a separate map that remembers what shaders were
     16        attached when the program links.
     17
     18        This fixes the bug, but the whole area is still a bit messy. For one,
     19        we're keeping around all the shader information even after it is
     20        no longer used.
     21        See https://bugs.webkit.org/show_bug.cgi?id=98204
     22
     23        Test: fast/canvas/webgl/detachShader-before-accessing-uniform.html
     24
     25        * platform/graphics/GraphicsContext3D.h: Add another map to remember
     26        what shaders were used when a program was linked.
     27        * platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:
     28        (WebCore::GraphicsContext3D::mappedSymbolInShaderSourceMap): New helper
     29        to look up a name in our source maps.
     30        (WebCore::GraphicsContext3D::mappedSymbolName): Use the helper, and look
     31        at linked shaders if there are no attached shaders.
     32        (WebCore::GraphicsContext3D::originalSymbolInShaderSourceMap): Does the
     33        reverse of the above.
     34        (WebCore::GraphicsContext3D::originalSymbolName):
     35        (WebCore::GraphicsContext3D::linkProgram): Add to the new map.
     36        (WebCore::GraphicsContext3D::deleteProgram): Delete the program from
     37        our shader entries.
     38
    1392017-09-09  Mark Lam  <mark.lam@apple.com>
    240

  • trunk/Source/WebCore/platform/graphics/GraphicsContext3D.h

    r221762 r221831  
    13491349    };
    13501350
     1351    // FIXME: Shaders are never removed from this map, even if they and their program are deleted.
     1352    // This is bad, and it also relies on the fact we never reuse Platform3DObject numbers.
    13511353    typedef HashMap<Platform3DObject, ShaderSourceEntry> ShaderSourceMap;
    13521354    ShaderSourceMap m_shaderSourceMap;
     1355
     1356    typedef HashMap<Platform3DObject, std::pair<Platform3DObject, Platform3DObject>> LinkedShaderMap;
     1357    LinkedShaderMap m_linkedShaderMap;
    13531358
    13541359    struct ActiveShaderSymbolCounts {
    … …  
    13781383    String mappedSymbolName(Platform3DObject shaders[2], size_t count, const String& name);
    13791384    String originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
     1385    std::optional<String> mappedSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType, const String& name);
     1386    std::optional<String> originalSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType, const String& name);
    13801387
    13811388    std::unique_ptr<ShaderNameHash> nameHashMapForShaders;

  • trunk/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp

    r221762 r221831  
    966966}
    967967
     968std::optional<String> GraphicsContext3D::mappedSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType symbolType, const String& name)
     969{
     970    auto result = m_shaderSourceMap.find(shader);
     971    if (result == m_shaderSourceMap.end())
     972        return std::nullopt;
     973
     974    const auto& symbolMap = result->value.symbolMap(symbolType);
     975    auto symbolEntry = symbolMap.find(name);
     976    if (symbolEntry == symbolMap.end())
     977        return std::nullopt;
     978
     979    auto& mappedName = symbolEntry->value.mappedName;
     980    return String(mappedName.c_str(), mappedName.length());
     981}
     982
    968983String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
    969984{
    … …  
    973988
    974989    for (GC3Dsizei i = 0; i < count; ++i) {
    975         ShaderSourceMap::iterator result = m_shaderSourceMap.find(shaders[i]);
    976         if (result == m_shaderSourceMap.end())
    977             continue;
    978 
    979         const ShaderSymbolMap& symbolMap = result->value.symbolMap(symbolType);
    980         ShaderSymbolMap::const_iterator symbolEntry = symbolMap.find(name);
    981         if (symbolEntry != symbolMap.end()) {
    982             const std::string& mappedName = symbolEntry->value.mappedName;
    983             return String(mappedName.c_str(), mappedName.length());
    984         }
     990        auto mappedName = mappedSymbolInShaderSourceMap(shaders[i], symbolType, name);
     991        if (mappedName)
     992            return mappedName.value();
     993    }
     994
     995    // We might have detached or deleted the shaders after linking.
     996    auto result = m_linkedShaderMap.find(program);
     997    if (result != m_linkedShaderMap.end()) {
     998        auto linkedShaders = result->value;
     999        auto mappedName = mappedSymbolInShaderSourceMap(linkedShaders.first, symbolType, name);
     1000        if (mappedName)
     1001            return mappedName.value();
     1002        mappedName = mappedSymbolInShaderSourceMap(linkedShaders.second, symbolType, name);
     1003        if (mappedName)
     1004            return mappedName.value();
    9851005    }
    9861006
    … …  
    9921012        setCurrentNameHashMapForShader(nameHashMapForShaders.get());
    9931013
    994         String generatedName = generateHashedName(name);
     1014        auto generatedName = generateHashedName(name);
    9951015
    9961016        setCurrentNameHashMapForShader(nullptr);
    … …  
    10031023    return name;
    10041024}
    1005    
     1025
     1026std::optional<String> GraphicsContext3D::originalSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType symbolType, const String& name)
     1027{
     1028    auto result = m_shaderSourceMap.find(shader);
     1029    if (result == m_shaderSourceMap.end())
     1030        return std::nullopt;
     1031
     1032    const auto& symbolMap = result->value.symbolMap(symbolType);
     1033    for (const auto& symbolEntry : symbolMap) {
     1034        if (name == symbolEntry.value.mappedName.c_str())
     1035            return symbolEntry.key;
     1036    }
     1037    return std::nullopt;
     1038}
     1039
    10061040String GraphicsContext3D::originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
    10071041{
    … …  
    10111045   
    10121046    for (GC3Dsizei i = 0; i < count; ++i) {
    1013         ShaderSourceMap::iterator result = m_shaderSourceMap.find(shaders[i]);
    1014         if (result == m_shaderSourceMap.end())
    1015             continue;
    1016        
    1017         const ShaderSymbolMap& symbolMap = result->value.symbolMap(symbolType);
    1018         for (const auto& symbolEntry : symbolMap) {
    1019             if (name == symbolEntry.value.mappedName.c_str())
    1020                 return symbolEntry.key;
    1021         }
     1047        auto originalName = originalSymbolInShaderSourceMap(shaders[i], symbolType, name);
     1048        if (originalName)
     1049            return originalName.value();
     1050    }
     1051
     1052    // We might have detached or deleted the shaders after linking.
     1053    auto result = m_linkedShaderMap.find(program);
     1054    if (result != m_linkedShaderMap.end()) {
     1055        auto linkedShaders = result->value;
     1056        auto originalName = originalSymbolInShaderSourceMap(linkedShaders.first, symbolType, name);
     1057        if (originalName)
     1058            return originalName.value();
     1059        originalName = originalSymbolInShaderSourceMap(linkedShaders.second, symbolType, name);
     1060        if (originalName)
     1061            return originalName.value();
    10221062    }
    10231063
    … …  
    11941234    ASSERT(program);
    11951235    makeContextCurrent();
     1236
     1237    GC3Dsizei count = 0;
     1238    Platform3DObject shaders[2] = { };
     1239    getAttachedShaders(program, 2, &count, shaders);
     1240
     1241    if (count == 2)
     1242        m_linkedShaderMap.set(program, std::make_pair(shaders[0], shaders[1]));
     1243
    11961244    ::glLinkProgram(program);
    11971245}
    … …  
    19051953{
    19061954    makeContextCurrent();
     1955    m_shaderProgramSymbolCountMap.remove(program);
    19071956    glDeleteProgram(program);
    19081957}
Note: See TracChangeset for help on using the changeset viewer.