- Revision
- 223356
- Author
- carlo...@webkit.org
- Date
- 2017-10-16 02:40:36 -0700 (Mon, 16 Oct 2017)
Log Message
Merge r221831 - 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.
Modified Paths
Added Paths
Diff
Modified: releases/WebKitGTK/webkit-2.18/LayoutTests/ChangeLog (223355 => 223356)
--- releases/WebKitGTK/webkit-2.18/LayoutTests/ChangeLog 2017-10-16 09:34:29 UTC (rev 223355)
+++ releases/WebKitGTK/webkit-2.18/LayoutTests/ChangeLog 2017-10-16 09:40:36 UTC (rev 223356)
@@ -1,3 +1,17 @@
+2017-09-08 Dean Jackson <d...@apple.com>
+
+ gl.detachShader breaks shader program
+ https://bugs.webkit.org/show_bug.cgi?id=137689
+ <rdar://problem/34025056>
+
+ Reviewed by Sam Weinig.
+
+ 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.
+
2017-09-06 Manuel Rego Casasnovas <r...@igalia.com>
[css-grid] grid shorthand should not reset the gutter properties
Added: releases/WebKitGTK/webkit-2.18/LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt (0 => 223356)
--- releases/WebKitGTK/webkit-2.18/LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt (rev 0)
+++ releases/WebKitGTK/webkit-2.18/LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt 2017-10-16 09:40:36 UTC (rev 223356)
@@ -0,0 +1,7 @@
+Vertex Shader compiled.
+Fragment Shader compiled.
+Program linked. Detaching and deleting shaders.
+Color uniform location was identified.
+Position attribute location was identified.
+Drawn.
+
Added: releases/WebKitGTK/webkit-2.18/LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform.html (0 => 223356)
--- releases/WebKitGTK/webkit-2.18/LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform.html (rev 0)
+++ releases/WebKitGTK/webkit-2.18/LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform.html 2017-10-16 09:40:36 UTC (rev 223356)
@@ -0,0 +1,122 @@
+<style>
+canvas {
+ width: 200px;
+ height: 200px;
+}
+</style>
+</head>
+<script id="vertexShaderSource" type="text/glsl">
+attribute vec4 position;
+void main() {
+ gl_Position = position;
+}
+</script>
+<script id="fragmentShaderSource" type="text/glsl">
+precision mediump float;
+uniform vec4 color;
+void main() {
+ gl_FragColor = color;
+}
+</script>
+<script>
+if (window.testRunner)
+ testRunner.dumpAsText();
+
+function output(msg) {
+ let d = document.querySelector("div");
+ d.innerHTML += `${msg}<br>`;
+}
+
+function drawTriangle() {
+
+ let canvas = document.querySelector("canvas");
+ canvas.width = 200;
+ canvas.height = 200;
+ let gl = canvas.getContext("webgl");
+
+ let vertexShader = gl.createShader(gl.VERTEX_SHADER);
+ gl.shaderSource(vertexShader, document.getElementById("vertexShaderSource").textContent);
+ gl.compileShader(vertexShader);
+ if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) {
+ output("Vertex Shader failed to compile.");
+ console.log(gl.getShaderInfoLog(vertexShader));
+ return;
+ }
+ output("Vertex Shader compiled.");
+
+ let fragmentShader = gl.createShader(gl.FRAGMENT_SHADER);
+ gl.shaderSource(fragmentShader, document.getElementById("fragmentShaderSource").textContent);
+ gl.compileShader(fragmentShader);
+ if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) {
+ output("Fragment Shader failed to compile.");
+ console.log(gl.getShaderInfoLog(fragmentShader));
+ return;
+ }
+ output("Fragment Shader compiled.");
+
+ let program = gl.createProgram();
+ gl.attachShader(program, vertexShader);
+ gl.attachShader(program, fragmentShader);
+ gl.linkProgram(program);
+
+ if (!gl.getProgramParameter(program, gl.LINK_STATUS)) {
+ output("Unable to link shaders into program.");
+ return;
+ }
+ output("Program linked. Detaching and deleting shaders.");
+
+ gl.detachShader(program, vertexShader);
+ gl.detachShader(program, fragmentShader);
+ gl.deleteShader(vertexShader);
+ gl.deleteShader(fragmentShader);
+
+ gl.useProgram(program);
+
+ let colorUniform = gl.getUniformLocation(program, "color");
+ if (colorUniform)
+ output("Color uniform location was identified.");
+ else {
+ output("FAIL: Color uniform location was not found.");
+ return;
+ }
+
+ let positionAttribute = gl.getAttribLocation(program, "position");
+ if (positionAttribute >= 0)
+ output("Position attribute location was identified.");
+ else {
+ output("FAIL: Position attribute location was not found.");
+ return;
+ }
+
+ gl.enableVertexAttribArray(positionAttribute);
+
+ let vertices = new Float32Array([
+ -0.8, -0.3,
+ 0.7, -0.8,
+ 0.55, 0.75
+ ]);
+
+ let triangleBuffer = gl.createBuffer();
+
+ gl.bindBuffer(gl.ARRAY_BUFFER, triangleBuffer);
+ gl.bufferData(gl.ARRAY_BUFFER, vertices, gl.STATIC_DRAW);
+
+ gl.clearColor(0, 0, 0, 1);
+ gl.clear(gl.COLOR_BUFFER_BIT);
+
+ let now = Date.now();
+ gl.uniform4fv(colorUniform, [1.0, 0.0, 0.0, 1.0]);
+
+ gl.bindBuffer(gl.ARRAY_BUFFER, triangleBuffer);
+ gl.vertexAttribPointer(positionAttribute, 2, gl.FLOAT, false, 0, 0);
+
+ gl.drawArrays(gl.TRIANGLES, 0, 3);
+ output("Drawn.");
+}
+
+window.addEventListener("load", drawTriangle, false);
+</script>
+<body>
+ <canvas></canvas>
+ <div></div>
+</body>
\ No newline at end of file
Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/ChangeLog (223355 => 223356)
--- releases/WebKitGTK/webkit-2.18/Source/WebCore/ChangeLog 2017-10-16 09:34:29 UTC (rev 223355)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/ChangeLog 2017-10-16 09:40:36 UTC (rev 223356)
@@ -1,3 +1,41 @@
+2017-09-08 Dean Jackson <d...@apple.com>
+
+ gl.detachShader breaks shader program
+ https://bugs.webkit.org/show_bug.cgi?id=137689
+ <rdar://problem/34025056>
+
+ Reviewed by Sam Weinig.
+
+ 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.
+
2017-09-09 Zan Dobersek <zdober...@igalia.com>
[GStreamer] Missing GRefPtr adoptions in MediaPlayerPrivateGStreamerBase, PlaybackPipeline
Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/platform/graphics/GraphicsContext3D.h (223355 => 223356)
--- releases/WebKitGTK/webkit-2.18/Source/WebCore/platform/graphics/GraphicsContext3D.h 2017-10-16 09:34:29 UTC (rev 223355)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/platform/graphics/GraphicsContext3D.h 2017-10-16 09:40:36 UTC (rev 223356)
@@ -1340,9 +1340,14 @@
}
};
+ // FIXME: Shaders are never removed from this map, even if they and their program are deleted.
+ // This is bad, and it also relies on the fact we never reuse Platform3DObject numbers.
typedef HashMap<Platform3DObject, ShaderSourceEntry> ShaderSourceMap;
ShaderSourceMap m_shaderSourceMap;
+ typedef HashMap<Platform3DObject, std::pair<Platform3DObject, Platform3DObject>> LinkedShaderMap;
+ LinkedShaderMap m_linkedShaderMap;
+
struct ActiveShaderSymbolCounts {
Vector<GC3Dint> filteredToActualAttributeIndexMap;
Vector<GC3Dint> filteredToActualUniformIndexMap;
@@ -1369,6 +1374,8 @@
String mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
String mappedSymbolName(Platform3DObject shaders[2], size_t count, const String& name);
String originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
+ std::optional<String> mappedSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType, const String& name);
+ std::optional<String> originalSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType, const String& name);
std::unique_ptr<ShaderNameHash> nameHashMapForShaders;
Modified: releases/WebKitGTK/webkit-2.18/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp (223355 => 223356)
--- releases/WebKitGTK/webkit-2.18/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp 2017-10-16 09:34:29 UTC (rev 223355)
+++ releases/WebKitGTK/webkit-2.18/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp 2017-10-16 09:40:36 UTC (rev 223356)
@@ -922,6 +922,21 @@
return builder.toString();
}
+std::optional<String> GraphicsContext3D::mappedSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType symbolType, const String& name)
+{
+ auto result = m_shaderSourceMap.find(shader);
+ if (result == m_shaderSourceMap.end())
+ return std::nullopt;
+
+ const auto& symbolMap = result->value.symbolMap(symbolType);
+ auto symbolEntry = symbolMap.find(name);
+ if (symbolEntry == symbolMap.end())
+ return std::nullopt;
+
+ auto& mappedName = symbolEntry->value.mappedName;
+ return String(mappedName.c_str(), mappedName.length());
+}
+
String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
{
GC3Dsizei count = 0;
@@ -929,16 +944,21 @@
getAttachedShaders(program, 2, &count, shaders);
for (GC3Dsizei i = 0; i < count; ++i) {
- ShaderSourceMap::iterator result = m_shaderSourceMap.find(shaders[i]);
- if (result == m_shaderSourceMap.end())
- continue;
+ auto mappedName = mappedSymbolInShaderSourceMap(shaders[i], symbolType, name);
+ if (mappedName)
+ return mappedName.value();
+ }
- const ShaderSymbolMap& symbolMap = result->value.symbolMap(symbolType);
- ShaderSymbolMap::const_iterator symbolEntry = symbolMap.find(name);
- if (symbolEntry != symbolMap.end()) {
- const std::string& mappedName = symbolEntry->value.mappedName;
- return String(mappedName.c_str(), mappedName.length());
- }
+ // We might have detached or deleted the shaders after linking.
+ auto result = m_linkedShaderMap.find(program);
+ if (result != m_linkedShaderMap.end()) {
+ auto linkedShaders = result->value;
+ auto mappedName = mappedSymbolInShaderSourceMap(linkedShaders.first, symbolType, name);
+ if (mappedName)
+ return mappedName.value();
+ mappedName = mappedSymbolInShaderSourceMap(linkedShaders.second, symbolType, name);
+ if (mappedName)
+ return mappedName.value();
}
if (symbolType == SHADER_SYMBOL_TYPE_ATTRIBUTE && !name.isEmpty()) {
@@ -948,7 +968,7 @@
nameHashMapForShaders = std::make_unique<ShaderNameHash>();
setCurrentNameHashMapForShader(nameHashMapForShaders.get());
- String generatedName = generateHashedName(name);
+ auto generatedName = generateHashedName(name);
setCurrentNameHashMapForShader(nullptr);
@@ -959,7 +979,21 @@
return name;
}
-
+
+std::optional<String> GraphicsContext3D::originalSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType symbolType, const String& name)
+{
+ auto result = m_shaderSourceMap.find(shader);
+ if (result == m_shaderSourceMap.end())
+ return std::nullopt;
+
+ const auto& symbolMap = result->value.symbolMap(symbolType);
+ for (const auto& symbolEntry : symbolMap) {
+ if (name == symbolEntry.value.mappedName.c_str())
+ return symbolEntry.key;
+ }
+ return std::nullopt;
+}
+
String GraphicsContext3D::originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
{
GC3Dsizei count;
@@ -967,17 +1001,23 @@
getAttachedShaders(program, 2, &count, shaders);
for (GC3Dsizei i = 0; i < count; ++i) {
- ShaderSourceMap::iterator result = m_shaderSourceMap.find(shaders[i]);
- if (result == m_shaderSourceMap.end())
- continue;
-
- const ShaderSymbolMap& symbolMap = result->value.symbolMap(symbolType);
- for (const auto& symbolEntry : symbolMap) {
- if (name == symbolEntry.value.mappedName.c_str())
- return symbolEntry.key;
- }
+ auto originalName = originalSymbolInShaderSourceMap(shaders[i], symbolType, name);
+ if (originalName)
+ return originalName.value();
}
+ // We might have detached or deleted the shaders after linking.
+ auto result = m_linkedShaderMap.find(program);
+ if (result != m_linkedShaderMap.end()) {
+ auto linkedShaders = result->value;
+ auto originalName = originalSymbolInShaderSourceMap(linkedShaders.first, symbolType, name);
+ if (originalName)
+ return originalName.value();
+ originalName = originalSymbolInShaderSourceMap(linkedShaders.second, symbolType, name);
+ if (originalName)
+ return originalName.value();
+ }
+
if (symbolType == SHADER_SYMBOL_TYPE_ATTRIBUTE && !name.isEmpty()) {
// Attributes are a special case: they may be requested before any shaders have been compiled,
// and aren't even required to be used in any shader program.
@@ -1140,6 +1180,14 @@
{
ASSERT(program);
makeContextCurrent();
+
+ GC3Dsizei count = 0;
+ Platform3DObject shaders[2] = { };
+ getAttachedShaders(program, 2, &count, shaders);
+
+ if (count == 2)
+ m_linkedShaderMap.set(program, std::make_pair(shaders[0], shaders[1]));
+
::glLinkProgram(program);
}
@@ -1845,6 +1893,7 @@
void GraphicsContext3D::deleteProgram(Platform3DObject program)
{
makeContextCurrent();
+ m_shaderProgramSymbolCountMap.remove(program);
glDeleteProgram(program);
}
