Audit material variants (google#2948)

libs/filabridge/include/private/filament/Variant.h

@@ -48,7 +48,7 @@ namespace filament {
         //           Reserved             X    0     X     1     0     0
         //
         // Standard variants:
-        //      Vertex shader             0    0     X     X     0     X
+        //      Vertex shader             0    0     X     X     X     X
         //    Fragment shader             X    0     0     X     X     X
 
         uint8_t key = 0;
@@ -63,6 +63,7 @@ namespace filament {
         static constexpr uint8_t FOG                    = 0x20; // fog
 
         static constexpr uint8_t VERTEX_MASK = DIRECTIONAL_LIGHTING |
+                                               DYNAMIC_LIGHTING |
                                                SHADOW_RECEIVER |
                                                SKINNING_OR_MORPHING |
                                                DEPTH;
@@ -76,7 +77,8 @@ namespace filament {
         static constexpr uint8_t DEPTH_MASK = DIRECTIONAL_LIGHTING |
                                               DYNAMIC_LIGHTING |
                                               SHADOW_RECEIVER |
-                                              DEPTH;
+                                              DEPTH |
+                                              FOG;
 
         // the depth variant deactivates all variants that make no sense when writing the depth
         // only -- essentially, all fragment-only variants.
@@ -98,18 +100,30 @@ namespace filament {
         inline void setFog(bool v) noexcept { set(v, FOG); }
 
         inline constexpr bool isDepthPass() const noexcept {
-            return (key & DEPTH_MASK) == DEPTH_VARIANT;
+            return isValidDepthVariant(key);
+        }
+
+        inline static constexpr bool isValidDepthVariant(uint8_t variantKey) noexcept {
+            // For a variant to be a valid depth variant, all of the bits in DEPTH_MASK must be 0,
+            // except for DEPTH.
+            return (variantKey & DEPTH_MASK) == DEPTH_VARIANT;
         }
 
         static constexpr bool isReserved(uint8_t variantKey) noexcept {
             // reserved variants that should just be skipped
-            return (variantKey & DEPTH_MASK) > DEPTH || (variantKey & 0b010111u) == 0b000100u;
+            // 1. If the DEPTH bit is set, then it must be a valid depth variant. Otherwise, the
+            // variant is reserved.
+            // 2. If SRE is set, either DYN or DIR must also be set (it makes no sense to have
+            // shadows without lights).
+            return
+                ((variantKey & DEPTH) && !isValidDepthVariant(variantKey)) ||
+                (variantKey & 0b010111u) == 0b000100u;
         }
 
         static constexpr uint8_t filterVariantVertex(uint8_t variantKey) noexcept {
-            // filter out vertex variants that are not needed. For e.g. dynamic lighting
-            // doesn't affect the vertex shader.
-            return variantKey;
+            // filter out vertex variants that are not needed. For e.g. fog doesn't affect the
+            // vertex shader.
+            return variantKey & VERTEX_MASK;
         }
 
         static constexpr uint8_t filterVariantFragment(uint8_t variantKey) noexcept {
@@ -120,7 +134,7 @@ namespace filament {
 
         static constexpr uint8_t filterVariant(uint8_t variantKey, bool isLit) noexcept {
             // special case for depth variant
-            if ((variantKey & DEPTH_MASK) == DEPTH_VARIANT) {
+            if (isValidDepthVariant(variantKey)) {
                 return variantKey;
             }
             // when the shading mode is unlit, remove all the lighting variants

libs/filamat/src/shaders/ShaderGenerator.cpp

@@ -189,6 +189,7 @@ std::string ShaderGenerator::createVertexProgram(filament::backend::ShaderModel
 
     bool litVariants = lit || material.hasShadowMultiplier;
     cg.generateDefine(vs, "HAS_DIRECTIONAL_LIGHTING", litVariants && variant.hasDirectionalLighting());
+    cg.generateDefine(vs, "HAS_DYNAMIC_LIGHTING", litVariants && variant.hasDynamicLighting());
     cg.generateDefine(vs, "HAS_SHADOWING", litVariants && variant.hasShadowReceiver());
     cg.generateDefine(vs, "HAS_SHADOW_MULTIPLIER", material.hasShadowMultiplier);
     cg.generateDefine(vs, "HAS_SKINNING_OR_MORPHING", variant.hasSkinningOrMorphing());

libs/matdbg/CMakeLists.txt

@@ -23,6 +23,7 @@ set(PUBLIC_HDRS
 
 set(SRCS
     src/CommonWriter.h
+    src/CommonWriter.cpp
     src/DebugServer.cpp
     src/JsonWriter.cpp
     src/ShaderReplacer.cpp

libs/matdbg/src/CommonWriter.cpp

@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "CommonWriter.h"
+
+#include <private/filament/Variant.h>
+
+namespace filament::matdbg {
+
+std::string formatVariantString(uint8_t variant) noexcept {
+    std::string variantString = "";
+
+    // NOTE: The 3-character nomenclature used here is consistent with the ASCII art seen in the
+    // Variant header file and allows the information to fit in a reasonable amount of space on
+    // the page. The HTML file has a legend.
+    if (variant) {
+        if (variant & Variant::DIRECTIONAL_LIGHTING)  variantString += "DIR|";
+        if (variant & Variant::DYNAMIC_LIGHTING)      variantString += "DYN|";
+        if (variant & Variant::SHADOW_RECEIVER)       variantString += "SRE|";
+        if (variant & Variant::SKINNING_OR_MORPHING)  variantString += "SKN|";
+        if (variant & Variant::DEPTH)                 variantString += "DEP|";
+        if (variant & Variant::FOG)                   variantString += "FOG|";
+        variantString = variantString.substr(0, variantString.length() - 1);
+    }
+
+    return variantString;
+}
+
+} // namespace filament::matdbg

libs/matdbg/src/CommonWriter.h

@@ -29,6 +29,8 @@
 #include <matdbg/JsonWriter.h>
 #include <matdbg/ShaderInfo.h>
 
+#include <string>
+
 namespace filament {
 namespace matdbg {
 
@@ -214,5 +216,9 @@ const char* toString(backend::SamplerFormat format) noexcept {
     }
 }
 
+// Returns a human-readable variant description.
+// For example: DYN|DIR
+std::string formatVariantString(uint8_t variant) noexcept;
+
 } // namespace matdbg
 } // namespace filament

libs/matdbg/src/JsonWriter.cpp

@@ -118,20 +118,8 @@ static bool printParametersInfo(ostream& json, const ChunkContainer& container)
 static void printShaderInfo(ostream& json, const std::vector<ShaderInfo>& info) {
     for (uint64_t i = 0; i < info.size(); ++i) {
         const auto& item = info[i];
-        string variantString = "";
-
-        // NOTE: The 3-character nomenclature used here is consistent with the ASCII art seen in the
-        // Variant header file and allows the information to fit in a reasonable amount of space on
-        // the page. The HTML file has a legend.
-        if (item.variant) {
-            if (item.variant & filament::Variant::DIRECTIONAL_LIGHTING)  variantString += "DIR|";
-            if (item.variant & filament::Variant::DYNAMIC_LIGHTING)      variantString += "DYN|";
-            if (item.variant & filament::Variant::SHADOW_RECEIVER)       variantString += "SRE|";
-            if (item.variant & filament::Variant::SKINNING_OR_MORPHING)  variantString += "SKN|";
-            if (item.variant & filament::Variant::DEPTH)                 variantString += "DEP|";
-            variantString = variantString.substr(0, variantString.length() - 1);
-        }
 
+        string variantString = formatVariantString(item.variant);
         string ps = (item.pipelineStage == backend::ShaderType::VERTEX) ? "vertex  " : "fragment";
         json
             << "    {"

libs/matdbg/src/TextWriter.cpp

@@ -290,7 +290,10 @@ static void printShaderInfo(ostream& text, const vector<ShaderInfo>& info) {
         text << " ";
         text << "0x" << hex << setfill('0') << setw(2)
              << right << (int) item.variant;
-        text << setfill(' ') << dec << endl;
+        text << setfill(' ') << dec;
+        text << "   ";
+        text << formatVariantString(item.variant);
+        text << endl;
     }
     text << endl;
 }

shaders/src/inputs.vs

@@ -80,6 +80,6 @@ LAYOUT_LOCATION(10) out highp vec4 vertex_uv01;
 LAYOUT_LOCATION(11) out highp vec4 vertex_lightSpacePosition;
 #endif
 
-#if defined(HAS_SHADOWING)
+#if defined(HAS_SHADOWING) && defined(HAS_DYNAMIC_LIGHTING)
 LAYOUT_LOCATION(12) out highp vec4 vertex_spotLightSpacePosition[MAX_SHADOW_CASTING_SPOTS];
 #endif

shaders/src/main.vs

@@ -92,7 +92,7 @@ void main() {
             frameUniforms.lightDirection, frameUniforms.shadowBias.y, getLightFromWorldMatrix());
 #endif
 
-#if defined(HAS_SHADOWING)
+#if defined(HAS_SHADOWING) && defined(HAS_DYNAMIC_LIGHTING)
     for (uint l = 0u; l < uint(MAX_SHADOW_CASTING_SPOTS); l++) {
         vec3 dir = shadowUniforms.directionShadowBias[l].xyz;
         float bias = shadowUniforms.directionShadowBias[l].w;