fix several issue with the debug datasource in View

- the last View created was always overriding previous View's datasource
- because of lazy registering of the data source it was possible that
  the registering lambda was called after the view was destroyed, leading
  to crashes
- all view would share the same PID parameters and these would be
  initialized to default value instead of the user provided value. so
  debug build would behave differently.

With this change we improve things:
- now only the first view gets to publish its data source. it's still
  not ideal, but works for our use case with gltf_Viewer
- the view can now unregister itself when it's destroyed
- only the view that successfully registered uses the debug PID values
  and publishes its data source.
- the normal parameters are used until we query the datasource (from
  imgui), so by default the behavior is now identical to release builds


This fixes a crash in gltf_viewer when opening the Debug panel.

filament/src/details/DebugRegistry.cpp

@@ -118,19 +118,32 @@ template bool FDebugRegistry::getProperty<float2>(const char* name, float2* v) c
 template bool FDebugRegistry::getProperty<float3>(const char* name, float3* v) const noexcept;
 template bool FDebugRegistry::getProperty<float4>(const char* name, float4* v) const noexcept;
 
-void FDebugRegistry::registerDataSource(std::string_view name,
+bool FDebugRegistry::registerDataSource(std::string_view name,
         void const* data, size_t count) noexcept {
     auto& dataSourceMap = mDataSourceMap;
-    if (dataSourceMap.find(name) == dataSourceMap.end()) {
+    bool const found = dataSourceMap.find(name) == dataSourceMap.end();
+    if (found) {
         dataSourceMap[name] = { data, count };
     }
+    return found;
 }
 
-void FDebugRegistry::registerDataSource(std::string_view name,
+bool FDebugRegistry::registerDataSource(std::string_view name,
         utils::Invocable<DataSource()>&& creator) noexcept {
-    mDataSourceCreatorMap[name] = std::move(creator);
+    auto& dataSourceCreatorMap = mDataSourceCreatorMap;
+    bool const found = dataSourceCreatorMap.find(name) == dataSourceCreatorMap.end();
+    if (found) {
+        dataSourceCreatorMap[name] = std::move(creator);
+    }
+    return found;
 }
 
+void FDebugRegistry::unregisterDataSource(std::string_view name) noexcept {
+    mDataSourceCreatorMap.erase(name);
+    mDataSourceMap.erase(name);
+}
+
+
 DebugRegistry::DataSource FDebugRegistry::getDataSource(const char* name) const noexcept {
     std::string_view const key{ name };
     auto& dataSourceMap = mDataSourceMap;

filament/src/details/DebugRegistry.h

@@ -101,12 +101,14 @@ public:
     }
 
     // registers a DataSource directly
-    void registerDataSource(std::string_view name, void const* data, size_t count) noexcept;
+    bool registerDataSource(std::string_view name, void const* data, size_t count) noexcept;
 
     // registers a DataSource lazily
-    void registerDataSource(std::string_view name,
+    bool registerDataSource(std::string_view name,
             utils::Invocable<DataSource()>&& creator) noexcept;
 
+    void unregisterDataSource(std::string_view name) noexcept;
+
 #if !defined(_MSC_VER)
 private:
 #endif

filament/src/details/View.cpp

@@ -31,15 +31,17 @@
 #include "details/Skybox.h"
 
 #include <filament/Exposure.h>
+#include <filament/DebugRegistry.h>
 #include <filament/TextureSampler.h>
 #include <filament/View.h>
 
 #include <private/filament/UibStructs.h>
 
+#include <utils/compiler.h>
+#include <utils/debug.h>
 #include <utils/Profiler.h>
 #include <utils/Slice.h>
 #include <utils/Systrace.h>
-#include <utils/debug.h>
 #include <utils/Zip2Iterator.h>
 
 #include <math/scalar.h>
@@ -47,6 +49,7 @@
 
 #include <array>
 #include <memory>
+#include <tuple>
 
 using namespace utils;
 
@@ -78,21 +81,29 @@ FView::FView(FEngine& engine)
     mPidController.setOutputDeadBand(-0.01f, 0.05f);
 
 #ifndef NDEBUG
-    debugRegistry.registerDataSource("d.view.frame_info",
-            [this]() -> DebugRegistry::DataSource {
-                assert_invariant(!mDebugFrameHistory);
-                mDebugFrameHistory = std::make_unique<std::array<DebugRegistry::FrameHistory, 5*60>>();
-                return { mDebugFrameHistory->data(), mDebugFrameHistory->size() };
+    // This can fail if another view has already registered this data source
+    mDebugState->owner = debugRegistry.registerDataSource("d.view.frame_info",
+            [weak = std::weak_ptr<DebugState>(mDebugState)]() -> DebugRegistry::DataSource {
+                // the View could have been destroyed by the time we do this
+                auto const state = weak.lock();
+                if (!state) {
+                    return { nullptr, 0 };
+                }
+                // Lazily allocate the buffer for the debug data source, and mark this
+                // data source as active. It can never go back to inactive.
+                assert_invariant(!state->debugFrameHistory);
+                state->active = true;
+                state->debugFrameHistory =
+                        std::make_unique<std::array<DebugRegistry::FrameHistory, 5 * 60>>();
+                return { state->debugFrameHistory->data(), state->debugFrameHistory->size() };
             });
-    debugRegistry.registerProperty("d.view.pid.kp", &engine.debug.view.pid.kp);
-    debugRegistry.registerProperty("d.view.pid.ki", &engine.debug.view.pid.ki);
-    debugRegistry.registerProperty("d.view.pid.kd", &engine.debug.view.pid.kd);
-    // default parameters for debugging UI
-    engine.debug.view.pid.kp = 1.0f - std::exp(-1.0f / 8.0f);
-    engine.debug.view.pid.ki = PID_CONTROLLER_Ki;
-    engine.debug.view.pid.kd = PID_CONTROLLER_Kd;
-    mPidController.setParallelGains(
-            engine.debug.view.pid.kp, engine.debug.view.pid.ki, engine.debug.view.pid.kd);
+
+    if (UTILS_UNLIKELY(mDebugState->owner)) {
+        // publish the properties (they will be initialized in the main loop)
+        debugRegistry.registerProperty("d.view.pid.kp", &engine.debug.view.pid.kp);
+        debugRegistry.registerProperty("d.view.pid.ki", &engine.debug.view.pid.ki);
+        debugRegistry.registerProperty("d.view.pid.kd", &engine.debug.view.pid.kd);
+    }
 #endif
 
     // allocate UBOs
@@ -126,6 +137,12 @@ void FView::terminate(FEngine& engine) {
     mFroxelizer.terminate(driver);
 
     engine.getEntityManager().destroy(mFogEntity);
+
+#ifndef NDEBUG
+    if (UTILS_UNLIKELY(mDebugState->owner)) {
+        engine.getDebugRegistry().unregisterDataSource("d.view.frame_info");
+    }
+#endif
 }
 
 void FView::setViewport(filament::Viewport const& viewport) noexcept {
@@ -167,6 +184,16 @@ float2 FView::updateScale(FEngine& engine,
         Renderer::FrameRateOptions const& frameRateOptions,
         Renderer::DisplayInfo const& displayInfo) noexcept {
 
+#ifndef NDEBUG
+    if (UTILS_LIKELY(!mDebugState->active)) {
+        // if we're not active, update the debug properties with the normal values
+        // and use that for configuring the PID controller.
+        engine.debug.view.pid.kp = 1.0f - std::exp(-frameRateOptions.scaleRate);
+        engine.debug.view.pid.ki = PID_CONTROLLER_Ki;
+        engine.debug.view.pid.kd = PID_CONTROLLER_Kd;
+    }
+#endif
+
     DynamicResolutionOptions const& options = mDynamicResolution;
     if (options.enabled) {
         if (!UTILS_UNLIKELY(info.valid)) {
@@ -250,14 +277,15 @@ float2 FView::updateScale(FEngine& engine,
 
 #ifndef NDEBUG
     // only for debugging...
-    if (mDebugFrameHistory) {
+    if (UTILS_UNLIKELY(mDebugState->active && mDebugState->debugFrameHistory)) {
+        auto* const debugFrameHistory = mDebugState->debugFrameHistory.get();
         using namespace std::chrono;
         using duration_ms = duration<float, std::milli>;
         const float target = (1000.0f * float(frameRateOptions.interval)) / displayInfo.refreshRate;
         const float targetWithHeadroom = target * (1.0f - frameRateOptions.headRoomRatio);
-        std::move(mDebugFrameHistory->begin() + 1,
-                mDebugFrameHistory->end(), mDebugFrameHistory->begin());
-        mDebugFrameHistory->back() = {
+        std::move(debugFrameHistory->begin() + 1,
+                debugFrameHistory->end(), debugFrameHistory->begin());
+        debugFrameHistory->back() = {
                 .target             = target,
                 .targetWithHeadroom = targetWithHeadroom,
                 .frameTime          = duration_cast<duration_ms>(info.frameTime).count(),

filament/src/details/View.h

@@ -576,7 +576,12 @@ private:
                                                     }};
 
 #ifndef NDEBUG
-    std::unique_ptr<std::array<DebugRegistry::FrameHistory, 5*60>> mDebugFrameHistory;
+    struct DebugState {
+        std::unique_ptr<std::array<DebugRegistry::FrameHistory, 5*60>> debugFrameHistory{};
+        bool owner = false;
+        bool active = false;
+    };
+    std::shared_ptr<DebugState> mDebugState{ new DebugState };
 #endif
 };