material-parser: refactor computing, caching of crc32

- Make the two ways to get crc32 explicitly documented.
 - Ensure the slow path of computing crc32 from scratch is only
   happens on init, either
     - Creating MaterialParser for validation
     - Or, using crc32 val as the key for the first time in
       MaterialCache
 - In doing the above, move crc32 validation to
   MaterialDefinition::createParser

filament/src/MaterialCache.cpp

@@ -28,11 +28,7 @@ namespace filament {
 
 size_t MaterialCache::MaterialKey::Hash::operator()(
         filament::MaterialCache::MaterialKey const& key) const noexcept {
-    uint32_t crc;
-    if (key.parser->getMaterialCrc32(&crc)) {
-        return size_t(crc);
-    }
-    return size_t(key.parser->computeCrc32());
+    return size_t(key.parser->getCrc32());
 }
 
 bool MaterialCache::MaterialKey::operator==(MaterialKey const& rhs) const noexcept {
@@ -60,8 +56,9 @@ void MaterialCache::terminate(FEngine& engine) {
 
 MaterialDefinition* UTILS_NULLABLE MaterialCache::acquireMaterial(FEngine& engine,
         const void* UTILS_NONNULL data, size_t size) noexcept {
-    std::unique_ptr<MaterialParser> parser = MaterialDefinition::createParser(engine.getBackend(),
-            engine.getShaderLanguage(), data, size);
+    bool const checkCrc32 = engine.features.material.check_crc32_after_loading;
+    std::unique_ptr<MaterialParser> parser =
+            MaterialDefinition::createParser(engine, data, size, checkCrc32);
     assert_invariant(parser);
 
     // The `key` must be constructed using parser.get() before parser is moved into the lambda

filament/src/MaterialDefinition.cpp

@@ -162,11 +162,30 @@ void releaseProgramsImpl(FEngine& engine, utils::Slice<Handle<HwProgram>> progra
 
 } // namespace
 
-std::unique_ptr<MaterialParser> MaterialDefinition::createParser(Backend const backend,
-        FixedCapacityVector<ShaderLanguage> languages, const void* data, size_t size) {
+std::unique_ptr<MaterialParser> MaterialDefinition::createParser(FEngine const& engine,
+        const void* data, size_t size, bool checkCrc32) {
+    auto const& languages = engine.getShaderLanguage();
+    auto const backend = engine.getBackend();
+
     // unique_ptr so we don't leak MaterialParser on failures below
     auto materialParser = std::make_unique<MaterialParser>(languages, data, size);
 
+    // Try checking CRC32 value for the package and skip if it's unavailable.
+    if (checkCrc32) {
+        uint32_t parsedCrc32 = 0;
+        bool const foundParsedCrc = materialParser->getMaterialCrc32(&parsedCrc32);
+        uint32_t const expectedCrc32 = materialParser->computeCrc32();
+
+        if (foundParsedCrc && parsedCrc32 != expectedCrc32) {
+            CString name;
+            materialParser->getName(&name);
+            LOG(ERROR) << "The material '" << name.c_str_safe()
+                       << "' is corrupted: crc32_expected=" << expectedCrc32
+                       << ", crc32_parsed=" << parsedCrc32;
+            return nullptr;
+        }
+    }
+
     MaterialParser::ParseResult const materialResult = materialParser->parse();
 
     CString name;
@@ -174,7 +193,7 @@ std::unique_ptr<MaterialParser> MaterialDefinition::createParser(Backend const b
     if (UTILS_UNLIKELY(materialResult == MaterialParser::ParseResult::ERROR_MISSING_BACKEND)) {
         CString languageNames;
         for (auto it = languages.begin(); it != languages.end(); ++it) {
-            languageNames.append(CString{shaderLanguageToString(*it)});
+            languageNames.append(CString{ shaderLanguageToString(*it) });
             if (std::next(it) != languages.end()) {
                 languageNames.append(", ");
             }
@@ -182,8 +201,9 @@ std::unique_ptr<MaterialParser> MaterialDefinition::createParser(Backend const b
 
         FILAMENT_CHECK_POSTCONDITION(
                 materialResult != MaterialParser::ParseResult::ERROR_MISSING_BACKEND)
-                << "the material " << name.c_str_safe() << " was not built for any of the " << to_string(backend)
-                << " backend's supported shader languages (" << languageNames.c_str() << ")\n";
+                << "the material " << name.c_str_safe() << " was not built for any of the "
+                << to_string(backend) << " backend's supported shader languages ("
+                << languageNames.c_str() << ")\n";
     }
 
     if (backend == Backend::NOOP) {
@@ -206,23 +226,6 @@ std::unique_ptr<MaterialParser> MaterialDefinition::createParser(Backend const b
 
 std::unique_ptr<MaterialDefinition> MaterialDefinition::create(FEngine& engine,
         std::unique_ptr<MaterialParser> parser) {
-    // Try checking CRC32 value for the package and skip if it's unavailable.
-    if (downcast(engine).features.material.check_crc32_after_loading) {
-        uint32_t parsedCrc32 = 0;
-        parser->getMaterialCrc32(&parsedCrc32);
-
-        uint32_t const expectedCrc32 = parser->computeCrc32();
-
-        if (parsedCrc32 != expectedCrc32) {
-            CString name;
-            parser->getName(&name);
-            LOG(ERROR) << "The material '" << name.c_str_safe()
-                       << "' is corrupted: crc32_expected=" << expectedCrc32
-                       << ", crc32_parsed=" << parsedCrc32;
-            return nullptr;
-        }
-    }
-
     uint32_t v = 0;
     parser->getShaderModels(&v);
     bitset32 shaderModels;

filament/src/MaterialDefinition.h

@@ -165,9 +165,8 @@ private:
     friend class MaterialCache;
     friend class FMaterial; // for onEditCallback
 
-    static std::unique_ptr<MaterialParser> createParser(backend::Backend const backend,
-            utils::FixedCapacityVector<backend::ShaderLanguage> languages,
-            const void* UTILS_NONNULL data, size_t size);
+    static std::unique_ptr<MaterialParser> createParser(FEngine const& engine,
+            const void* UTILS_NONNULL data, size_t size, bool checkCrc32);
 
     static std::unique_ptr<MaterialDefinition> create(FEngine& engine,
             std::unique_ptr<MaterialParser> parser);

filament/src/MaterialParser.cpp

@@ -44,6 +44,7 @@
 
 #include <array>
 #include <atomic>
+#include <mutex>
 #include <optional>
 #include <tuple>
 #include <utility>
@@ -120,12 +121,10 @@ bool MaterialParser::operator==(MaterialParser const& rhs) const noexcept {
     if (mImpl.mManagedBuffer.size() != rhs.mImpl.mManagedBuffer.size()) {
         return false;
     }
-    std::optional<uint32_t> lhsCrc32 = getPrecomputedCrc32();
-    if (lhsCrc32) {
-        std::optional<uint32_t> rhsCrc32 = rhs.getPrecomputedCrc32();
-        if (rhsCrc32 && *lhsCrc32 != *rhsCrc32) {
-            return false;
-        }
+    uint32_t const lhsCrc32 = getCrc32();
+    uint32_t const rhsCrc32 = rhs.getCrc32();
+    if (lhsCrc32 != rhsCrc32) {
+        return false;
     }
     return !memcmp(mImpl.mManagedBuffer.data(), rhs.mImpl.mManagedBuffer.data(),
             mImpl.mManagedBuffer.size());
@@ -187,12 +186,34 @@ MaterialParser::ParseResult MaterialParser::parse() noexcept {
     return ParseResult::SUCCESS;
 }
 
-uint32_t MaterialParser::computeCrc32() const noexcept {
-    uint32_t crc32 = mCrc32.load(std::memory_order_relaxed);
-    if (crc32) {
-        return crc32;
+uint32_t MaterialParser::getCrc32() const noexcept {
+    if (mCrc32Cached.load(std::memory_order_relaxed)) {
+        return mCrc32;
     }
 
+    std::lock_guard<utils::Mutex> lock(mCrc32CachedLock);
+
+    // If we enter this section after another thread has passed through it, we need to check whether
+    // crc32 has been cached or not. This is the slow path that will happen hopefully just once.
+    if (mCrc32Cached.load(std::memory_order_relaxed)) {
+        return mCrc32;
+    }
+
+    // First check whether the compiled material contains the crc32 already.
+    if (uint32_t parsedCrc32 = 0; getMaterialCrc32(&parsedCrc32)) {
+        mCrc32 = parsedCrc32;
+        mCrc32Cached.store(true);
+        return parsedCrc32;
+    }
+
+    // No crc32 found, so we compute it.
+    mCrc32 = computeCrc32();
+    mCrc32Cached.store(true);
+    return mCrc32;
+}
+
+
+uint32_t MaterialParser::computeCrc32() const noexcept {
     const size_t size = mImpl.mManagedBuffer.size();
     const void* const UTILS_NONNULL payload = mImpl.mManagedBuffer.data();
 
@@ -202,29 +223,7 @@ uint32_t MaterialParser::computeCrc32() const noexcept {
 
     std::vector<uint32_t> crc32Table;
     utils::hash::crc32GenerateTable(crc32Table);
-    crc32 = utils::hash::crc32Update(0, payload, originalSize, crc32Table);
-    mCrc32.store(crc32, std::memory_order_relaxed);
-    return crc32;
-}
-
-std::optional<uint32_t> MaterialParser::getPrecomputedCrc32() const noexcept {
-    uint32_t cachedCrc32 = mCrc32.load(std::memory_order_relaxed);
-    if (cachedCrc32) {
-        return cachedCrc32;
-    }
-    uint32_t parsedCrc32;
-    if (getMaterialCrc32(&parsedCrc32)) {
-        return parsedCrc32;
-    }
-    return std::nullopt;
-}
-
-uint32_t MaterialParser::getCrc32() const noexcept {
-    std::optional<uint32_t> crc32 = getPrecomputedCrc32();
-    if (crc32) {
-        return *crc32;
-    }
-    return computeCrc32();
+    return utils::hash::crc32Update(0, payload, originalSize, crc32Table);
 }
 
 ShaderLanguage MaterialParser::getShaderLanguage() const noexcept {

filament/src/MaterialParser.h

@@ -30,6 +30,7 @@
 
 #include <utils/CString.h>
 #include <utils/FixedCapacityVector.h>
+#include <utils/Mutex.h>
 
 #include <array>
 #include <optional>
@@ -70,13 +71,13 @@ public:
 
     ParseResult parse() noexcept;
 
-    // Compute the CRC32 of the material or return the cached value.
-    uint32_t computeCrc32() const noexcept;
-    // Return the cached computed CRC32 or the CRC32 built into the material file if one exists.
-    std::optional<uint32_t> getPrecomputedCrc32() const noexcept;
-    // Return the CRC32 of the material.
+    // Return the CRC32 of the material. This will use a cached value if it is available.
     uint32_t getCrc32() const noexcept;
 
+    // Return the CRC32 of the material. This will *always* compute the crc32 from the full compiled
+    // binary content (i.e. this is the slow path).
+    uint32_t computeCrc32() const noexcept;
+
     backend::ShaderLanguage getShaderLanguage() const noexcept;
 
     // Accessors
@@ -196,9 +197,10 @@ private:
     filaflat::ChunkContainer& getChunkContainer() noexcept;
     filaflat::ChunkContainer const& getChunkContainer() const noexcept;
     MaterialParserDetails mImpl;
-    // 0 == not cached. This technically means that a file with a CRC32 of 0 will never be cached,
-    // but this is unlikely, and keeping it a 32-bit value guarantees that it will be lockless.
-    mutable std::atomic<uint32_t> mCrc32 = 0;
+
+    mutable uint32_t mCrc32 = 0;
+    mutable std::atomic<bool> mCrc32Cached;
+    mutable utils::Mutex mCrc32CachedLock;
 };
 
 struct ChunkUniformInterfaceBlock {

filament/src/details/Material.cpp

@@ -414,8 +414,10 @@ void FMaterial::onEditCallback(void* userdata, const CString&, const void* packa
 
     // This is called on a web server thread, so we defer clearing the program cache
     // and swapping out the MaterialParser until the next getProgram call.
-    std::unique_ptr<MaterialParser> pending = MaterialDefinition::createParser(
-            engine.getBackend(), engine.getShaderLanguage(), packageData, packageSize);
+    // Also note that we don't check for crc32 because it's not meant for a dynamically generated
+    // material package.
+    std::unique_ptr<MaterialParser> pending = MaterialDefinition::createParser(engine, packageData,
+            packageSize, false /* checkCrc32 */);
     material->setPendingEdits(std::move(pending));
 }