Work around atan(y, x) bug on NVIDIA
atan(y, x) is not always returning expected results on NVIDIA OpenGL
drivers between versions 367 and 375. Work around this by emulating
atan(y, x) using the regular atan(x) function. A fix to the driver is
expected in a future release.
It is most convenient to implement the vector atan(y, x) functions by
using the scalar atan(y, x) function. Support for simple dependencies
between emulated functions is added to BuiltInFunctionEmulator. In the
current implementation one function is allowed to have at most one
other function as its dependency.
BUG=chromium:672380
TEST=angle_end2end_tests
Change-Id: I9eba8b0b7979c7c7eaed353b264932e41830beb1
Reviewed-on: https://chromium-review.googlesource.com/419016
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
Reviewed-by: Jamie Madill <jmadill@chromium.org>
diff --git a/src/compiler/translator/BuiltInFunctionEmulator.cpp b/src/compiler/translator/BuiltInFunctionEmulator.cpp
index 4b6f6e1..a3dd260 100644
--- a/src/compiler/translator/BuiltInFunctionEmulator.cpp
+++ b/src/compiler/translator/BuiltInFunctionEmulator.cpp
@@ -108,29 +108,50 @@
{
}
-void BuiltInFunctionEmulator::addEmulatedFunction(TOperator op,
- const TType *param,
- const char *emulatedFunctionDefinition)
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunction(
+ TOperator op,
+ const TType *param,
+ const char *emulatedFunctionDefinition)
{
- mEmulatedFunctions[FunctionId(op, param)] = std::string(emulatedFunctionDefinition);
+ FunctionId id(op, param);
+ mEmulatedFunctions[id] = std::string(emulatedFunctionDefinition);
+ return id;
}
-void BuiltInFunctionEmulator::addEmulatedFunction(TOperator op,
- const TType *param1,
- const TType *param2,
- const char *emulatedFunctionDefinition)
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunction(
+ TOperator op,
+ const TType *param1,
+ const TType *param2,
+ const char *emulatedFunctionDefinition)
{
- mEmulatedFunctions[FunctionId(op, param1, param2)] = std::string(emulatedFunctionDefinition);
+ FunctionId id(op, param1, param2);
+ mEmulatedFunctions[id] = std::string(emulatedFunctionDefinition);
+ return id;
}
-void BuiltInFunctionEmulator::addEmulatedFunction(TOperator op,
- const TType *param1,
- const TType *param2,
- const TType *param3,
- const char *emulatedFunctionDefinition)
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunctionWithDependency(
+ FunctionId dependency,
+ TOperator op,
+ const TType *param1,
+ const TType *param2,
+ const char *emulatedFunctionDefinition)
{
- mEmulatedFunctions[FunctionId(op, param1, param2, param3)] =
- std::string(emulatedFunctionDefinition);
+ FunctionId id(op, param1, param2);
+ mEmulatedFunctions[id] = std::string(emulatedFunctionDefinition);
+ mFunctionDependencies[id] = dependency;
+ return id;
+}
+
+BuiltInFunctionEmulator::FunctionId BuiltInFunctionEmulator::addEmulatedFunction(
+ TOperator op,
+ const TType *param1,
+ const TType *param2,
+ const TType *param3,
+ const char *emulatedFunctionDefinition)
+{
+ FunctionId id(op, param1, param2, param3);
+ mEmulatedFunctions[id] = std::string(emulatedFunctionDefinition);
+ return id;
}
bool BuiltInFunctionEmulator::IsOutputEmpty() const
@@ -175,6 +196,12 @@
if (mFunctions[i] == functionId)
return true;
}
+ // If the function depends on another, mark the dependency as called.
+ auto dependency = mFunctionDependencies.find(functionId);
+ if (dependency != mFunctionDependencies.end())
+ {
+ SetFunctionCalled((*dependency).second);
+ }
// Copy the functionId if it needs to be stored, to make sure that the TType pointers inside
// remain valid and constant.
mFunctions.push_back(functionId.getCopy());
@@ -197,6 +224,7 @@
void BuiltInFunctionEmulator::Cleanup()
{
mFunctions.clear();
+ mFunctionDependencies.clear();
}
// static
@@ -206,6 +234,14 @@
return "webgl_" + name.substr(0, name.length() - 1) + "_emu(";
}
+BuiltInFunctionEmulator::FunctionId::FunctionId()
+ : mOp(EOpNull),
+ mParam1(TCache::getType(EbtVoid)),
+ mParam2(TCache::getType(EbtVoid)),
+ mParam3(TCache::getType(EbtVoid))
+{
+}
+
BuiltInFunctionEmulator::FunctionId::FunctionId(TOperator op, const TType *param)
: mOp(op), mParam1(param), mParam2(TCache::getType(EbtVoid)), mParam3(TCache::getType(EbtVoid))
{
diff --git a/src/compiler/translator/BuiltInFunctionEmulator.h b/src/compiler/translator/BuiltInFunctionEmulator.h
index af9ed8b..137a2c5 100644
--- a/src/compiler/translator/BuiltInFunctionEmulator.h
+++ b/src/compiler/translator/BuiltInFunctionEmulator.h
@@ -35,40 +35,17 @@
// Output function emulation definition. This should be before any other shader source.
void OutputEmulatedFunctions(TInfoSinkBase &out) const;
- // Add functions that need to be emulated.
- void addEmulatedFunction(TOperator op,
- const TType *param,
- const char *emulatedFunctionDefinition);
- void addEmulatedFunction(TOperator op,
- const TType *param1,
- const TType *param2,
- const char *emulatedFunctionDefinition);
- void addEmulatedFunction(TOperator op,
- const TType *param1,
- const TType *param2,
- const TType *param3,
- const char *emulatedFunctionDefinition);
-
- private:
- class BuiltInFunctionEmulationMarker;
-
- // Records that a function is called by the shader and might need to be emulated. If the
- // function is not in mEmulatedFunctions, this becomes a no-op. Returns true if the function
- // call needs to be replaced with an emulated one.
- bool SetFunctionCalled(TOperator op, const TType ¶m);
- bool SetFunctionCalled(TOperator op, const TType ¶m1, const TType ¶m2);
- bool SetFunctionCalled(TOperator op,
- const TType ¶m1,
- const TType ¶m2,
- const TType ¶m3);
-
class FunctionId
{
public:
+ FunctionId();
FunctionId(TOperator op, const TType *param);
FunctionId(TOperator op, const TType *param1, const TType *param2);
FunctionId(TOperator op, const TType *param1, const TType *param2, const TType *param3);
+ FunctionId(const FunctionId &) = default;
+ FunctionId &operator=(const FunctionId &) = default;
+
bool operator==(const FunctionId &other) const;
bool operator<(const FunctionId &other) const;
@@ -85,11 +62,48 @@
const TType *mParam3;
};
+ // Add functions that need to be emulated.
+ FunctionId addEmulatedFunction(TOperator op,
+ const TType *param,
+ const char *emulatedFunctionDefinition);
+ FunctionId addEmulatedFunction(TOperator op,
+ const TType *param1,
+ const TType *param2,
+ const char *emulatedFunctionDefinition);
+ FunctionId addEmulatedFunction(TOperator op,
+ const TType *param1,
+ const TType *param2,
+ const TType *param3,
+ const char *emulatedFunctionDefinition);
+
+ FunctionId addEmulatedFunctionWithDependency(FunctionId dependency,
+ TOperator op,
+ const TType *param1,
+ const TType *param2,
+ const char *emulatedFunctionDefinition);
+
+ private:
+ class BuiltInFunctionEmulationMarker;
+
+ // Records that a function is called by the shader and might need to be emulated. If the
+ // function is not in mEmulatedFunctions, this becomes a no-op. Returns true if the function
+ // call needs to be replaced with an emulated one.
+ bool SetFunctionCalled(TOperator op, const TType ¶m);
+ bool SetFunctionCalled(TOperator op, const TType ¶m1, const TType ¶m2);
+ bool SetFunctionCalled(TOperator op,
+ const TType ¶m1,
+ const TType ¶m2,
+ const TType ¶m3);
+
bool SetFunctionCalled(const FunctionId &functionId);
// Map from function id to emulated function definition
std::map<FunctionId, std::string> mEmulatedFunctions;
+ // Map from dependent functions to their dependencies. This structure allows each function to
+ // have at most one dependency.
+ std::map<FunctionId, FunctionId> mFunctionDependencies;
+
// Called function ids
std::vector<FunctionId> mFunctions;
};
diff --git a/src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp b/src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp
index 99bc313..dada53b 100644
--- a/src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp
+++ b/src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp
@@ -75,6 +75,43 @@
"}\n");
}
+void InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator *emu)
+{
+ const TType *float1 = TCache::getType(EbtFloat);
+ auto floatFuncId = emu->addEmulatedFunction(
+ EOpAtan, float1, float1,
+ "webgl_emu_precision float webgl_atan_emu(webgl_emu_precision float y, webgl_emu_precision "
+ "float x)\n"
+ "{\n"
+ " if (x > 0.0) return atan(y / x);\n"
+ " else if (x < 0.0 && y >= 0.0) return atan(y / x) + 3.14159265;\n"
+ " else if (x < 0.0 && y < 0.0) return atan(y / x) - 3.14159265;\n"
+ " else return 1.57079632 * sign(y);\n"
+ "}\n");
+ for (int dim = 2; dim <= 4; ++dim)
+ {
+ const TType *floatVec = TCache::getType(EbtFloat, static_cast<unsigned char>(dim));
+ std::stringstream ss;
+ ss << "webgl_emu_precision vec" << dim << " webgl_atan_emu(webgl_emu_precision vec" << dim
+ << " y, webgl_emu_precision vec" << dim << " x)\n"
+ "{\n"
+ " return vec"
+ << dim << "(";
+ for (int i = 0; i < dim; ++i)
+ {
+ ss << "webgl_atan_emu(y[" << i << "], x[" << i << "])";
+ if (i < dim - 1)
+ {
+ ss << ", ";
+ }
+ }
+ ss << ");\n"
+ "}\n";
+ emu->addEmulatedFunctionWithDependency(floatFuncId, EOpAtan, floatVec, floatVec,
+ ss.str().c_str());
+ }
+}
+
// Emulate built-in functions missing from GLSL 1.30 and higher
void InitBuiltInFunctionEmulatorForGLSLMissingFunctions(BuiltInFunctionEmulator *emu,
sh::GLenum shaderType,
diff --git a/src/compiler/translator/BuiltInFunctionEmulatorGLSL.h b/src/compiler/translator/BuiltInFunctionEmulatorGLSL.h
index e4f10d4..e1b4779 100644
--- a/src/compiler/translator/BuiltInFunctionEmulatorGLSL.h
+++ b/src/compiler/translator/BuiltInFunctionEmulatorGLSL.h
@@ -24,6 +24,10 @@
//
void InitBuiltInIsnanFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator *emu,
int targetGLSLVersion);
+//
+// This works around atan(y, x) bug in NVIDIA drivers.
+//
+void InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(BuiltInFunctionEmulator *emu);
//
// This function is emulating built-in functions missing from GLSL 1.30 and higher.
diff --git a/src/compiler/translator/TranslatorESSL.cpp b/src/compiler/translator/TranslatorESSL.cpp
index 809dfe4..ca32621 100644
--- a/src/compiler/translator/TranslatorESSL.cpp
+++ b/src/compiler/translator/TranslatorESSL.cpp
@@ -6,6 +6,7 @@
#include "compiler/translator/TranslatorESSL.h"
+#include "compiler/translator/BuiltInFunctionEmulatorGLSL.h"
#include "compiler/translator/EmulatePrecision.h"
#include "compiler/translator/RecordConstantPrecision.h"
#include "compiler/translator/OutputESSL.h"
@@ -19,6 +20,15 @@
{
}
+void TranslatorESSL::initBuiltInFunctionEmulator(BuiltInFunctionEmulator *emu,
+ ShCompileOptions compileOptions)
+{
+ if (compileOptions & SH_EMULATE_ATAN2_FLOAT_FUNCTION)
+ {
+ InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(emu);
+ }
+}
+
void TranslatorESSL::translate(TIntermNode *root, ShCompileOptions compileOptions)
{
TInfoSinkBase &sink = getInfoSink().obj;
diff --git a/src/compiler/translator/TranslatorESSL.h b/src/compiler/translator/TranslatorESSL.h
index b7b46a6..75d487a 100644
--- a/src/compiler/translator/TranslatorESSL.h
+++ b/src/compiler/translator/TranslatorESSL.h
@@ -18,6 +18,9 @@
TranslatorESSL(sh::GLenum type, ShShaderSpec spec);
protected:
+ void initBuiltInFunctionEmulator(BuiltInFunctionEmulator *emu,
+ ShCompileOptions compileOptions) override;
+
void translate(TIntermNode *root, ShCompileOptions compileOptions) override;
bool shouldFlattenPragmaStdglInvariantAll() override;
diff --git a/src/compiler/translator/TranslatorGLSL.cpp b/src/compiler/translator/TranslatorGLSL.cpp
index 728080e..2f9089f 100644
--- a/src/compiler/translator/TranslatorGLSL.cpp
+++ b/src/compiler/translator/TranslatorGLSL.cpp
@@ -36,6 +36,11 @@
InitBuiltInIsnanFunctionEmulatorForGLSLWorkarounds(emu, getShaderVersion());
}
+ if (compileOptions & SH_EMULATE_ATAN2_FLOAT_FUNCTION)
+ {
+ InitBuiltInAtanFunctionEmulatorForGLSLWorkarounds(emu);
+ }
+
int targetGLSLVersion = ShaderOutputTypeToGLSLVersion(getOutputType());
InitBuiltInFunctionEmulatorForGLSLMissingFunctions(emu, getShaderType(), targetGLSLVersion);
}
diff --git a/src/libANGLE/renderer/gl/ShaderGL.cpp b/src/libANGLE/renderer/gl/ShaderGL.cpp
index ae9c6e8..84bc4d4 100644
--- a/src/libANGLE/renderer/gl/ShaderGL.cpp
+++ b/src/libANGLE/renderer/gl/ShaderGL.cpp
@@ -70,6 +70,11 @@
options |= SH_EMULATE_ISNAN_FLOAT_FUNCTION;
}
+ if (mWorkarounds.emulateAtan2Float)
+ {
+ options |= SH_EMULATE_ATAN2_FLOAT_FUNCTION;
+ }
+
if (mWorkarounds.useUnusedBlocksWithStandardOrSharedLayout)
{
options |= SH_USE_UNUSED_STANDARD_SHARED_BLOCKS;
diff --git a/src/libANGLE/renderer/gl/WorkaroundsGL.h b/src/libANGLE/renderer/gl/WorkaroundsGL.h
index ed17548..0a5c60d 100644
--- a/src/libANGLE/renderer/gl/WorkaroundsGL.h
+++ b/src/libANGLE/renderer/gl/WorkaroundsGL.h
@@ -28,7 +28,8 @@
useUnusedBlocksWithStandardOrSharedLayout(false),
dontRemoveInvariantForFragmentInput(false),
removeInvariantAndCentroidForESSL3(false),
- rewriteFloatUnaryMinusOperator(false)
+ rewriteFloatUnaryMinusOperator(false),
+ emulateAtan2Float(false)
{
}
@@ -126,6 +127,10 @@
// replace "-float".
// Tracking bug: http://crbug.com/308366
bool rewriteFloatUnaryMinusOperator;
+
+ // On NVIDIA drivers, atan(y, x) may return a wrong answer.
+ // Tracking bug: http://crbug.com/672380
+ bool emulateAtan2Float;
};
}
diff --git a/src/libANGLE/renderer/gl/renderergl_utils.cpp b/src/libANGLE/renderer/gl/renderergl_utils.cpp
index ddf13e9..aebae45 100644
--- a/src/libANGLE/renderer/gl/renderergl_utils.cpp
+++ b/src/libANGLE/renderer/gl/renderergl_utils.cpp
@@ -953,6 +953,10 @@
workarounds->removeInvariantAndCentroidForESSL3 =
functions->isAtMostGL(gl::Version(4, 1)) ||
(functions->standard == STANDARD_GL_DESKTOP && IsAMD(vendor));
+
+ // TODO(oetuaho): Make this specific to the affected driver versions. Versions that came after
+ // 364 are known to be affected, at least up to 375.
+ workarounds->emulateAtan2Float = IsNvidia(vendor);
}
}
diff --git a/src/tests/gl_tests/GLSLTest.cpp b/src/tests/gl_tests/GLSLTest.cpp
index 18622ef..7a835ef 100644
--- a/src/tests/gl_tests/GLSLTest.cpp
+++ b/src/tests/gl_tests/GLSLTest.cpp
@@ -2170,13 +2170,6 @@
// Test that -float calculation is correct.
TEST_P(GLSLTest_ES3, UnaryMinusOperatorFloat)
{
- // TODO(oetuaho@nvidia.com): re-enable once http://crbug.com/672380 is fixed.
- if ((IsWindows() || IsLinux()) && IsNVIDIA() && IsOpenGL())
- {
- std::cout << "Test disabled on this OpenGL configuration." << std::endl;
- return;
- }
-
const std::string &vert =
"#version 300 es\n"
"in highp vec4 position;\n"
@@ -2198,6 +2191,31 @@
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
+// Test that atan(vec2, vec2) calculation is correct.
+TEST_P(GLSLTest_ES3, AtanVec2)
+{
+ const std::string &vert =
+ "#version 300 es\n"
+ "in highp vec4 position;\n"
+ "void main() {\n"
+ " gl_Position = position;\n"
+ "}\n";
+ const std::string &frag =
+ "#version 300 es\n"
+ "out highp vec4 o_color;\n"
+ "void main() {\n"
+ " highp float f = 1.0;\n"
+ " // atan(tan(0.5), f) should be 0.5.\n"
+ " highp vec2 v = atan(vec2(tan(0.5)), vec2(f));\n"
+ " o_color = (abs(v[0] - 0.5) < 0.001 && abs(v[1] - 0.5) < 0.001) ? vec4(0, 1, 0, 1) : "
+ "vec4(1, 0, 0, 1);\n"
+ "}\n";
+
+ ANGLE_GL_PROGRAM(prog, vert, frag);
+ drawQuad(prog.get(), "position", 0.5f);
+ EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
// Convers a bug with the unary minus operator on signed integer workaround.
TEST_P(GLSLTest_ES3, UnaryMinusOperatorSignedInt)
{