From 89835a7a678ec0f93adb12499b24d7140859f029 Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Tue, 20 Aug 2024 09:28:29 -0700 Subject: [PATCH 01/26] Fix a bug for calculating distance (#8058) This is a missing part from the change 22d99bac3d0492c0284f7654fa0de0694643e220 --- filament/src/RenderPass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filament/src/RenderPass.cpp b/filament/src/RenderPass.cpp index d9f4a74dc46..55ec3264b2d 100644 --- a/filament/src/RenderPass.cpp +++ b/filament/src/RenderPass.cpp @@ -616,7 +616,7 @@ RenderPass::Command* RenderPass::generateCommandsImpl(RenderPass::CommandTypeFla // Here, objects close to the camera (but behind) will be drawn first. // An alternative that keeps the mathematical ordering is given here: // distanceBits ^= ((int32_t(distanceBits) >> 31) | 0x80000000u); - float const distance = -dot(soaWorldAABBCenter[i], cameraForward) - cameraPositionDotCameraForward; + float const distance = -(dot(soaWorldAABBCenter[i], cameraForward) - cameraPositionDotCameraForward); uint32_t const distanceBits = reinterpret_cast(distance); // calculate the per-primitive face winding order inversion From 1f9a11802d01adfe16ea039e3dc500c6ea7bfed8 Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Tue, 20 Aug 2024 10:13:55 -0700 Subject: [PATCH 02/26] Minor tweak (#8053) Move the common part outside the loop. --- filament/src/fg/PassNode.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/filament/src/fg/PassNode.cpp b/filament/src/fg/PassNode.cpp index cf76ea35b76..51eb06645d5 100644 --- a/filament/src/fg/PassNode.cpp +++ b/filament/src/fg/PassNode.cpp @@ -190,10 +190,10 @@ void RenderPassNode::resolve() noexcept { minHeight = std::min(minHeight, h); maxHeight = std::max(maxHeight, h); } - // additionally, clear implies discardStart - rt.backend.params.flags.discardStart |= ( - rt.descriptor.clearFlags & rt.targetBufferFlags); } + // additionally, clear implies discardStart + rt.backend.params.flags.discardStart |= ( + rt.descriptor.clearFlags & rt.targetBufferFlags); assert_invariant(minWidth == maxWidth); assert_invariant(minHeight == maxHeight); From b70aa43727a5bdaff16b484b8c4ddba7ab809500 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Mon, 19 Aug 2024 22:42:32 -0700 Subject: [PATCH 03/26] depth clamp cannot work with VSM This is because the shadowmap doesn't store depth values so it doesn't work to "flatten" all casters behind de camera to the near plane. The bulk of shadowing works but the filtering/edges don't always. Disable depth-clamping when VSM is enabled. --- filament/src/ShadowMapManager.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/filament/src/ShadowMapManager.cpp b/filament/src/ShadowMapManager.cpp index 1f3c902be58..c248dfbcc0f 100644 --- a/filament/src/ShadowMapManager.cpp +++ b/filament/src/ShadowMapManager.cpp @@ -371,7 +371,13 @@ FrameGraphId ShadowMapManager::render(FEngine& engine, FrameG if (view.isFrontFaceWindingInverted()) { renderPassFlags |= RenderPass::HAS_INVERSE_FRONT_FACES; } - if (mIsDepthClampSupported && engine.debug.shadowmap.depth_clamp) { + + bool const canUseDepthClamp = + !view.hasVSM() && + mIsDepthClampSupported && + engine.debug.shadowmap.depth_clamp; + + if (canUseDepthClamp) { renderPassFlags |= RenderPass::HAS_DEPTH_CLAMP; } @@ -650,9 +656,14 @@ ShadowMapManager::ShadowTechnique ShadowMapManager::updateCascadeShadowMaps(FEng cameraInfo.zf = -nearFarPlanes[i + 1]; updateNearFarPlanes(&cameraInfo.cullingProjection, cameraInfo.zn, cameraInfo.zf); + bool const canUseDepthClamp = + !view.hasVSM() && + mIsDepthClampSupported && + engine.debug.shadowmap.depth_clamp; + auto shaderParameters = shadowMap.updateDirectional(engine, lightData, 0, cameraInfo, shadowMapInfo, sceneInfo, - mIsDepthClampSupported && engine.debug.shadowmap.depth_clamp); + canUseDepthClamp); if (shadowMap.hasVisibleShadows()) { const size_t shadowIndex = shadowMap.getShadowIndex(); From 3351db11783941a2b45e86dd2943bd3451189530 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Mon, 19 Aug 2024 14:56:51 -0700 Subject: [PATCH 04/26] improve FrameSkipper when disregarding skipping It is legal for the app to draw a frame even when FrameSkipper detects that a frame should be skipped. In that case we used to overwrite the most recent fence, but this wasn't ideal. We now proceed as if the fence had signaled, i.e. we destroy it and move all the fences up, creating a new one at the end of the delayed array. --- filament/src/FrameSkipper.cpp | 43 ++++++++++++++++++++--------------- filament/src/FrameSkipper.h | 5 +++- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/filament/src/FrameSkipper.cpp b/filament/src/FrameSkipper.cpp index 7942ecfa5ad..ef14f5a0f82 100644 --- a/filament/src/FrameSkipper.cpp +++ b/filament/src/FrameSkipper.cpp @@ -16,16 +16,22 @@ #include "FrameSkipper.h" -#include +#include + +#include #include +#include + +#include + namespace filament { using namespace utils; using namespace backend; FrameSkipper::FrameSkipper(size_t latency) noexcept - : mLast(latency - 1) { + : mLast(std::max(latency, MAX_FRAME_LATENCY) - 1) { assert_invariant(latency <= MAX_FRAME_LATENCY); } @@ -41,31 +47,32 @@ void FrameSkipper::terminate(DriverApi& driver) noexcept { bool FrameSkipper::beginFrame(DriverApi& driver) noexcept { auto& fences = mDelayedFences; - auto fence = fences.front(); - if (fence) { - auto status = driver.getFenceStatus(fence); - if (status == FenceStatus::TIMEOUT_EXPIRED) { - // Sync not ready, skip frame + if (fences.front()) { + // Do we have a latency old fence? + auto status = driver.getFenceStatus(fences.front()); + if (UTILS_UNLIKELY(status == FenceStatus::TIMEOUT_EXPIRED)) { + // The fence hasn't signaled yet, skip this frame return false; } assert_invariant(status == FenceStatus::CONDITION_SATISFIED); - driver.destroyFence(fence); } - // shift all fences down by 1 - std::move(fences.begin() + 1, fences.end(), fences.begin()); - fences.back() = {}; return true; } void FrameSkipper::endFrame(DriverApi& driver) noexcept { - // If the user produced a new frame despite the fact that the previous one wasn't finished - // (i.e. FrameSkipper::beginFrame() returned false), we need to make sure to replace - // a fence that might be here already) - auto& fence = mDelayedFences[mLast]; - if (fence) { - driver.destroyFence(fence); + auto& fences = mDelayedFences; + size_t const last = mLast; + + // pop the oldest fence and advance the other ones + if (fences.front()) { + driver.destroyFence(fences.front()); } - fence = driver.createFence(); + std::move(fences.begin() + 1, fences.end(), fences.begin()); + + // add a new fence to the end + assert_invariant(!fences[last]); + + fences[last] = driver.createFence(); } } // namespace filament diff --git a/filament/src/FrameSkipper.h b/filament/src/FrameSkipper.h index 3b7cedbc25c..61bc4049735 100644 --- a/filament/src/FrameSkipper.h +++ b/filament/src/FrameSkipper.h @@ -22,6 +22,9 @@ #include +#include +#include + namespace filament { /* @@ -60,7 +63,7 @@ class FrameSkipper { private: using Container = std::array, MAX_FRAME_LATENCY>; mutable Container mDelayedFences{}; - size_t mLast; + uint8_t const mLast; }; } // namespace filament From d5a274fa25dd85f127578db58bc7da1ee02f772d Mon Sep 17 00:00:00 2001 From: Ben Doherty Date: Tue, 20 Aug 2024 12:28:13 -0700 Subject: [PATCH 05/26] Add better glShaderSource fix for IMG devices (#8061) --- filament/backend/src/opengl/OpenGLContext.cpp | 3 -- filament/backend/src/opengl/OpenGLContext.h | 9 ---- .../src/opengl/ShaderCompilerService.cpp | 50 ++++++++----------- 3 files changed, 20 insertions(+), 42 deletions(-) diff --git a/filament/backend/src/opengl/OpenGLContext.cpp b/filament/backend/src/opengl/OpenGLContext.cpp index 96545458258..a7933407bb4 100644 --- a/filament/backend/src/opengl/OpenGLContext.cpp +++ b/filament/backend/src/opengl/OpenGLContext.cpp @@ -541,9 +541,6 @@ void OpenGLContext::initBugs(Bugs* bugs, Extensions const& exts, bugs->delay_fbo_destruction = true; // PowerVR seems to have no problem with this (which is good for us) bugs->allow_read_only_ancillary_feedback_loop = true; - // PowerVR doesn't respect lengths passed to glShaderSource, so concatenate them into a - // single string. - bugs->concatenate_shader_strings = true; } else if (strstr(renderer, "Apple")) { // Apple GPU } else if (strstr(renderer, "Tegra") || diff --git a/filament/backend/src/opengl/OpenGLContext.h b/filament/backend/src/opengl/OpenGLContext.h index 3b4724b0957..2972480e71f 100644 --- a/filament/backend/src/opengl/OpenGLContext.h +++ b/filament/backend/src/opengl/OpenGLContext.h @@ -316,12 +316,6 @@ class OpenGLContext final : public TimerQueryFactoryInterface { // bugs or performance issues. bool force_feature_level0; - // Some drivers don't respect the length argument of glShaderSource() and (apparently) - // require each shader source string to be null-terminated. This works around the issue by - // concatenating the strings into a single null-terminated string before passing it to - // glShaderSource(). - bool concatenate_shader_strings; - } bugs = {}; // state getters -- as needed. @@ -568,9 +562,6 @@ class OpenGLContext final : public TimerQueryFactoryInterface { { bugs.force_feature_level0, "force_feature_level0", ""}, - { bugs.concatenate_shader_strings, - "concatenate_shader_strings", - ""}, }}; // this is chosen to minimize code size diff --git a/filament/backend/src/opengl/ShaderCompilerService.cpp b/filament/backend/src/opengl/ShaderCompilerService.cpp index 0c01d383b35..eb6876859be 100644 --- a/filament/backend/src/opengl/ShaderCompilerService.cpp +++ b/filament/backend/src/opengl/ShaderCompilerService.cpp @@ -597,40 +597,30 @@ void ShaderCompilerService::compileShaders(OpenGLContext& context, version = "#version 310 es\n"; } - const std::array sources = { - version.data(), - prolog.data(), - specializationConstantString.c_str(), - packingFunctions.data(), - body.data() + std::array sources = { + version, + prolog, + specializationConstantString, + packingFunctions, + { body.data(), body.size() - 1 } // null-terminated }; - const std::array lengths = { - (GLint)version.length(), - (GLint)prolog.length(), - (GLint)specializationConstantString.length(), - (GLint)packingFunctions.length(), - (GLint)body.length() - 1 // null terminated - }; + // Some of the sources may be zero-length. Remove them as to avoid passing lengths of + // zero to glShaderSource(). glShaderSource should work with lengths of zero, but some + // drivers instead interpret zero as a sentinel for a null-terminated string. + auto partitionPoint = std::stable_partition( + sources.begin(), sources.end(), [](std::string_view s) { return !s.empty(); }); + size_t count = std::distance(sources.begin(), partitionPoint); + + std::array shaderStrings; + std::array lengths; + for (size_t i = 0; i < count; i++) { + shaderStrings[i] = sources[i].data(); + lengths[i] = sources[i].size(); + } GLuint const shaderId = glCreateShader(glShaderType); - - if (UTILS_UNLIKELY(context.bugs.concatenate_shader_strings)) { - size_t totalSize = 0; - for (size_t i = 0; i < sources.size(); i++) { - totalSize += lengths[i]; - } - std::string concatenatedShaderSource; - concatenatedShaderSource.reserve(totalSize); - for (size_t i = 0; i < sources.size(); i++) { - concatenatedShaderSource.append(sources[i], lengths[i]); - } - const GLchar* ptr = concatenatedShaderSource.c_str(); - GLint length = concatenatedShaderSource.length(); - glShaderSource(shaderId, 1, &ptr, &length); - } else { - glShaderSource(shaderId, sources.size(), sources.data(), lengths.data()); - } + glShaderSource(shaderId, count, shaderStrings.data(), lengths.data()); glCompileShader(shaderId); From 574518ea745cba5101d63023b9f21c05dcea29f2 Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Tue, 20 Aug 2024 14:44:17 -0700 Subject: [PATCH 06/26] Cleanup color pass for multiview (#8062) --- filament/src/RendererUtils.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/filament/src/RendererUtils.cpp b/filament/src/RendererUtils.cpp index d465a03288b..c4f14e4ce16 100644 --- a/filament/src/RendererUtils.cpp +++ b/filament/src/RendererUtils.cpp @@ -75,7 +75,6 @@ FrameGraphId RendererUtils::colorPass( TargetBufferFlags const clearColorFlags = config.clearFlags & TargetBufferFlags::COLOR; TargetBufferFlags clearDepthFlags = config.clearFlags & TargetBufferFlags::DEPTH; TargetBufferFlags clearStencilFlags = config.clearFlags & TargetBufferFlags::STENCIL; - uint8_t layerCount = 1; data.shadows = blackboard.get("shadows"); data.ssao = blackboard.get("ssao"); @@ -172,9 +171,6 @@ FrameGraphId RendererUtils::colorPass( data.color = builder.write(data.color, FrameGraphTexture::Usage::COLOR_ATTACHMENT); data.depth = builder.write(data.depth, FrameGraphTexture::Usage::DEPTH_ATTACHMENT); - if (view.hasStereo() && engine.getConfig().stereoscopicType == StereoscopicType::MULTIVIEW) { - layerCount = engine.getConfig().stereoscopicEyeCount; - } /* * There is a bit of magic happening here regarding the viewport used. @@ -196,7 +192,7 @@ FrameGraphId RendererUtils::colorPass( .stencil = data.stencil }, .clearColor = config.clearColor, .samples = config.msaa, - .layerCount = layerCount, + .layerCount = static_cast(colorBufferDesc.depth), .clearFlags = clearColorFlags | clearDepthFlags | clearStencilFlags}); blackboard["depth"] = data.depth; }, From 1c2ffc9ed4581080e53b23dda63ed7bfee6311e7 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 20 Aug 2024 14:07:34 -0700 Subject: [PATCH 07/26] new screenshot debug option in gltf_viewer screenshots are saved in the current directory as "screenshotXX.ppm" with XX increasing after each screenshot. --- libs/viewer/include/viewer/AutomationEngine.h | 3 +++ libs/viewer/src/AutomationEngine.cpp | 5 ++-- samples/gltf_viewer.cpp | 25 ++++++++++++++++--- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/libs/viewer/include/viewer/AutomationEngine.h b/libs/viewer/include/viewer/AutomationEngine.h index 8747f59d8da..f0d907a0f7a 100644 --- a/libs/viewer/include/viewer/AutomationEngine.h +++ b/libs/viewer/include/viewer/AutomationEngine.h @@ -224,6 +224,9 @@ class UTILS_PUBLIC AutomationEngine { */ static void exportSettings(const Settings& settings, const char* filename); + static void exportScreenshot(View* view, Renderer* renderer, std::string filename, + bool autoclose, AutomationEngine* automationEngine); + Options getOptions() const { return mOptions; } bool isRunning() const { return mIsRunning; } size_t currentTest() const { return mCurrentTest; } diff --git a/libs/viewer/src/AutomationEngine.cpp b/libs/viewer/src/AutomationEngine.cpp index 19d23ef1681..810df99dbb8 100644 --- a/libs/viewer/src/AutomationEngine.cpp +++ b/libs/viewer/src/AutomationEngine.cpp @@ -56,7 +56,7 @@ static void convertRGBAtoRGB(void* buffer, uint32_t width, uint32_t height) { } } -static void exportScreenshot(View* view, Renderer* renderer, std::string filename, +void AutomationEngine::exportScreenshot(View* view, Renderer* renderer, std::string filename, bool autoclose, AutomationEngine* automationEngine) { const Viewport& vp = view->getViewport(); const size_t byteCount = vp.width * vp.height * 4; @@ -244,7 +244,8 @@ void AutomationEngine::tick(Engine* engine, const ViewerContent& content, float } if (mOptions.exportScreenshots) { - exportScreenshot(content.view, content.renderer, prefix + ".ppm", isLastTest, this); + AutomationEngine::exportScreenshot( + content.view, content.renderer, prefix + ".ppm", isLastTest, this); } if (isLastTest) { diff --git a/samples/gltf_viewer.cpp b/samples/gltf_viewer.cpp index b5c8e1b7086..e3dc4363a38 100644 --- a/samples/gltf_viewer.cpp +++ b/samples/gltf_viewer.cpp @@ -61,8 +61,10 @@ #include #include #include +#include #include #include +#include #include #include "generated/resources/gltf_demo.h" @@ -137,6 +139,8 @@ struct App { AutomationSpec* automationSpec = nullptr; AutomationEngine* automationEngine = nullptr; + bool screenshot = false; + uint8_t screenshotSeq = 0; }; static const char* DEFAULT_IBL = "assets/ibl/lightroom_14b"; @@ -877,10 +881,15 @@ int main(int argc, char** argv) { if (ImGui::CollapsingHeader("Debug")) { auto& debug = engine->getDebugRegistry(); - if (ImGui::Button("Capture frame")) { - bool* captureFrame = - debug.getPropertyAddress("d.renderer.doFrameCapture"); - *captureFrame = true; + if (engine->getBackend() == Engine::Backend::METAL) { + if (ImGui::Button("Capture frame")) { + bool* captureFrame = + debug.getPropertyAddress("d.renderer.doFrameCapture"); + *captureFrame = true; + } + } + if (ImGui::Button("Screenshot")) { + app.screenshot = true; } ImGui::Checkbox("Disable buffer padding", debug.getPropertyAddress("d.renderer.disable_buffer_padding")); @@ -1138,6 +1147,14 @@ int main(int argc, char** argv) { }; auto postRender = [&app](Engine* engine, View* view, Scene*, Renderer* renderer) { + if (app.screenshot) { + std::ostringstream stringStream; + stringStream << "screenshot" << std::setfill('0') << std::setw(2) << +app.screenshotSeq; + AutomationEngine::exportScreenshot( + view, renderer, stringStream.str() + ".ppm", false, app.automationEngine); + ++app.screenshotSeq; + app.screenshot = false; + } if (app.automationEngine->shouldClose()) { FilamentApp::get().close(); return; From 30387af61cbd29a9e9f3bdbc6959938222b64c81 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 20 Aug 2024 22:14:05 -0700 Subject: [PATCH 08/26] reenable the SimplificationPass in spirv-opt Issues with it were fixed (https://github.com/KhronosGroup/SPIRV-Cross/pull/2163), so it should be safe to reenable. The invalid glsl code seems to be resolved. --- libs/filamat/src/GLSLPostProcessor.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/libs/filamat/src/GLSLPostProcessor.cpp b/libs/filamat/src/GLSLPostProcessor.cpp index 3a4a4824e0e..bd879b1ee0c 100644 --- a/libs/filamat/src/GLSLPostProcessor.cpp +++ b/libs/filamat/src/GLSLPostProcessor.cpp @@ -671,13 +671,15 @@ void GLSLPostProcessor::fixupClipDistance( // - triggers a crash on some Adreno drivers (b/291140208, b/289401984, b/289393290) // However Metal requires this pass in order to correctly generate half-precision MSL // -// CreateSimplificationPass() creates a lot of problems: +// Note: CreateSimplificationPass() used to creates a lot of problems: // - Adreno GPU show artifacts after running simplification passes (Vulkan) // - spirv-cross fails generating working glsl // (https://github.com/KhronosGroup/SPIRV-Cross/issues/2162) -// - generally it makes the code more complicated, e.g.: replacing for loops with -// while-if-break, unclear if it helps for anything. -// However, the simplification passes below are necessary when targeting Metal, otherwise the +// +// However this problem was addressed in spirv-cross here: +// https://github.com/KhronosGroup/SPIRV-Cross/pull/2163 +// +// The simplification passes below are necessary when targeting Metal, otherwise the // result is mismatched half / float assignments in MSL. @@ -710,11 +712,11 @@ void GLSLPostProcessor::registerPerformancePasses(Optimizer& optimizer, Config c RegisterPass(CreateAggressiveDCEPass()); RegisterPass(CreateRedundancyEliminationPass()); RegisterPass(CreateCombineAccessChainsPass()); - RegisterPass(CreateSimplificationPass(), MaterialBuilder::TargetApi::METAL); + RegisterPass(CreateSimplificationPass()); RegisterPass(CreateVectorDCEPass()); RegisterPass(CreateDeadInsertElimPass()); RegisterPass(CreateDeadBranchElimPass()); - RegisterPass(CreateSimplificationPass(), MaterialBuilder::TargetApi::METAL); + RegisterPass(CreateSimplificationPass()); RegisterPass(CreateIfConversionPass()); RegisterPass(CreateCopyPropagateArraysPass()); RegisterPass(CreateReduceLoadSizePass()); @@ -723,7 +725,7 @@ void GLSLPostProcessor::registerPerformancePasses(Optimizer& optimizer, Config c RegisterPass(CreateRedundancyEliminationPass()); RegisterPass(CreateDeadBranchElimPass()); RegisterPass(CreateBlockMergePass()); - RegisterPass(CreateSimplificationPass(), MaterialBuilder::TargetApi::METAL); + RegisterPass(CreateSimplificationPass()); } void GLSLPostProcessor::registerSizePasses(Optimizer& optimizer, Config const& config) { From 5e7106b521121a3c086bd30b815b51e505290d9b Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 20 Aug 2024 17:28:52 -0700 Subject: [PATCH 09/26] custom variables can now have their precision specified e.g.: variables : [ { name : vertex, precision : medium, } ], --- docs/Materials.md.html | 13 +++++- .../filamat/include/filamat/MaterialBuilder.h | 11 ++++- libs/filamat/src/MaterialBuilder.cpp | 18 +++++++- libs/filamat/src/shaders/CodeGenerator.cpp | 13 ++++-- libs/filamat/src/shaders/CodeGenerator.h | 2 +- tools/matc/src/matc/ParametersProcessor.cpp | 43 +++++++++++++++++-- 6 files changed, 87 insertions(+), 13 deletions(-) diff --git a/docs/Materials.md.html b/docs/Materials.md.html index 85428dbba9c..753e3a6b2f5 100644 --- a/docs/Materials.md.html +++ b/docs/Materials.md.html @@ -1308,7 +1308,12 @@ declare a variable called `eyeDirection` you can access it in the fragment shader using `variable_eyeDirection`. In the vertex shader, the interpolant name is simply a member of the `MaterialVertexInputs` structure (`material.eyeDirection` in your example). Each - interpolant is of type `float4` (`vec4`) in the shaders. + interpolant is of type `float4` (`vec4`) in the shaders. By default the precision of the + interpolant is `highp` in *both* the vertex and fragment shaders. + An alternate syntax can be used to specify both the name and precision of the interpolant. + In this case the specified precision is used as-is in both fragment and vertex stages, in + particular if `default` is specified the default precision is used is the fragment shader + (`mediump`) and in the vertex shader (`highp`). ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ JSON material { @@ -1320,7 +1325,11 @@ } ], variables : [ - eyeDirection + eyeDirection, + { + name : eyeColor, + precision : medium + } ], vertexDomain : device, depthWrite : false, diff --git a/libs/filamat/include/filamat/MaterialBuilder.h b/libs/filamat/include/filamat/MaterialBuilder.h index 2587adba659..7a7e3ba2e2d 100644 --- a/libs/filamat/include/filamat/MaterialBuilder.h +++ b/libs/filamat/include/filamat/MaterialBuilder.h @@ -323,6 +323,9 @@ class UTILS_PUBLIC MaterialBuilder : public MaterialBuilderBase { //! Custom variables (all float4). MaterialBuilder& variable(Variable v, const char* name) noexcept; + MaterialBuilder& variable(Variable v, const char* name, + ParameterPrecision precision) noexcept; + /** * Require a specified attribute. * @@ -708,11 +711,17 @@ class UTILS_PUBLIC MaterialBuilder : public MaterialBuilderBase { ShaderStage stage; }; + struct CustomVariable { + utils::CString name; + Precision precision; + bool hasPrecision; + }; + static constexpr size_t MATERIAL_PROPERTIES_COUNT = filament::MATERIAL_PROPERTIES_COUNT; using Property = filament::Property; using PropertyList = bool[MATERIAL_PROPERTIES_COUNT]; - using VariableList = utils::CString[MATERIAL_VARIABLES_COUNT]; + using VariableList = CustomVariable[MATERIAL_VARIABLES_COUNT]; using OutputList = std::vector; static constexpr size_t MAX_COLOR_OUTPUT = filament::backend::MRT::MAX_SUPPORTED_RENDER_TARGET_COUNT; diff --git a/libs/filamat/src/MaterialBuilder.cpp b/libs/filamat/src/MaterialBuilder.cpp index 35add4f503f..1905c86cc00 100644 --- a/libs/filamat/src/MaterialBuilder.cpp +++ b/libs/filamat/src/MaterialBuilder.cpp @@ -234,7 +234,21 @@ MaterialBuilder& MaterialBuilder::variable(Variable v, const char* name) noexcep case Variable::CUSTOM2: case Variable::CUSTOM3: assert(size_t(v) < MATERIAL_VARIABLES_COUNT); - mVariables[size_t(v)] = CString(name); + mVariables[size_t(v)] = { CString(name), Precision::DEFAULT, false }; + break; + } + return *this; +} + +MaterialBuilder& MaterialBuilder::variable(Variable v, + const char* name, ParameterPrecision precision) noexcept { + switch (v) { + case Variable::CUSTOM0: + case Variable::CUSTOM1: + case Variable::CUSTOM2: + case Variable::CUSTOM3: + assert(size_t(v) < MATERIAL_VARIABLES_COUNT); + mVariables[size_t(v)] = { CString(name), precision, true }; break; } return *this; @@ -1383,7 +1397,7 @@ bool MaterialBuilder::checkMaterialLevelFeatures(MaterialInfo const& info) const bool MaterialBuilder::hasCustomVaryings() const noexcept { for (const auto& variable : mVariables) { - if (!variable.empty()) { + if (!variable.name.empty()) { return true; } } diff --git a/libs/filamat/src/shaders/CodeGenerator.cpp b/libs/filamat/src/shaders/CodeGenerator.cpp index 1c74cbd7797..2b90c299410 100644 --- a/libs/filamat/src/shaders/CodeGenerator.cpp +++ b/libs/filamat/src/shaders/CodeGenerator.cpp @@ -451,15 +451,20 @@ io::sstream& CodeGenerator::generatePostProcessMain(io::sstream& out, ShaderStag } io::sstream& CodeGenerator::generateVariable(io::sstream& out, ShaderStage stage, - const CString& name, size_t index) { - + const MaterialBuilder::CustomVariable& variable, size_t index) { + auto const& name = variable.name; + const char* precisionString = getPrecisionQualifier(variable.precision); if (!name.empty()) { if (stage == ShaderStage::VERTEX) { out << "\n#define VARIABLE_CUSTOM" << index << " " << name.c_str() << "\n"; out << "\n#define VARIABLE_CUSTOM_AT" << index << " variable_" << name.c_str() << "\n"; - out << "LAYOUT_LOCATION(" << index << ") VARYING vec4 variable_" << name.c_str() << ";\n"; + out << "LAYOUT_LOCATION(" << index << ") VARYING " << precisionString << " vec4 variable_" << name.c_str() << ";\n"; } else if (stage == ShaderStage::FRAGMENT) { - out << "\nLAYOUT_LOCATION(" << index << ") VARYING highp vec4 variable_" << name.c_str() << ";\n"; + if (!variable.hasPrecision && variable.precision == Precision::DEFAULT) { + // for backward compatibility + precisionString = "highp"; + } + out << "\nLAYOUT_LOCATION(" << index << ") VARYING " << precisionString << " vec4 variable_" << name.c_str() << ";\n"; } } return out; diff --git a/libs/filamat/src/shaders/CodeGenerator.h b/libs/filamat/src/shaders/CodeGenerator.h index 233243f348c..f7bb4af5ebb 100644 --- a/libs/filamat/src/shaders/CodeGenerator.h +++ b/libs/filamat/src/shaders/CodeGenerator.h @@ -103,7 +103,7 @@ class UTILS_PRIVATE CodeGenerator { // generate declarations for custom interpolants static utils::io::sstream& generateVariable(utils::io::sstream& out, ShaderStage stage, - const utils::CString& name, size_t index); + const MaterialBuilder::CustomVariable& variable, size_t index); // generate declarations for non-custom "in" variables utils::io::sstream& generateShaderInputs(utils::io::sstream& out, ShaderStage type, diff --git a/tools/matc/src/matc/ParametersProcessor.cpp b/tools/matc/src/matc/ParametersProcessor.cpp index 828597e51a6..34bbc2d72a3 100644 --- a/tools/matc/src/matc/ParametersProcessor.cpp +++ b/tools/matc/src/matc/ParametersProcessor.cpp @@ -615,14 +615,51 @@ static bool processVariables(MaterialBuilder& builder, const JsonishValue& value } for (size_t i = 0; i < elements.size(); i++) { - auto elementValue = elements[i]; + ParameterPrecision precision = ParameterPrecision::DEFAULT; MaterialBuilder::Variable v = intToVariable(i); - if (elementValue->getType() != JsonishValue::Type::STRING) { + std::string nameString; + + auto elementValue = elements[i]; + if (elementValue->getType() == JsonishValue::Type::OBJECT) { + + JsonishObject const& jsonObject = *elementValue->toJsonObject(); + + const JsonishValue* nameValue = jsonObject.getValue("name"); + if (!nameValue) { + std::cerr << "variables: entry without 'name' key." << std::endl; + return false; + } + if (nameValue->getType() != JsonishValue::STRING) { + std::cerr << "variables: name value must be STRING." << std::endl; + return false; + } + + const JsonishValue* precisionValue = jsonObject.getValue("precision"); + if (precisionValue) { + if (precisionValue->getType() != JsonishValue::STRING) { + std::cerr << "variables: precision must be a STRING." << std::endl; + return false; + } + auto precisionString = precisionValue->toJsonString(); + if (!Enums::isValid(precisionString->getString())){ + return logEnumIssue("variables", *precisionString, Enums::map()); + } + } + + nameString = nameValue->toJsonString()->getString(); + if (precisionValue) { + precision = Enums::toEnum( + precisionValue->toJsonString()->getString()); + } + builder.variable(v, nameString.c_str(), precision); + } else if (elementValue->getType() == JsonishValue::Type::STRING) { + nameString = elementValue->toJsonString()->getString(); + builder.variable(v, nameString.c_str()); + } else { std::cerr << "variables: array index " << i << " is not a STRING. found:" << JsonishValue::typeToString(elementValue->getType()) << std::endl; return false; } - builder.variable(v, elementValue->toJsonString()->getString().c_str()); } return true; From 4ae7aa6a1bf2cff5feb89a8bda988c9d87078668 Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Thu, 22 Aug 2024 13:39:04 -0700 Subject: [PATCH 10/26] Fix compatibility warning for multiview (#8075) Verify the compatibility between a compiled material and the engine's setting only when the engine is set up for stereo. Default materials are always compiled with either 'instanced' or 'multiview. Therefore Filament will emit warnings unintentionally if the engine is set up for stereo. This commit fixes it. --- filament/src/details/Material.cpp | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/filament/src/details/Material.cpp b/filament/src/details/Material.cpp index 69c1e529ff2..17e5d463346 100644 --- a/filament/src/details/Material.cpp +++ b/filament/src/details/Material.cpp @@ -192,19 +192,27 @@ Material* Material::Builder::build(Engine& engine) { return nullptr; } + // Print a warning if the material's stereo type doesn't align with the engine's setting. MaterialDomain materialDomain; + UserVariantFilterMask variantFilterMask; materialParser->getMaterialDomain(&materialDomain); - if (materialDomain == MaterialDomain::SURFACE) { + materialParser->getMaterialVariantFilterMask(&variantFilterMask); + bool const hasStereoVariants = !(variantFilterMask & UserVariantFilterMask(UserVariantFilterBit::STE)); + if (materialDomain == MaterialDomain::SURFACE && hasStereoVariants) { StereoscopicType const engineStereoscopicType = engine.getConfig().stereoscopicType; - StereoscopicType materialStereoscopicType = StereoscopicType::NONE; - materialParser->getStereoscopicType(&materialStereoscopicType); - if (materialStereoscopicType != engineStereoscopicType) { - CString name; - materialParser->getName(&name); - slog.w << "The stereoscopic type in the compiled material '" << name.c_str_safe() - << "' is " << (int)materialStereoscopicType - << ", which is not compatiable with the engine's setting " - << (int)engineStereoscopicType << "." << io::endl; + // Default materials are always compiled with either 'instanced' or 'multiview'. + // So, we only verify compatibility if the engine is set up for stereo. + if (engineStereoscopicType != StereoscopicType::NONE) { + StereoscopicType materialStereoscopicType = StereoscopicType::NONE; + materialParser->getStereoscopicType(&materialStereoscopicType); + if (materialStereoscopicType != engineStereoscopicType) { + CString name; + materialParser->getName(&name); + slog.w << "The stereoscopic type in the compiled material '" << name.c_str_safe() + << "' is " << (int)materialStereoscopicType + << ", which is not compatiable with the engine's setting " + << (int)engineStereoscopicType << "." << io::endl; + } } } From be22e9305deff4d3296f319cdf9730ebff2b3c66 Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Fri, 23 Aug 2024 09:09:56 -0700 Subject: [PATCH 11/26] matc: initialize CustomVariable (#8076) The default values were not provided via default construction. Caused matc to crash on Linux. --- libs/filamat/include/filamat/MaterialBuilder.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/filamat/include/filamat/MaterialBuilder.h b/libs/filamat/include/filamat/MaterialBuilder.h index 7a7e3ba2e2d..767bdecc6fd 100644 --- a/libs/filamat/include/filamat/MaterialBuilder.h +++ b/libs/filamat/include/filamat/MaterialBuilder.h @@ -713,8 +713,8 @@ class UTILS_PUBLIC MaterialBuilder : public MaterialBuilderBase { struct CustomVariable { utils::CString name; - Precision precision; - bool hasPrecision; + Precision precision = Precision::DEFAULT; + bool hasPrecision = false; }; static constexpr size_t MATERIAL_PROPERTIES_COUNT = filament::MATERIAL_PROPERTIES_COUNT; From b058794dd177d5f4a7c14de505abac5b62c4c797 Mon Sep 17 00:00:00 2001 From: Balaji M Date: Wed, 21 Aug 2024 13:33:46 -0400 Subject: [PATCH 12/26] decoding meshoptimized gltf just once --- libs/gltfio/src/extended/AssetLoaderExtended.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/gltfio/src/extended/AssetLoaderExtended.cpp b/libs/gltfio/src/extended/AssetLoaderExtended.cpp index 19ee1eed77a..4ac5868f2a5 100644 --- a/libs/gltfio/src/extended/AssetLoaderExtended.cpp +++ b/libs/gltfio/src/extended/AssetLoaderExtended.cpp @@ -527,11 +527,11 @@ bool AssetLoaderExtended::createPrimitive(Input* input, Output* out, if (!mCgltfBuffersLoaded) { mCgltfBuffersLoaded = utility::loadCgltfBuffers(gltf, mGltfPath.c_str(), mUriDataCache); + utility::decodeMeshoptCompression(gltf); if (!mCgltfBuffersLoaded) return false; } utility::decodeDracoMeshes(gltf, prim, input->dracoCache); - utility::decodeMeshoptCompression(gltf); auto slots = computeGeometries(prim, jobType, attributesMap, morphTargets, out->uvmap, mEngine); From 485c05789bef129f2d1b294e9f48593771f39842 Mon Sep 17 00:00:00 2001 From: Balaji M Date: Wed, 21 Aug 2024 14:32:54 -0400 Subject: [PATCH 13/26] return statement moved to separate line Co-authored-by: Powei Feng --- libs/gltfio/src/extended/AssetLoaderExtended.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libs/gltfio/src/extended/AssetLoaderExtended.cpp b/libs/gltfio/src/extended/AssetLoaderExtended.cpp index 4ac5868f2a5..37434783911 100644 --- a/libs/gltfio/src/extended/AssetLoaderExtended.cpp +++ b/libs/gltfio/src/extended/AssetLoaderExtended.cpp @@ -527,8 +527,10 @@ bool AssetLoaderExtended::createPrimitive(Input* input, Output* out, if (!mCgltfBuffersLoaded) { mCgltfBuffersLoaded = utility::loadCgltfBuffers(gltf, mGltfPath.c_str(), mUriDataCache); + if (!mCgltfBuffersLoaded) { + return false; + } utility::decodeMeshoptCompression(gltf); - if (!mCgltfBuffersLoaded) return false; } utility::decodeDracoMeshes(gltf, prim, input->dracoCache); From 9674a5ae3c37e3b7443ef386f38157a1e30f23b2 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Thu, 22 Aug 2024 13:56:34 -0700 Subject: [PATCH 14/26] add support for subresources in "blit" This is needed for debugging, but is a pretty low-hanging fruit. --- filament/src/PostProcessManager.cpp | 16 ++++++++++++---- filament/src/materials/blitArray.mat | 6 +++++- filament/src/materials/blitLow.mat | 6 +++++- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/filament/src/PostProcessManager.cpp b/filament/src/PostProcessManager.cpp index cc15f8b8952..18b6ab30dc7 100644 --- a/filament/src/PostProcessManager.cpp +++ b/filament/src/PostProcessManager.cpp @@ -2945,6 +2945,10 @@ FrameGraphId PostProcessManager::upscale(FrameGraph& fg, bool 1.0f / outputDesc.height}); } + if (blitterNames[index] == "blitLow") { + mi->setParameter("levelOfDetail", 0.0f); + } + mi->setParameter("viewport", float4{ (float)vp.left / inputDesc.width, (float)vp.bottom / inputDesc.height, @@ -3003,9 +3007,8 @@ FrameGraphId PostProcessManager::blit(FrameGraph& fg, bool tr SamplerMagFilter filterMag, SamplerMinFilter filterMin) noexcept { - // TODO: add support for sub-resources - assert_invariant(fg.getSubResourceDescriptor(input).layer == 0); - assert_invariant(fg.getSubResourceDescriptor(input).level == 0); + uint32_t const layer = fg.getSubResourceDescriptor(input).layer; + float const levelOfDetail = fg.getSubResourceDescriptor(input).level; struct QuadBlitData { FrameGraphId input; @@ -3031,7 +3034,8 @@ FrameGraphId PostProcessManager::blit(FrameGraph& fg, bool tr // -------------------------------------------------------------------------------- // set uniforms - PostProcessMaterial const& material = getPostProcessMaterial("blitLow"); + PostProcessMaterial const& material = + getPostProcessMaterial(layer ? "blitArray" : "blitLow"); auto* mi = material.getMaterialInstance(mEngine); mi->setParameter("color", color, { .filterMag = filterMag, @@ -3043,6 +3047,10 @@ FrameGraphId PostProcessManager::blit(FrameGraph& fg, bool tr float(vp.width) / inputDesc.width, float(vp.height) / inputDesc.height }); + mi->setParameter("levelOfDetail", levelOfDetail); + if (layer) { + mi->setParameter("layerIndex", layer); + } mi->commit(driver); mi->use(driver); diff --git a/filament/src/materials/blitArray.mat b/filament/src/materials/blitArray.mat index 2d7cfc4da3b..39d3a18e534 100644 --- a/filament/src/materials/blitArray.mat +++ b/filament/src/materials/blitArray.mat @@ -10,6 +10,10 @@ material { type : int, name : layerIndex }, + { + type : float, + name : levelOfDetail, + }, { type : float4, name : viewport, @@ -33,6 +37,6 @@ vertex { fragment { void postProcess(inout PostProcessInputs postProcess) { - postProcess.color = textureLod(materialParams_color, vec3(variable_vertex.xy, materialParams.layerIndex), 0.0); + postProcess.color = textureLod(materialParams_color, vec3(variable_vertex.xy, materialParams.layerIndex), materialParams.levelOfDetail); } } diff --git a/filament/src/materials/blitLow.mat b/filament/src/materials/blitLow.mat index 3ed03983c9d..16148577655 100644 --- a/filament/src/materials/blitLow.mat +++ b/filament/src/materials/blitLow.mat @@ -10,6 +10,10 @@ material { type : float4, name : viewport, precision: high + }, + { + type : float, + name : levelOfDetail, } ], variables : [ @@ -33,7 +37,7 @@ fragment { #if FILAMENT_EFFECTIVE_VERSION == 100 postProcess.color = texture2D(materialParams_color, variable_vertex.xy); #else - postProcess.color = textureLod(materialParams_color, variable_vertex.xy, 0.0); + postProcess.color = textureLod(materialParams_color, variable_vertex.xy, materialParams.levelOfDetail); #endif } } From 6f20cf4b02f9caf5bc2674c2598ad881fd4d688b Mon Sep 17 00:00:00 2001 From: Ben Doherty Date: Sat, 24 Aug 2024 09:17:05 -0700 Subject: [PATCH 15/26] Support tagging driver handles with a name (#8038) --- NEW_RELEASE_NOTES.md | 2 + filament/CMakeLists.txt | 1 + .../include/private/backend/DriverAPI.inc | 4 ++ .../include/private/backend/HandleAllocator.h | 38 +++++++++++++++++-- filament/backend/src/HandleAllocator.cpp | 3 ++ filament/backend/src/metal/MetalDriver.mm | 4 ++ filament/backend/src/noop/NoopDriver.cpp | 3 ++ filament/backend/src/opengl/OpenGLDriver.cpp | 4 ++ filament/backend/src/vulkan/VulkanDriver.cpp | 4 ++ .../src/vulkan/VulkanResourceAllocator.h | 4 ++ filament/include/filament/BufferObject.h | 17 ++++++++- filament/include/filament/FilamentAPI.h | 17 +++++++++ filament/include/filament/IndexBuffer.h | 17 ++++++++- filament/include/filament/InstanceBuffer.h | 17 ++++++++- filament/include/filament/MorphTargetBuffer.h | 17 ++++++++- filament/include/filament/SkinningBuffer.h | 17 ++++++++- filament/include/filament/Stream.h | 17 ++++++++- filament/include/filament/Texture.h | 17 ++++++++- filament/include/filament/VertexBuffer.h | 17 ++++++++- filament/src/FilamentBuilder.cpp | 31 +++++++++++++++ filament/src/components/RenderableManager.cpp | 3 ++ filament/src/details/BufferObject.cpp | 5 +++ filament/src/details/IndexBuffer.cpp | 5 +++ filament/src/details/InstanceBuffer.cpp | 3 +- filament/src/details/InstanceBuffer.h | 4 ++ filament/src/details/Material.cpp | 1 + filament/src/details/MaterialInstance.cpp | 2 + filament/src/details/MorphTargetBuffer.cpp | 7 ++++ filament/src/details/SkinningBuffer.cpp | 7 ++++ filament/src/details/Stream.cpp | 6 ++- filament/src/details/Texture.cpp | 3 ++ filament/src/details/VertexBuffer.cpp | 11 +++++- 32 files changed, 294 insertions(+), 14 deletions(-) create mode 100644 filament/src/FilamentBuilder.cpp diff --git a/NEW_RELEASE_NOTES.md b/NEW_RELEASE_NOTES.md index 4a1a9c7fa7e..01d998de3c5 100644 --- a/NEW_RELEASE_NOTES.md +++ b/NEW_RELEASE_NOTES.md @@ -7,3 +7,5 @@ for next branch cut* header. appropriate header in [RELEASE_NOTES.md](./RELEASE_NOTES.md). ## Release notes for next branch cut + +- Add a `name` API to Filament objects for debugging handle use-after-free assertions diff --git a/filament/CMakeLists.txt b/filament/CMakeLists.txt index 796acedb7d2..18f402ad9e7 100644 --- a/filament/CMakeLists.txt +++ b/filament/CMakeLists.txt @@ -61,6 +61,7 @@ set(SRCS src/Engine.cpp src/Exposure.cpp src/Fence.cpp + src/FilamentBuilder.cpp src/FrameInfo.cpp src/FrameSkipper.cpp src/Froxelizer.cpp diff --git a/filament/backend/include/private/backend/DriverAPI.inc b/filament/backend/include/private/backend/DriverAPI.inc index d27cea8f4c8..283e3e0e5ca 100644 --- a/filament/backend/include/private/backend/DriverAPI.inc +++ b/filament/backend/include/private/backend/DriverAPI.inc @@ -162,6 +162,10 @@ DECL_DRIVER_API_0(finish) // reset state tracking, if the driver does any state tracking (e.g. GL) DECL_DRIVER_API_0(resetState) +DECL_DRIVER_API_N(setDebugTag, + backend::HandleBase::HandleId, handleId, + utils::CString, tag) + /* * Creating driver objects * ----------------------- diff --git a/filament/backend/include/private/backend/HandleAllocator.h b/filament/backend/include/private/backend/HandleAllocator.h index 9c31b594875..b61f8e376f0 100644 --- a/filament/backend/include/private/backend/HandleAllocator.h +++ b/filament/backend/include/private/backend/HandleAllocator.h @@ -20,11 +20,12 @@ #include #include +#include #include +#include #include #include #include -#include #include @@ -173,8 +174,10 @@ class HandleAllocator { uint8_t const age = (tag & HANDLE_AGE_MASK) >> HANDLE_AGE_SHIFT; auto const pNode = static_cast(p); uint8_t const expectedAge = pNode[-1].age; - FILAMENT_CHECK_POSTCONDITION(expectedAge == age) << - "use-after-free of Handle with id=" << handle.getId(); + // getHandleTag() is only called if the check fails. + FILAMENT_CHECK_POSTCONDITION(expectedAge == age) + << "use-after-free of Handle with id=" << handle.getId() + << ", tag=" << getHandleTag(handle.getId()).c_str_safe(); } } @@ -201,6 +204,34 @@ class HandleAllocator { return handle_cast(const_cast&>(handle)); } + void associateTagToHandle(HandleBase::HandleId id, utils::CString&& tag) noexcept { + // TODO: for now, only pool handles check for use-after-free, so we only keep tags for + // those + if (isPoolHandle(id)) { + // Truncate the tag's age to N bits. + constexpr uint8_t N = 2; // support a history of 4 tags + constexpr uint8_t mask = (1 << N) - 1; + + uint8_t const age = (id & HANDLE_AGE_MASK) >> HANDLE_AGE_SHIFT; + uint8_t const newAge = age & mask; + uint32_t const key = (id & ~HANDLE_AGE_MASK) | (newAge << HANDLE_AGE_SHIFT); + + // This line is the costly part. In the future, we could potentially use a custom + // allocator. + mDebugTags[key] = std::move(tag); + } + } + + utils::CString getHandleTag(HandleBase::HandleId id) const noexcept { + if (!isPoolHandle(id)) { + return "(no tag)"; + } + if (auto pos = mDebugTags.find(id); pos != mDebugTags.end()) { + return pos->second; + } + return "(no tag)"; + } + private: template @@ -363,6 +394,7 @@ class HandleAllocator { // Below is only used when running out of space in the HandleArena mutable utils::Mutex mLock; tsl::robin_map mOverflowMap; + tsl::robin_map mDebugTags; HandleBase::HandleId mId = 0; bool mUseAfterFreeCheckDisabled = false; }; diff --git a/filament/backend/src/HandleAllocator.cpp b/filament/backend/src/HandleAllocator.cpp index 07d028d9d49..6cdcaa4af4f 100644 --- a/filament/backend/src/HandleAllocator.cpp +++ b/filament/backend/src/HandleAllocator.cpp @@ -80,6 +80,9 @@ HandleAllocator::HandleAllocator(const char* name, size_t size, bool disableUseAfterFreeCheck) noexcept : mHandleArena(name, size, disableUseAfterFreeCheck), mUseAfterFreeCheckDisabled(disableUseAfterFreeCheck) { + // Reserve initial space for debug tags. This prevents excessive calls to malloc when the first + // few tags are set. + mDebugTags.reserve(512); } template diff --git a/filament/backend/src/metal/MetalDriver.mm b/filament/backend/src/metal/MetalDriver.mm index 397a1599336..3022e7381b3 100644 --- a/filament/backend/src/metal/MetalDriver.mm +++ b/filament/backend/src/metal/MetalDriver.mm @@ -2072,6 +2072,10 @@ void MetalDriver::resetState(int) { } +void MetalDriver::setDebugTag(HandleBase::HandleId handleId, utils::CString tag) { + mHandleAllocator.associateTagToHandle(handleId, std::move(tag)); +} + void MetalDriver::runAtNextTick(const std::function& fn) noexcept { std::lock_guard const lock(mTickOpsLock); mTickOps.push_back(fn); diff --git a/filament/backend/src/noop/NoopDriver.cpp b/filament/backend/src/noop/NoopDriver.cpp index a692cfce834..096f57c3989 100644 --- a/filament/backend/src/noop/NoopDriver.cpp +++ b/filament/backend/src/noop/NoopDriver.cpp @@ -392,4 +392,7 @@ void NoopDriver::endTimerQuery(Handle tqh) { void NoopDriver::resetState(int) { } +void NoopDriver::setDebugTag(HandleBase::HandleId handleId, utils::CString tag) { +} + } // namespace filament diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp index fdf567849e8..ed23067bb87 100644 --- a/filament/backend/src/opengl/OpenGLDriver.cpp +++ b/filament/backend/src/opengl/OpenGLDriver.cpp @@ -2222,6 +2222,10 @@ void OpenGLDriver::makeCurrent(Handle schDraw, Handle // Updating driver objects // ------------------------------------------------------------------------------------------------ +void OpenGLDriver::setDebugTag(HandleBase::HandleId handleId, utils::CString tag) { + mHandleAllocator.associateTagToHandle(handleId, std::move(tag)); +} + void OpenGLDriver::setVertexBufferObject(Handle vbh, uint32_t index, Handle boh) { DEBUG_MARKER() diff --git a/filament/backend/src/vulkan/VulkanDriver.cpp b/filament/backend/src/vulkan/VulkanDriver.cpp index 46020676630..0b444c0b75e 100644 --- a/filament/backend/src/vulkan/VulkanDriver.cpp +++ b/filament/backend/src/vulkan/VulkanDriver.cpp @@ -2038,6 +2038,10 @@ void VulkanDriver::debugCommandBegin(CommandStream* cmds, bool synchronous, cons void VulkanDriver::resetState(int) { } +void VulkanDriver::setDebugTag(HandleBase::HandleId handleId, utils::CString tag) { + mResourceAllocator.associateHandle(handleId, std::move(tag)); +} + // explicit instantiation of the Dispatcher template class ConcreteDispatcher; diff --git a/filament/backend/src/vulkan/VulkanResourceAllocator.h b/filament/backend/src/vulkan/VulkanResourceAllocator.h index 04187658b7d..572aeed4f78 100644 --- a/filament/backend/src/vulkan/VulkanResourceAllocator.h +++ b/filament/backend/src/vulkan/VulkanResourceAllocator.h @@ -105,6 +105,10 @@ class VulkanResourceAllocator { mHandleAllocatorImpl.deallocate(handle, obj); } + inline void associateHandle(HandleBase::HandleId id, utils::CString&& tag) noexcept { + mHandleAllocatorImpl.associateTagToHandle(id, std::move(tag)); + } + private: AllocatorImpl mHandleAllocatorImpl; diff --git a/filament/include/filament/BufferObject.h b/filament/include/filament/BufferObject.h index 74a4b1ff390..8c76719c85e 100644 --- a/filament/include/filament/BufferObject.h +++ b/filament/include/filament/BufferObject.h @@ -54,7 +54,7 @@ class UTILS_PUBLIC BufferObject : public FilamentAPI { using BufferDescriptor = backend::BufferDescriptor; using BindingType = backend::BufferObjectBinding; - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: Builder() noexcept; @@ -78,6 +78,21 @@ class UTILS_PUBLIC BufferObject : public FilamentAPI { */ Builder& bindingType(BindingType bindingType) noexcept; + /** + * Associate an optional name with this BufferObject for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this BufferObject + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the BufferObject and returns a pointer to it. After creation, the buffer * object is uninitialized. Use BufferObject::setBuffer() to initialize it. diff --git a/filament/include/filament/FilamentAPI.h b/filament/include/filament/FilamentAPI.h index 19d6ba246a9..7a6f16d6307 100644 --- a/filament/include/filament/FilamentAPI.h +++ b/filament/include/filament/FilamentAPI.h @@ -19,6 +19,7 @@ #include #include +#include #include @@ -54,6 +55,22 @@ class UTILS_PUBLIC FilamentAPI { template using BuilderBase = utils::PrivateImplementation; +void builderMakeName(utils::CString& outName, const char* name, size_t len) noexcept; + +template +class UTILS_PUBLIC BuilderNameMixin { +public: + Builder& name(const char* name, size_t len) noexcept { + builderMakeName(mName, name, len); + return static_cast(*this); + } + + utils::CString const& getName() const noexcept { return mName; } + +private: + utils::CString mName; +}; + } // namespace filament #endif // TNT_FILAMENT_FILAMENTAPI_H diff --git a/filament/include/filament/IndexBuffer.h b/filament/include/filament/IndexBuffer.h index 35b8a10ef26..ff29e1f44d2 100644 --- a/filament/include/filament/IndexBuffer.h +++ b/filament/include/filament/IndexBuffer.h @@ -59,7 +59,7 @@ class UTILS_PUBLIC IndexBuffer : public FilamentAPI { UINT = uint8_t(backend::ElementType::UINT), //!< 32-bit indices }; - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: Builder() noexcept; @@ -83,6 +83,21 @@ class UTILS_PUBLIC IndexBuffer : public FilamentAPI { */ Builder& bufferType(IndexType indexType) noexcept; + /** + * Associate an optional name with this IndexBuffer for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this IndexBuffer + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the IndexBuffer object and returns a pointer to it. After creation, the index * buffer is uninitialized. Use IndexBuffer::setBuffer() to initialize the IndexBuffer. diff --git a/filament/include/filament/InstanceBuffer.h b/filament/include/filament/InstanceBuffer.h index 2135152d883..843c7983bb7 100644 --- a/filament/include/filament/InstanceBuffer.h +++ b/filament/include/filament/InstanceBuffer.h @@ -38,7 +38,7 @@ class UTILS_PUBLIC InstanceBuffer : public FilamentAPI { struct BuilderDetails; public: - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: @@ -70,6 +70,21 @@ class UTILS_PUBLIC InstanceBuffer : public FilamentAPI { */ Builder& localTransforms(math::mat4f const* UTILS_NULLABLE localTransforms) noexcept; + /** + * Associate an optional name with this InstanceBuffer for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this InstanceBuffer + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the InstanceBuffer object and returns a pointer to it. */ diff --git a/filament/include/filament/MorphTargetBuffer.h b/filament/include/filament/MorphTargetBuffer.h index 655bb8d848d..cb77bb79c85 100644 --- a/filament/include/filament/MorphTargetBuffer.h +++ b/filament/include/filament/MorphTargetBuffer.h @@ -39,7 +39,7 @@ class UTILS_PUBLIC MorphTargetBuffer : public FilamentAPI { struct BuilderDetails; public: - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: Builder() noexcept; @@ -63,6 +63,21 @@ class UTILS_PUBLIC MorphTargetBuffer : public FilamentAPI { */ Builder& count(size_t count) noexcept; + /** + * Associate an optional name with this MorphTargetBuffer for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this MorphTargetBuffer + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the MorphTargetBuffer object and returns a pointer to it. * diff --git a/filament/include/filament/SkinningBuffer.h b/filament/include/filament/SkinningBuffer.h index 36ae30ed438..27b13191e54 100644 --- a/filament/include/filament/SkinningBuffer.h +++ b/filament/include/filament/SkinningBuffer.h @@ -39,7 +39,7 @@ class UTILS_PUBLIC SkinningBuffer : public FilamentAPI { struct BuilderDetails; public: - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: Builder() noexcept; @@ -69,6 +69,21 @@ class UTILS_PUBLIC SkinningBuffer : public FilamentAPI { */ Builder& initialize(bool initialize = true) noexcept; + /** + * Associate an optional name with this SkinningBuffer for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this SkinningBuffer + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the SkinningBuffer object and returns a pointer to it. * diff --git a/filament/include/filament/Stream.h b/filament/include/filament/Stream.h index 6cafbacc8b3..883a8ccab3c 100644 --- a/filament/include/filament/Stream.h +++ b/filament/include/filament/Stream.h @@ -94,7 +94,7 @@ class UTILS_PUBLIC Stream : public FilamentAPI { * * To create a NATIVE stream, call the
stream
method on the builder. */ - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: Builder() noexcept; @@ -136,6 +136,21 @@ class UTILS_PUBLIC Stream : public FilamentAPI { */ Builder& height(uint32_t height) noexcept; + /** + * Associate an optional name with this Stream for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this Stream + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the Stream object and returns a pointer to it. * diff --git a/filament/include/filament/Texture.h b/filament/include/filament/Texture.h index 8a27f831c2b..b351c429435 100644 --- a/filament/include/filament/Texture.h +++ b/filament/include/filament/Texture.h @@ -112,7 +112,7 @@ class UTILS_PUBLIC Texture : public FilamentAPI { //! Use Builder to construct a Texture object instance - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: Builder() noexcept; @@ -202,6 +202,21 @@ class UTILS_PUBLIC Texture : public FilamentAPI { */ Builder& swizzle(Swizzle r, Swizzle g, Swizzle b, Swizzle a) noexcept; + /** + * Associate an optional name with this Texture for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this Texture + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the Texture object and returns a pointer to it. * diff --git a/filament/include/filament/VertexBuffer.h b/filament/include/filament/VertexBuffer.h index fccbd0046f8..64106434abc 100644 --- a/filament/include/filament/VertexBuffer.h +++ b/filament/include/filament/VertexBuffer.h @@ -61,7 +61,7 @@ class UTILS_PUBLIC VertexBuffer : public FilamentAPI { using AttributeType = backend::ElementType; using BufferDescriptor = backend::BufferDescriptor; - class Builder : public BuilderBase { + class Builder : public BuilderBase, public BuilderNameMixin { friend struct BuilderDetails; public: Builder() noexcept; @@ -158,6 +158,21 @@ class UTILS_PUBLIC VertexBuffer : public FilamentAPI { */ Builder& advancedSkinning(bool enabled) noexcept; + /** + * Associate an optional name with this VertexBuffer for debugging purposes. + * + * name will show in error messages and should be kept as short as possible. The name is + * truncated to a maximum of 128 characters. + * + * The name string is copied during this method so clients may free its memory after + * the function returns. + * + * @param name A string to identify this VertexBuffer + * @param len Length of name, should be less than or equal to 128 + * @return This Builder, for chaining calls. + */ + // Builder& name(const char* UTILS_NONNULL name, size_t len) noexcept; // inherited + /** * Creates the VertexBuffer object and returns a pointer to it. * diff --git a/filament/src/FilamentBuilder.cpp b/filament/src/FilamentBuilder.cpp new file mode 100644 index 00000000000..08f1cb32318 --- /dev/null +++ b/filament/src/FilamentBuilder.cpp @@ -0,0 +1,31 @@ +/* + * Copyright (C) 2024 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 + +#include + +namespace filament { + +void builderMakeName(utils::CString& outName, const char* name, size_t len) noexcept { + if (!name) { + return; + } + size_t const length = std::min(len, size_t { 128u }); + outName = utils::CString(name, length); +} + +} // namespace filament diff --git a/filament/src/components/RenderableManager.cpp b/filament/src/components/RenderableManager.cpp index 6338d53b725..7b79477bcdc 100644 --- a/filament/src/components/RenderableManager.cpp +++ b/filament/src/components/RenderableManager.cpp @@ -588,6 +588,9 @@ void FRenderableManager::create( // full size of the UBO. instances.handle = driver.createBufferObject(sizeof(PerRenderableUib), BufferObjectBinding::UNIFORM, backend::BufferUsage::DYNAMIC); + if (auto name = instances.buffer->getName(); !name.empty()) { + driver.setDebugTag(instances.handle.getId(), std::move(name)); + } } const uint32_t boneCount = builder->mSkinningBoneCount; diff --git a/filament/src/details/BufferObject.cpp b/filament/src/details/BufferObject.cpp index 04a68223ba2..a462e42a423 100644 --- a/filament/src/details/BufferObject.cpp +++ b/filament/src/details/BufferObject.cpp @@ -20,6 +20,8 @@ #include "FilamentAPI-impl.h" +#include + namespace filament { struct BufferObject::BuilderDetails { @@ -56,6 +58,9 @@ FBufferObject::FBufferObject(FEngine& engine, const BufferObject::Builder& build FEngine::DriverApi& driver = engine.getDriverApi(); mHandle = driver.createBufferObject(builder->mByteCount, builder->mBindingType, backend::BufferUsage::STATIC); + if (auto name = builder.getName(); !name.empty()) { + driver.setDebugTag(mHandle.getId(), std::move(name)); + } } void FBufferObject::terminate(FEngine& engine) { diff --git a/filament/src/details/IndexBuffer.cpp b/filament/src/details/IndexBuffer.cpp index 66fc5d2f58c..ae881de358d 100644 --- a/filament/src/details/IndexBuffer.cpp +++ b/filament/src/details/IndexBuffer.cpp @@ -20,6 +20,8 @@ #include "FilamentAPI-impl.h" +#include + namespace filament { struct IndexBuffer::BuilderDetails { @@ -58,6 +60,9 @@ FIndexBuffer::FIndexBuffer(FEngine& engine, const IndexBuffer::Builder& builder) (backend::ElementType)builder->mIndexType, uint32_t(builder->mIndexCount), backend::BufferUsage::STATIC); + if (auto name = builder.getName(); !name.empty()) { + driver.setDebugTag(mHandle.getId(), std::move(name)); + } } void FIndexBuffer::terminate(FEngine& engine) { diff --git a/filament/src/details/InstanceBuffer.cpp b/filament/src/details/InstanceBuffer.cpp index af22554d0f2..89fdd0e5d9a 100644 --- a/filament/src/details/InstanceBuffer.cpp +++ b/filament/src/details/InstanceBuffer.cpp @@ -61,7 +61,8 @@ InstanceBuffer* InstanceBuffer::Builder::build(Engine& engine) { // ------------------------------------------------------------------------------------------------ -FInstanceBuffer::FInstanceBuffer(FEngine& engine, const Builder& builder) { +FInstanceBuffer::FInstanceBuffer(FEngine& engine, const Builder& builder) + : mName(builder.getName()) { mInstanceCount = builder->mInstanceCount; mLocalTransforms.reserve(mInstanceCount); diff --git a/filament/src/details/InstanceBuffer.h b/filament/src/details/InstanceBuffer.h index 2d25f7833e8..56bac500c2b 100644 --- a/filament/src/details/InstanceBuffer.h +++ b/filament/src/details/InstanceBuffer.h @@ -25,6 +25,7 @@ #include +#include #include namespace filament { @@ -46,10 +47,13 @@ class FInstanceBuffer : public InstanceBuffer { void prepare(FEngine& engine, math::mat4f rootTransform, const PerRenderableData& ubo, backend::Handle handle); + utils::CString const& getName() const noexcept { return mName; } + private: friend class RenderableManager; utils::FixedCapacityVector mLocalTransforms; + utils::CString mName; size_t mInstanceCount; }; diff --git a/filament/src/details/Material.cpp b/filament/src/details/Material.cpp index 17e5d463346..b27c6608731 100644 --- a/filament/src/details/Material.cpp +++ b/filament/src/details/Material.cpp @@ -618,6 +618,7 @@ Program FMaterial::getProgramWithVariants( void FMaterial::createAndCacheProgram(Program&& p, Variant variant) const noexcept { auto program = mEngine.getDriverApi().createProgram(std::move(p)); + mEngine.getDriverApi().setDebugTag(program.getId(), mName); assert_invariant(program); mCachedPrograms[variant.key] = program; } diff --git a/filament/src/details/MaterialInstance.cpp b/filament/src/details/MaterialInstance.cpp index 4595c7e6c3b..152ecd480a4 100644 --- a/filament/src/details/MaterialInstance.cpp +++ b/filament/src/details/MaterialInstance.cpp @@ -51,6 +51,7 @@ FMaterialInstance::FMaterialInstance(FEngine& engine, FMaterial const* material) mUniforms = UniformBuffer(material->getUniformInterfaceBlock().getSize()); mUbHandle = driver.createBufferObject(mUniforms.getSize(), BufferObjectBinding::UNIFORM, backend::BufferUsage::STATIC); + driver.setDebugTag(mUbHandle.getId(), material->getName()); } if (!material->getSamplerInterfaceBlock().isEmpty()) { @@ -116,6 +117,7 @@ FMaterialInstance::FMaterialInstance(FEngine& engine, mUniforms.setUniforms(other->getUniformBuffer()); mUbHandle = driver.createBufferObject(mUniforms.getSize(), BufferObjectBinding::UNIFORM, backend::BufferUsage::DYNAMIC); + driver.setDebugTag(mUbHandle.getId(), material->getName()); } if (!material->getSamplerInterfaceBlock().isEmpty()) { diff --git a/filament/src/details/MorphTargetBuffer.cpp b/filament/src/details/MorphTargetBuffer.cpp index d4124fb2a6b..d3323b92c3b 100644 --- a/filament/src/details/MorphTargetBuffer.cpp +++ b/filament/src/details/MorphTargetBuffer.cpp @@ -25,6 +25,8 @@ #include #include +#include + namespace filament { using namespace backend; @@ -124,6 +126,11 @@ FMorphTargetBuffer::FMorphTargetBuffer(FEngine& engine, const Builder& builder) mCount, TextureUsage::DEFAULT); + if (auto name = builder.getName(); !name.empty()) { + driver.setDebugTag(mPbHandle.getId(), name); + driver.setDebugTag(mTbHandle.getId(), std::move(name)); + } + // create and update sampler group mSbHandle = driver.createSamplerGroup(PerRenderPrimitiveMorphingSib::SAMPLER_COUNT, utils::FixedSizeString<32>("Morph target samplers")); diff --git a/filament/src/details/SkinningBuffer.cpp b/filament/src/details/SkinningBuffer.cpp index b7092638670..c4c79c3a992 100644 --- a/filament/src/details/SkinningBuffer.cpp +++ b/filament/src/details/SkinningBuffer.cpp @@ -27,6 +27,8 @@ #include #include +#include + #include namespace filament { @@ -80,6 +82,11 @@ FSkinningBuffer::FSkinningBuffer(FEngine& engine, const Builder& builder) BufferObjectBinding::UNIFORM, BufferUsage::DYNAMIC); + if (auto name = builder.getName(); !name.empty()) { + // TODO: We should also tag the texture created inside createIndicesAndWeightsHandle. + driver.setDebugTag(mHandle.getId(), std::move(name)); + } + if (builder->mInitialize) { // initialize the bones to identity (before rounding up) auto* out = driver.allocatePod(mBoneCount); diff --git a/filament/src/details/Stream.cpp b/filament/src/details/Stream.cpp index 317113ace03..c60bcf8bef3 100644 --- a/filament/src/details/Stream.cpp +++ b/filament/src/details/Stream.cpp @@ -23,10 +23,10 @@ #include +#include #include #include - namespace filament { using namespace backend; @@ -80,6 +80,10 @@ FStream::FStream(FEngine& engine, const Builder& builder) noexcept } else { mStreamHandle = engine.getDriverApi().createStreamAcquired(); } + + if (auto name = builder.getName(); !name.empty()) { + engine.getDriverApi().setDebugTag(mStreamHandle.getId(), std::move(name)); + } } void FStream::terminate(FEngine& engine) noexcept { diff --git a/filament/src/details/Texture.cpp b/filament/src/details/Texture.cpp index 3734df4b6d9..a59a24da97d 100644 --- a/filament/src/details/Texture.cpp +++ b/filament/src/details/Texture.cpp @@ -243,6 +243,9 @@ FTexture::FTexture(FEngine& engine, const Builder& builder) { mHandle = driver.importTexture(builder->mImportedId, mTarget, mLevelCount, mFormat, mSampleCount, mWidth, mHeight, mDepth, mUsage); } + if (auto name = builder.getName(); !name.empty()) { + driver.setDebugTag(mHandle.getId(), std::move(name)); + } } // frees driver resources, object becomes invalid diff --git a/filament/src/details/VertexBuffer.cpp b/filament/src/details/VertexBuffer.cpp index 6269bc5ed7b..8aabcf4fc79 100644 --- a/filament/src/details/VertexBuffer.cpp +++ b/filament/src/details/VertexBuffer.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -259,7 +260,9 @@ FVertexBuffer::FVertexBuffer(FEngine& engine, const VertexBuffer::Builder& build mBufferCount, mDeclaredAttributes.count(), mAttributes); mHandle = driver.createVertexBuffer(mVertexCount, mVertexBufferInfoHandle); - + if (auto name = builder.getName(); !name.empty()) { + driver.setDebugTag(mHandle.getId(), name); + } // calculate buffer sizes size_t bufferSizes[MAX_VERTEX_BUFFER_COUNT] = {}; @@ -287,6 +290,9 @@ FVertexBuffer::FVertexBuffer(FEngine& engine, const VertexBuffer::Builder& build if (!mBufferObjects[i]) { BufferObjectHandle bo = driver.createBufferObject(bufferSizes[i], backend::BufferObjectBinding::VERTEX, backend::BufferUsage::STATIC); + if (auto name = builder.getName(); !name.empty()) { + driver.setDebugTag(bo.getId(), name); + } driver.setVertexBufferObject(mHandle, i, bo); mBufferObjects[i] = bo; } @@ -303,6 +309,9 @@ FVertexBuffer::FVertexBuffer(FEngine& engine, const VertexBuffer::Builder& build if (!mBufferObjects[i]) { BufferObjectHandle const bo = driver.createBufferObject(bufferSizes[i], backend::BufferObjectBinding::VERTEX, backend::BufferUsage::STATIC); + if (auto name = builder.getName(); !name.empty()) { + driver.setDebugTag(bo.getId(), name); + } driver.setVertexBufferObject(mHandle, i, bo); mBufferObjects[i] = bo; } From be4391950dacae98d9d388dfef862305044f7390 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 23 Aug 2024 22:32:17 -0700 Subject: [PATCH 16/26] fix gltfio ubershaders default parameters were not initialized which could cause them to be incorrectly evaluated in the shader. this is actually a pretty crazy bug that has been around since forever and which we were "lucky" to not run into sooner. this was exposed with the specular extension that was added recently. --- libs/gltfio/src/MaterialProvider.cpp | 8 ++++++- libs/gltfio/src/UbershaderProvider.cpp | 30 ++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/libs/gltfio/src/MaterialProvider.cpp b/libs/gltfio/src/MaterialProvider.cpp index 7e7fa6d947e..7de92341524 100644 --- a/libs/gltfio/src/MaterialProvider.cpp +++ b/libs/gltfio/src/MaterialProvider.cpp @@ -56,7 +56,13 @@ bool operator==(const MaterialKey& k1, const MaterialKey& k2) { (k1.volumeThicknessUV == k2.volumeThicknessUV) && (k1.hasSheen == k2.hasSheen) && (k1.hasIOR == k2.hasIOR) && - (k1.hasVolume == k2.hasVolume); + (k1.hasVolume == k2.hasVolume) && + (k1.hasSpecular == k2.hasSpecular) && + (k1.hasSpecularTexture == k2.hasSpecularTexture) && + (k1.hasSpecularColorTexture == k2.hasSpecularColorTexture) && + (k1.specularTextureUV == k2.specularTextureUV) && + (k1.specularColorTextureUV == k2.specularColorTextureUV) + ; } // Filament supports up to 2 UV sets. glTF has arbitrary texcoord set indices, but it allows diff --git a/libs/gltfio/src/UbershaderProvider.cpp b/libs/gltfio/src/UbershaderProvider.cpp index 64451aa363c..d70c5ab6955 100644 --- a/libs/gltfio/src/UbershaderProvider.cpp +++ b/libs/gltfio/src/UbershaderProvider.cpp @@ -259,6 +259,24 @@ MaterialInstance* UbershaderProvider::createMaterialInstance(MaterialKey* config mi->setParameter("occlusionUvMatrix", identity); mi->setParameter("emissiveUvMatrix", identity); + // set to -1 all possibly optional parameters + for (auto const* param : { + "clearCoatIndex", + "clearCoatRoughnessIndex", + "clearCoatNormalIndex", + "sheenColorIndex", + "sheenRoughnessIndex", + "volumeThicknessUvMatrix", + "volumeThicknessIndex", + "transmissionIndex", + "specularIndex", + "specularColorIndex", + }) { + if (material->hasParameter(param)) { + mi->setParameter(param, -1); + } + } + if (config->hasClearCoat) { mi->setParameter("clearCoatIndex", getUvIndex(config->clearCoatUV, config->hasClearCoatTexture)); @@ -340,10 +358,10 @@ MaterialInstance* UbershaderProvider::createMaterialInstance(MaterialKey* config mi->setParameter("sheenRoughnessMap", mDummyTexture, sampler); } - if (mi->getMaterial()->hasParameter("ior")) { + if (material->hasParameter("ior")) { mi->setParameter("ior", 1.5f); } - if (mi->getMaterial()->hasParameter("reflectance")) { + if (material->hasParameter("reflectance")) { mi->setParameter("reflectance", 0.5f); } @@ -355,6 +373,14 @@ MaterialInstance* UbershaderProvider::createMaterialInstance(MaterialKey* config mi->setParameter("specularColorMap", mDummyTexture, sampler); } + if (material->hasParameter("specularColorFactor")) { + mi->setParameter("specularColorFactor", float3(1.0f)); + } + + if (material->hasParameter("specularStrength")) { + mi->setParameter("specularStrength", 1.0f); + } + return mi; } From a7317e7a9922ba807230ca99fc2af6b8dae9a3a3 Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Mon, 26 Aug 2024 16:30:15 -0700 Subject: [PATCH 17/26] Revert two depth relevant changes (#8083) This reverts commits - b70aa43727a5bdaff16b484b8c4ddba7ab809500 "depth clamp cannot work with VSM" - 6c0bd360b309a46a603668531e85baa845000826 "Add support for depth clamp and use it for shadows" --- .../backend/include/backend/DriverEnums.h | 2 +- .../include/private/backend/DriverAPI.inc | 1 - filament/backend/src/metal/MetalContext.h | 1 - filament/backend/src/metal/MetalDriver.mm | 11 ------- filament/backend/src/metal/MetalState.h | 1 - filament/backend/src/noop/NoopDriver.cpp | 4 --- filament/backend/src/opengl/OpenGLContext.cpp | 2 -- filament/backend/src/opengl/OpenGLContext.h | 8 ++--- filament/backend/src/opengl/OpenGLDriver.cpp | 12 ------- filament/backend/src/opengl/gl_headers.h | 6 ---- filament/backend/src/vulkan/VulkanContext.h | 4 --- filament/backend/src/vulkan/VulkanDriver.cpp | 6 ---- .../src/vulkan/VulkanPipelineCache.cpp | 1 - .../backend/src/vulkan/VulkanPipelineCache.h | 4 +-- .../src/vulkan/platform/VulkanPlatform.cpp | 1 - filament/src/RenderPass.cpp | 11 ++----- filament/src/RenderPass.h | 3 +- filament/src/ShadowMap.cpp | 12 +++---- filament/src/ShadowMapManager.cpp | 31 +++---------------- filament/src/ShadowMapManager.h | 1 - filament/src/details/DebugRegistry.cpp | 24 +++++++++----- filament/src/details/Engine.h | 1 - samples/gltf_viewer.cpp | 2 -- 23 files changed, 35 insertions(+), 114 deletions(-) diff --git a/filament/backend/include/backend/DriverEnums.h b/filament/backend/include/backend/DriverEnums.h index 35cd89a57e7..8da0cfb02fc 100644 --- a/filament/backend/include/backend/DriverEnums.h +++ b/filament/backend/include/backend/DriverEnums.h @@ -1058,7 +1058,7 @@ struct RasterState { bool inverseFrontFaces : 1; // 31 //! padding, must be 0 - bool depthClamp : 1; // 32 + uint8_t padding : 1; // 32 }; uint32_t u = 0; }; diff --git a/filament/backend/include/private/backend/DriverAPI.inc b/filament/backend/include/private/backend/DriverAPI.inc index 283e3e0e5ca..99ea2d528eb 100644 --- a/filament/backend/include/private/backend/DriverAPI.inc +++ b/filament/backend/include/private/backend/DriverAPI.inc @@ -309,7 +309,6 @@ DECL_DRIVER_API_SYNCHRONOUS_0(bool, isParallelShaderCompileSupported) DECL_DRIVER_API_SYNCHRONOUS_0(bool, isDepthStencilResolveSupported) DECL_DRIVER_API_SYNCHRONOUS_N(bool, isDepthStencilBlitSupported, backend::TextureFormat, format) DECL_DRIVER_API_SYNCHRONOUS_0(bool, isProtectedTexturesSupported) -DECL_DRIVER_API_SYNCHRONOUS_0(bool, isDepthClampSupported) DECL_DRIVER_API_SYNCHRONOUS_0(uint8_t, getMaxDrawBuffers) DECL_DRIVER_API_SYNCHRONOUS_0(size_t, getMaxUniformBufferSize) DECL_DRIVER_API_SYNCHRONOUS_0(math::float2, getClipSpaceParams) diff --git a/filament/backend/src/metal/MetalContext.h b/filament/backend/src/metal/MetalContext.h index fc8cfc12b46..f771ca5b417 100644 --- a/filament/backend/src/metal/MetalContext.h +++ b/filament/backend/src/metal/MetalContext.h @@ -112,7 +112,6 @@ struct MetalContext { std::array ssboState; CullModeStateTracker cullModeState; WindingStateTracker windingState; - DepthClampStateTracker depthClampState; Handle currentRenderPrimitive; // State caches. diff --git a/filament/backend/src/metal/MetalDriver.mm b/filament/backend/src/metal/MetalDriver.mm index 3022e7381b3..b7d0a31deaa 100644 --- a/filament/backend/src/metal/MetalDriver.mm +++ b/filament/backend/src/metal/MetalDriver.mm @@ -834,10 +834,6 @@ return false; } -bool MetalDriver::isDepthClampSupported() { - return true; -} - bool MetalDriver::isWorkaroundNeeded(Workaround workaround) { switch (workaround) { case Workaround::SPLIT_EASU: @@ -1755,13 +1751,6 @@ [mContext->currentRenderPassEncoder setFrontFacingWinding:winding]; } - // depth clip mode - MTLDepthClipMode depthClipMode = rs.depthClamp ? MTLDepthClipModeClamp : MTLDepthClipModeClip; - mContext->depthClampState.updateState(depthClipMode); - if (mContext->depthClampState.stateChanged()) { - [mContext->currentRenderPassEncoder setDepthClipMode:depthClipMode]; - } - // Set the depth-stencil state, if a state change is needed. DepthStencilState depthState; if (depthAttachment) { diff --git a/filament/backend/src/metal/MetalState.h b/filament/backend/src/metal/MetalState.h index 1d541cc0065..83579cb5d50 100644 --- a/filament/backend/src/metal/MetalState.h +++ b/filament/backend/src/metal/MetalState.h @@ -382,7 +382,6 @@ using SamplerStateCache = StateCache, SamplerS using CullModeStateTracker = StateTracker; using WindingStateTracker = StateTracker; -using DepthClampStateTracker = StateTracker; // Argument encoder diff --git a/filament/backend/src/noop/NoopDriver.cpp b/filament/backend/src/noop/NoopDriver.cpp index 096f57c3989..adc9c1bc55f 100644 --- a/filament/backend/src/noop/NoopDriver.cpp +++ b/filament/backend/src/noop/NoopDriver.cpp @@ -202,10 +202,6 @@ bool NoopDriver::isProtectedTexturesSupported() { return true; } -bool NoopDriver::isDepthClampSupported() { - return false; -} - bool NoopDriver::isWorkaroundNeeded(Workaround) { return false; } diff --git a/filament/backend/src/opengl/OpenGLContext.cpp b/filament/backend/src/opengl/OpenGLContext.cpp index a7933407bb4..66327633f17 100644 --- a/filament/backend/src/opengl/OpenGLContext.cpp +++ b/filament/backend/src/opengl/OpenGLContext.cpp @@ -676,7 +676,6 @@ void OpenGLContext::initExtensionsGLES(Extensions* ext, GLint major, GLint minor #ifndef __EMSCRIPTEN__ ext->EXT_debug_marker = exts.has("GL_EXT_debug_marker"sv); #endif - ext->EXT_depth_clamp = exts.has("GL_EXT_depth_clamp"sv); ext->EXT_discard_framebuffer = exts.has("GL_EXT_discard_framebuffer"sv); #ifndef __EMSCRIPTEN__ ext->EXT_disjoint_timer_query = exts.has("GL_EXT_disjoint_timer_query"sv); @@ -747,7 +746,6 @@ void OpenGLContext::initExtensionsGL(Extensions* ext, GLint major, GLint minor) ext->EXT_color_buffer_half_float = true; // Assumes core profile. ext->EXT_clip_cull_distance = true; ext->EXT_debug_marker = exts.has("GL_EXT_debug_marker"sv); - ext->EXT_depth_clamp = true; ext->EXT_discard_framebuffer = false; ext->EXT_disjoint_timer_query = true; ext->EXT_multisampled_render_to_texture = false; diff --git a/filament/backend/src/opengl/OpenGLContext.h b/filament/backend/src/opengl/OpenGLContext.h index 2972480e71f..902fcc9f094 100644 --- a/filament/backend/src/opengl/OpenGLContext.h +++ b/filament/backend/src/opengl/OpenGLContext.h @@ -220,9 +220,8 @@ class OpenGLContext final : public TimerQueryFactoryInterface { bool EXT_color_buffer_float; bool EXT_color_buffer_half_float; bool EXT_debug_marker; - bool EXT_depth_clamp; - bool EXT_discard_framebuffer; bool EXT_disjoint_timer_query; + bool EXT_discard_framebuffer; bool EXT_multisampled_render_to_texture2; bool EXT_multisampled_render_to_texture; bool EXT_protected_textures; @@ -240,10 +239,10 @@ class OpenGLContext final : public TimerQueryFactoryInterface { bool KHR_parallel_shader_compile; bool KHR_texture_compression_astc_hdr; bool KHR_texture_compression_astc_ldr; - bool OES_EGL_image_external_essl3; - bool OES_depth24; bool OES_depth_texture; + bool OES_depth24; bool OES_packed_depth_stencil; + bool OES_EGL_image_external_essl3; bool OES_rgb8_rgba8; bool OES_standard_derivatives; bool OES_texture_npot; @@ -628,7 +627,6 @@ constexpr size_t OpenGLContext::getIndexForCap(GLenum cap) noexcept { //NOLINT #ifdef BACKEND_OPENGL_VERSION_GL case GL_PROGRAM_POINT_SIZE: index = 10; break; #endif - case GL_DEPTH_CLAMP: index = 11; break; default: break; } assert_invariant(index < state.enables.caps.size()); diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp index ed23067bb87..bb9d2a63a57 100644 --- a/filament/backend/src/opengl/OpenGLDriver.cpp +++ b/filament/backend/src/opengl/OpenGLDriver.cpp @@ -451,14 +451,6 @@ void OpenGLDriver::setRasterState(RasterState rs) noexcept { } else { gl.disable(GL_SAMPLE_ALPHA_TO_COVERAGE); } - - if (gl.ext.EXT_depth_clamp) { - if (rs.depthClamp) { - gl.enable(GL_DEPTH_CLAMP); - } else { - gl.disable(GL_DEPTH_CLAMP); - } - } } void OpenGLDriver::setStencilState(StencilState ss) noexcept { @@ -2127,10 +2119,6 @@ bool OpenGLDriver::isProtectedTexturesSupported() { return getContext().ext.EXT_protected_textures; } -bool OpenGLDriver::isDepthClampSupported() { - return getContext().ext.EXT_depth_clamp; -} - bool OpenGLDriver::isWorkaroundNeeded(Workaround workaround) { switch (workaround) { case Workaround::SPLIT_EASU: diff --git a/filament/backend/src/opengl/gl_headers.h b/filament/backend/src/opengl/gl_headers.h index 04ad63ff6ac..569c8ed2859 100644 --- a/filament/backend/src/opengl/gl_headers.h +++ b/filament/backend/src/opengl/gl_headers.h @@ -201,12 +201,6 @@ using namespace glext; # define GL_CLIP_DISTANCE1 0x3001 #endif -#if defined(GL_EXT_depth_clamp) -# define GL_DEPTH_CLAMP GL_DEPTH_CLAMP_EXT -#else -# define GL_DEPTH_CLAMP 0x864F -#endif - #if defined(GL_KHR_debug) # define GL_DEBUG_OUTPUT GL_DEBUG_OUTPUT_KHR # define GL_DEBUG_OUTPUT_SYNCHRONOUS GL_DEBUG_OUTPUT_SYNCHRONOUS_KHR diff --git a/filament/backend/src/vulkan/VulkanContext.h b/filament/backend/src/vulkan/VulkanContext.h index 46fe475612a..9f506e03f76 100644 --- a/filament/backend/src/vulkan/VulkanContext.h +++ b/filament/backend/src/vulkan/VulkanContext.h @@ -125,10 +125,6 @@ struct VulkanContext { return mPhysicalDeviceFeatures.imageCubeArray == VK_TRUE; } - inline bool isDepthClampSupported() const noexcept { - return mPhysicalDeviceFeatures.depthClamp == VK_TRUE; - } - inline bool isDebugMarkersSupported() const noexcept { return mDebugMarkersSupported; } diff --git a/filament/backend/src/vulkan/VulkanDriver.cpp b/filament/backend/src/vulkan/VulkanDriver.cpp index 0b444c0b75e..0855102dd91 100644 --- a/filament/backend/src/vulkan/VulkanDriver.cpp +++ b/filament/backend/src/vulkan/VulkanDriver.cpp @@ -944,10 +944,6 @@ bool VulkanDriver::isProtectedTexturesSupported() { return false; } -bool VulkanDriver::isDepthClampSupported() { - return mContext.isDepthClampSupported(); -} - bool VulkanDriver::isWorkaroundNeeded(Workaround workaround) { switch (workaround) { case Workaround::SPLIT_EASU: { @@ -1820,8 +1816,6 @@ void VulkanDriver::bindPipeline(PipelineState const& pipelineState) { .dstAlphaBlendFactor = getBlendFactor(rasterState.blendFunctionDstAlpha), .colorWriteMask = (VkColorComponentFlags) (rasterState.colorWrite ? 0xf : 0x0), .rasterizationSamples = rt->getSamples(), - .depthClamp = rasterState.depthClamp, - .reserved = 0, .colorTargetCount = rt->getColorTargetCount(mCurrentRenderPass), .colorBlendOp = rasterState.blendEquationRGB, .alphaBlendOp = rasterState.blendEquationAlpha, diff --git a/filament/backend/src/vulkan/VulkanPipelineCache.cpp b/filament/backend/src/vulkan/VulkanPipelineCache.cpp index b3d09da86ed..4c77525bd1b 100644 --- a/filament/backend/src/vulkan/VulkanPipelineCache.cpp +++ b/filament/backend/src/vulkan/VulkanPipelineCache.cpp @@ -184,7 +184,6 @@ VulkanPipelineCache::PipelineCacheEntry* VulkanPipelineCache::createPipeline() n vkRaster.polygonMode = VK_POLYGON_MODE_FILL; vkRaster.cullMode = raster.cullMode; vkRaster.frontFace = raster.frontFace; - vkRaster.depthClampEnable = raster.depthClamp; vkRaster.depthBiasEnable = raster.depthBiasEnable; vkRaster.depthBiasConstantFactor = raster.depthBiasConstantFactor; vkRaster.depthBiasClamp = 0.0f; diff --git a/filament/backend/src/vulkan/VulkanPipelineCache.h b/filament/backend/src/vulkan/VulkanPipelineCache.h index 985574b74c2..e6816497c05 100644 --- a/filament/backend/src/vulkan/VulkanPipelineCache.h +++ b/filament/backend/src/vulkan/VulkanPipelineCache.h @@ -90,9 +90,7 @@ class VulkanPipelineCache { VkBlendFactor srcAlphaBlendFactor : 5; VkBlendFactor dstAlphaBlendFactor : 5; VkColorComponentFlags colorWriteMask : 4; - uint8_t rasterizationSamples : 4;// offset = 4 bytes - uint8_t depthClamp : 1; - uint8_t reserved : 3; + uint8_t rasterizationSamples; // offset = 4 bytes uint8_t colorTargetCount; // offset = 5 bytes BlendEquation colorBlendOp : 4; // offset = 6 bytes BlendEquation alphaBlendOp : 4; diff --git a/filament/backend/src/vulkan/platform/VulkanPlatform.cpp b/filament/backend/src/vulkan/platform/VulkanPlatform.cpp index 2e88542080f..b4152807ccd 100644 --- a/filament/backend/src/vulkan/platform/VulkanPlatform.cpp +++ b/filament/backend/src/vulkan/platform/VulkanPlatform.cpp @@ -325,7 +325,6 @@ VkDevice createLogicalDevice(VkPhysicalDevice physicalDevice, // We could simply enable all supported features, but since that may have performance // consequences let's just enable the features we need. VkPhysicalDeviceFeatures enabledFeatures{ - .depthClamp = features.depthClamp, .samplerAnisotropy = features.samplerAnisotropy, .textureCompressionETC2 = features.textureCompressionETC2, .textureCompressionBC = features.textureCompressionBC, diff --git a/filament/src/RenderPass.cpp b/filament/src/RenderPass.cpp index 55ec3264b2d..b931419775c 100644 --- a/filament/src/RenderPass.cpp +++ b/filament/src/RenderPass.cpp @@ -419,8 +419,7 @@ RenderPass::Command* RenderPass::instanceify(FEngine& engine, UTILS_ALWAYS_INLINE // This function exists only to make the code more readable. we want it inlined. inline // and we don't need it in the compilation unit void RenderPass::setupColorCommand(Command& cmdDraw, Variant variant, - FMaterialInstance const* const UTILS_RESTRICT mi, - bool inverseFrontFaces, bool hasDepthClamp) noexcept { + FMaterialInstance const* const UTILS_RESTRICT mi, bool inverseFrontFaces) noexcept { FMaterial const * const UTILS_RESTRICT ma = mi->getMaterial(); variant = Variant::filterVariant(variant, ma->isVariantLit()); @@ -461,7 +460,6 @@ void RenderPass::setupColorCommand(Command& cmdDraw, Variant variant, cmdDraw.info.rasterState.colorWrite = mi->isColorWriteEnabled(); cmdDraw.info.rasterState.depthWrite = mi->isDepthWriteEnabled(); cmdDraw.info.rasterState.depthFunc = mi->getDepthFunc(); - cmdDraw.info.rasterState.depthClamp = hasDepthClamp; cmdDraw.info.materialVariant = variant; // we keep "RasterState::colorWrite" to the value set by material (could be disabled) } @@ -560,9 +558,6 @@ RenderPass::Command* RenderPass::generateCommandsImpl(RenderPass::CommandTypeFla bool const hasInstancedStereo = renderFlags & IS_INSTANCED_STEREOSCOPIC; - bool const hasDepthClamp = - renderFlags & HAS_DEPTH_CLAMP; - float const cameraPositionDotCameraForward = dot(cameraPosition, cameraForward); auto const* const UTILS_RESTRICT soaWorldAABBCenter = soa.data(); @@ -582,7 +577,6 @@ RenderPass::Command* RenderPass::generateCommandsImpl(RenderPass::CommandTypeFla cmd.info.rasterState.depthWrite = true; cmd.info.rasterState.depthFunc = RasterState::DepthFunc::GE; cmd.info.rasterState.alphaToCoverage = false; - cmd.info.rasterState.depthClamp = hasDepthClamp; } for (uint32_t i = range.first; i < range.last; ++i) { @@ -697,8 +691,7 @@ RenderPass::Command* RenderPass::generateCommandsImpl(RenderPass::CommandTypeFla cmd.info.morphingOffset = primitive.getMorphingBufferOffset(); if constexpr (isColorPass) { - RenderPass::setupColorCommand(cmd, renderableVariant, mi, - inverseFrontFaces, hasDepthClamp); + RenderPass::setupColorCommand(cmd, renderableVariant, mi, inverseFrontFaces); const bool blendPass = Pass(cmd.key & PASS_MASK) == Pass::BLENDED; if (blendPass) { // TODO: at least for transparent objects, AABB should be per primitive diff --git a/filament/src/RenderPass.h b/filament/src/RenderPass.h index 3cad6b56031..e729d67dd7f 100644 --- a/filament/src/RenderPass.h +++ b/filament/src/RenderPass.h @@ -284,7 +284,6 @@ class RenderPass { static constexpr RenderFlags HAS_SHADOWING = 0x01; static constexpr RenderFlags HAS_INVERSE_FRONT_FACES = 0x02; static constexpr RenderFlags IS_INSTANCED_STEREOSCOPIC = 0x04; - static constexpr RenderFlags HAS_DEPTH_CLAMP = 0x08; // Arena used for commands using Arena = utils::Arena< @@ -445,7 +444,7 @@ class RenderPass { uint8_t instancedStereoEyeCount) noexcept; static void setupColorCommand(Command& cmdDraw, Variant variant, - FMaterialInstance const* mi, bool inverseFrontFaces, bool hasDepthClamp) noexcept; + FMaterialInstance const* mi, bool inverseFrontFaces) noexcept; static void updateSummedPrimitiveCounts( FScene::RenderableSoa& renderableData, utils::Range vr) noexcept; diff --git a/filament/src/ShadowMap.cpp b/filament/src/ShadowMap.cpp index bbf008e4a04..4c406680542 100644 --- a/filament/src/ShadowMap.cpp +++ b/filament/src/ShadowMap.cpp @@ -829,13 +829,13 @@ ShadowMap::Corners ShadowMap::computeFrustumCorners( Corners const csViewFrustumCorners = { .vertices = { { -1, -1, far }, - { 1, -1, far }, - { -1, 1, far }, - { 1, 1, far }, + { 1, -1, far }, + { -1, 1, far }, + { 1, 1, far }, { -1, -1, near }, - { 1, -1, near }, - { -1, 1, near }, - { 1, 1, near }, + { 1, -1, near }, + { -1, 1, near }, + { 1, 1, near }, } }; diff --git a/filament/src/ShadowMapManager.cpp b/filament/src/ShadowMapManager.cpp index c248dfbcc0f..a6f4a9fa3c0 100644 --- a/filament/src/ShadowMapManager.cpp +++ b/filament/src/ShadowMapManager.cpp @@ -66,15 +66,15 @@ namespace filament { using namespace backend; using namespace math; -ShadowMapManager::ShadowMapManager(FEngine& engine) - : mIsDepthClampSupported(engine.getDriverApi().isDepthClampSupported()) { +// do this only if depth-clamp is available +static constexpr bool USE_DEPTH_CLAMP = false; + +ShadowMapManager::ShadowMapManager(FEngine& engine) { FDebugRegistry& debugRegistry = engine.getDebugRegistry(); debugRegistry.registerProperty("d.shadowmap.visualize_cascades", &engine.debug.shadowmap.visualize_cascades); debugRegistry.registerProperty("d.shadowmap.disable_light_frustum_align", &engine.debug.shadowmap.disable_light_frustum_align); - debugRegistry.registerProperty("d.shadowmap.depth_clamp", - &engine.debug.shadowmap.depth_clamp); } ShadowMapManager::~ShadowMapManager() { @@ -367,22 +367,7 @@ FrameGraphId ShadowMapManager::render(FEngine& engine, FrameG // generate and sort the commands for rendering the shadow map - RenderPass::RenderFlags renderPassFlags{}; - if (view.isFrontFaceWindingInverted()) { - renderPassFlags |= RenderPass::HAS_INVERSE_FRONT_FACES; - } - - bool const canUseDepthClamp = - !view.hasVSM() && - mIsDepthClampSupported && - engine.debug.shadowmap.depth_clamp; - - if (canUseDepthClamp) { - renderPassFlags |= RenderPass::HAS_DEPTH_CLAMP; - } - RenderPass const pass = passBuilder - .renderFlags(renderPassFlags) .camera(cameraInfo) .visibilityMask(entry.visibilityMask) .geometry(scene->getRenderableData(), @@ -656,14 +641,8 @@ ShadowMapManager::ShadowTechnique ShadowMapManager::updateCascadeShadowMaps(FEng cameraInfo.zf = -nearFarPlanes[i + 1]; updateNearFarPlanes(&cameraInfo.cullingProjection, cameraInfo.zn, cameraInfo.zf); - bool const canUseDepthClamp = - !view.hasVSM() && - mIsDepthClampSupported && - engine.debug.shadowmap.depth_clamp; - auto shaderParameters = shadowMap.updateDirectional(engine, - lightData, 0, cameraInfo, shadowMapInfo, sceneInfo, - canUseDepthClamp); + lightData, 0, cameraInfo, shadowMapInfo, sceneInfo, USE_DEPTH_CLAMP); if (shadowMap.hasVisibleShadows()) { const size_t shadowIndex = shadowMap.getShadowIndex(); diff --git a/filament/src/ShadowMapManager.h b/filament/src/ShadowMapManager.h index 95ad0127741..18b4a24e191 100644 --- a/filament/src/ShadowMapManager.h +++ b/filament/src/ShadowMapManager.h @@ -232,7 +232,6 @@ class ShadowMapManager { ShadowMapCacheContainer mShadowMapCache; uint32_t mDirectionalShadowMapCount = 0; uint32_t mSpotShadowMapCount = 0; - bool const mIsDepthClampSupported; bool mInitialized = false; ShadowMap& getShadowMap(size_t index) noexcept { diff --git a/filament/src/details/DebugRegistry.cpp b/filament/src/details/DebugRegistry.cpp index 8915cf312e1..c2f5d3d3fb6 100644 --- a/filament/src/details/DebugRegistry.cpp +++ b/filament/src/details/DebugRegistry.cpp @@ -28,6 +28,12 @@ #include #include +#ifndef NDEBUG +# define DEBUG_PROPERTIES_WRITABLE true +#else +# define DEBUG_PROPERTIES_WRITABLE false +#endif + using namespace filament::math; using namespace utils; @@ -73,15 +79,17 @@ bool FDebugRegistry::hasProperty(const char* name) const noexcept { template bool FDebugRegistry::setProperty(const char* name, T v) noexcept { - auto info = getPropertyInfo(name); - T* const addr = static_cast(info.first); - if (addr) { - auto old = *addr; - *addr = v; - if (info.second && old != v) { - info.second(); + if constexpr (DEBUG_PROPERTIES_WRITABLE) { + auto info = getPropertyInfo(name); + T* const addr = static_cast(info.first); + if (addr) { + auto old = *addr; + *addr = v; + if (info.second && old != v) { + info.second(); + } + return true; } - return true; } return false; } diff --git a/filament/src/details/Engine.h b/filament/src/details/Engine.h index fa963704177..d456a7b042c 100644 --- a/filament/src/details/Engine.h +++ b/filament/src/details/Engine.h @@ -600,7 +600,6 @@ class FEngine : public Engine { bool focus_shadowcasters = true; bool visualize_cascades = false; bool disable_light_frustum_align = false; - bool depth_clamp = true; float dzn = -1.0f; float dzf = 1.0f; float display_shadow_texture_scale = 0.25f; diff --git a/samples/gltf_viewer.cpp b/samples/gltf_viewer.cpp index e3dc4363a38..6206c0c9a46 100644 --- a/samples/gltf_viewer.cpp +++ b/samples/gltf_viewer.cpp @@ -905,8 +905,6 @@ int main(int argc, char** argv) { debug.getPropertyAddress("d.shadowmap.focus_shadowcasters")); ImGui::Checkbox("Disable light frustum alignment", debug.getPropertyAddress("d.shadowmap.disable_light_frustum_align")); - ImGui::Checkbox("Depth clamp", - debug.getPropertyAddress("d.shadowmap.depth_clamp")); bool debugDirectionalShadowmap; if (debug.getProperty("d.shadowmap.debug_directional_shadowmap", From c84f5d2a7fbc97e367a3dc9f0aae50e14e1ff8eb Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Thu, 22 Aug 2024 17:00:04 -0700 Subject: [PATCH 18/26] fix screen-space reflection when sub-pass colorgrading is used the problem was that in that case, SSR was using the colorgraded color buffer as history. --- filament/src/RendererUtils.cpp | 19 ++++++++++++------- filament/src/details/Renderer.cpp | 28 ++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/filament/src/RendererUtils.cpp b/filament/src/RendererUtils.cpp index c4f14e4ce16..e1a18cc7f15 100644 --- a/filament/src/RendererUtils.cpp +++ b/filament/src/RendererUtils.cpp @@ -153,6 +153,8 @@ FrameGraphId RendererUtils::colorPass( } if (colorGradingConfig.asSubpass) { + assert_invariant(config.msaa <= 1); + assert_invariant(colorBufferDesc.samples <= 1); data.output = builder.createTexture("Tonemapped Buffer", { .width = colorBufferDesc.width, .height = colorBufferDesc.height, @@ -263,7 +265,9 @@ FrameGraphId RendererUtils::colorPass( // when color grading is done as a subpass, the output of the color-pass is the ldr buffer auto output = colorGradingConfig.asSubpass ? colorPass->output : colorPass->color; - blackboard["color"] = output; + // linear color buffer + blackboard["color"] = colorPass->color; + return output; } @@ -275,7 +279,7 @@ std::pair, bool> RendererUtils::refractionPass( RenderPass const& pass) noexcept { auto& blackboard = fg.getBlackboard(); - auto input = blackboard.get("color"); + FrameGraphId output; // find the first refractive object in channel 2 @@ -295,13 +299,13 @@ std::pair, bool> RendererUtils::refractionPass( PostProcessManager& ppm = engine.getPostProcessManager(); // clear the color/depth buffers, which will orphan (and cull) the color pass - input.clear(); blackboard.remove("color"); blackboard.remove("depth"); config.hasScreenSpaceReflectionsOrRefractions = true; - input = RendererUtils::colorPass(fg, "Color Pass (opaque)", engine, view, { + FrameGraphId const input = RendererUtils::colorPass(fg, + "Color Pass (opaque)", engine, view, { // When rendering the opaques, we need to conserve the sample buffer, // so create a config that specifies the sample count. .width = config.physicalViewport.width, @@ -330,7 +334,8 @@ std::pair, bool> RendererUtils::refractionPass( output = RendererUtils::colorPass(fg, "Color Pass (transparent)", engine, view, { .width = config.physicalViewport.width, .height = config.physicalViewport.height }, - config, colorGradingConfig, pass.getExecutor(refraction, pass.end())); + config, colorGradingConfig, + pass.getExecutor(refraction, pass.end())); if (config.msaa > 1 && !colorGradingConfig.asSubpass) { // We need to do a resolve here because later passes (such as color grading or DoF) will @@ -339,8 +344,8 @@ std::pair, bool> RendererUtils::refractionPass( // conserve the multi-sample buffer. output = ppm.resolve(fg, "Resolved Color Buffer", output, { .levels = 1 }); } - } else { - output = input; + // this becomes the screen-space reflection history + blackboard["color"] = output; } return { output, hasScreenSpaceRefraction }; } diff --git a/filament/src/details/Renderer.cpp b/filament/src/details/Renderer.cpp index 1b8ffdd3596..ee9857b87f6 100644 --- a/filament/src/details/Renderer.cpp +++ b/filament/src/details/Renderer.cpp @@ -655,16 +655,16 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { // on qualcomm hardware -- we might need a backend dependent toggle at some point const PostProcessManager::ColorGradingConfig colorGradingConfig{ .asSubpass = - hasColorGrading && msaaSampleCount <= 1 && - !bloomOptions.enabled && !dofOptions.enabled && !taaOptions.enabled && driver.isFrameBufferFetchSupported() && + hasColorGrading && + !bloomOptions.enabled && !dofOptions.enabled && !taaOptions.enabled && !engine.debug.renderer.disable_subpasses, .customResolve = - msaaOptions.customResolve && msaaSampleCount > 1 && - hasColorGrading && driver.isFrameBufferFetchMultiSampleSupported() && + msaaOptions.customResolve && + hasColorGrading && !engine.debug.renderer.disable_subpasses, .translucent = needsAlphaChannel, .fxaa = hasFXAA, @@ -673,6 +673,9 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { TextureFormat::RGBA8 : getLdrFormat(needsAlphaChannel) }; + // by construction (msaaSampleCount) both asSubpass and customResolve can't be true + assert_invariant(colorGradingConfig.asSubpass + colorGradingConfig.customResolve < 2); + // whether we're scaled at all bool scaled = any(notEqual(scale, float2(1.0f))); @@ -1108,20 +1111,26 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { colorBufferDesc, config, colorGradingConfigForColor, pass.getExecutor()); if (view.isScreenSpaceRefractionEnabled() && !pass.empty()) { - // this cancels the colorPass() call above if refraction is active. - // the color pass + refraction + color-grading as subpass if needed + // This cancels the colorPass() call above if refraction is active. + // The color pass + refraction + color-grading as subpass if needed const auto [output, enabled] = RendererUtils::refractionPass(fg, mEngine, view, config, ssrConfig, colorGradingConfigForColor, pass); - colorPassOutput = output; hasScreenSpaceRefraction = enabled; + if (enabled) { + colorPassOutput = output; + } } + // Here, colorPassOutput can be either tonemapped or not; it's tonemapped if color gradding + // is done as a subpass. + if (colorGradingConfig.customResolve) { // TODO: we have to "uncompress" (i.e. detonemap) the color buffer here because it's used // by many other passes (Bloom, TAA, DoF, etc...). We could make this more // efficient by using ARM_shader_framebuffer_fetch. We use a load/store (i.e. // subpass) here because it's more convenient. colorPassOutput = ppm.customResolveUncompressPass(fg, colorPassOutput); + blackboard["color"] = colorPassOutput; } // export the color buffer if screen-space reflections are enabled @@ -1137,7 +1146,10 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { // The "output" of this pass is going to be used during the next frame as // an "import". builder.sideEffect(); - data.history = builder.sample(colorPassOutput); // FIXME: an access must be declared for detach(), why? + + // we can't use colorPassOutput here because it could be tonemapped + auto color = blackboard.get("color"); + data.history = builder.sample(color); // FIXME: an access must be declared for detach(), why? }, [&view, projection](FrameGraphResources const& resources, auto const& data, backend::DriverApi&) { auto& history = view.getFrameHistory(); From f11e5cb081ea5478d7ec62cb9a23e45ca0a5f9e0 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 23 Aug 2024 16:01:40 -0700 Subject: [PATCH 19/26] remove uses of the blackboard the framegraph blackboard tends to create a lot of problems hard to debug, so we are now more explicit. here we remove usages of the blackboard in RenderUtils.cpp. --- filament/src/PostProcessManager.cpp | 1 + filament/src/RendererUtils.cpp | 79 ++++++++++++++--------------- filament/src/RendererUtils.h | 26 +++++++++- filament/src/details/Renderer.cpp | 48 +++++++++++------- 4 files changed, 93 insertions(+), 61 deletions(-) diff --git a/filament/src/PostProcessManager.cpp b/filament/src/PostProcessManager.cpp index 18b6ab30dc7..f0ee30c682b 100644 --- a/filament/src/PostProcessManager.cpp +++ b/filament/src/PostProcessManager.cpp @@ -3193,6 +3193,7 @@ FrameGraphId PostProcessManager::resolve(FrameGraph& fg, assert_invariant(dst); assert_invariant(srcDesc.format == dstDesc.format); assert_invariant(srcDesc.width == dstDesc.width && srcDesc.height == dstDesc.height); + assert_invariant(srcDesc.samples > 1 && dstDesc.samples <= 1); driver.resolve( dst, dstSubDesc.level, dstSubDesc.layer, src, srcSubDesc.level, srcSubDesc.layer); diff --git a/filament/src/RendererUtils.cpp b/filament/src/RendererUtils.cpp index e1a18cc7f15..c25530f8811 100644 --- a/filament/src/RendererUtils.cpp +++ b/filament/src/RendererUtils.cpp @@ -40,6 +40,7 @@ #include #include +#include #include #include @@ -50,8 +51,9 @@ namespace filament { using namespace backend; using namespace math; -FrameGraphId RendererUtils::colorPass( +RendererUtils::ColorPassOutput RendererUtils::colorPass( FrameGraph& fg, const char* name, FEngine& engine, FView const& view, + ColorPassInput const& colorPassInput, FrameGraphTexture::Descriptor const& colorBufferDesc, ColorPassConfig const& config, PostProcessManager::ColorGradingConfig colorGradingConfig, RenderPass::Executor passExecutor) noexcept { @@ -67,8 +69,6 @@ FrameGraphId RendererUtils::colorPass( FrameGraphId structure; }; - Blackboard& blackboard = fg.getBlackboard(); - auto& colorPass = fg.addPass(name, [&](FrameGraph::Builder& builder, ColorPassData& data) { @@ -76,21 +76,21 @@ FrameGraphId RendererUtils::colorPass( TargetBufferFlags clearDepthFlags = config.clearFlags & TargetBufferFlags::DEPTH; TargetBufferFlags clearStencilFlags = config.clearFlags & TargetBufferFlags::STENCIL; - data.shadows = blackboard.get("shadows"); - data.ssao = blackboard.get("ssao"); - data.color = blackboard.get("color"); - data.depth = blackboard.get("depth"); + data.color = colorPassInput.linearColor; + data.depth = colorPassInput.depth; + data.shadows = colorPassInput.shadows; + data.ssao = colorPassInput.ssao; // Screen-space reflection or refractions if (config.hasScreenSpaceReflectionsOrRefractions) { - data.ssr = blackboard.get("ssr"); + data.ssr = colorPassInput.ssr; if (data.ssr) { data.ssr = builder.sample(data.ssr); } } if (config.hasContactShadows) { - data.structure = blackboard.get("structure"); + data.structure = colorPassInput.structure; assert_invariant(data.structure); data.structure = builder.sample(data.structure); } @@ -196,7 +196,6 @@ FrameGraphId RendererUtils::colorPass( .samples = config.msaa, .layerCount = static_cast(colorBufferDesc.depth), .clearFlags = clearColorFlags | clearDepthFlags | clearStencilFlags}); - blackboard["depth"] = data.depth; }, [=, passExecutor = std::move(passExecutor), &view, &engine](FrameGraphResources const& resources, ColorPassData const& data, DriverApi& driver) { @@ -262,26 +261,21 @@ FrameGraphId RendererUtils::colorPass( } ); - // when color grading is done as a subpass, the output of the color-pass is the ldr buffer - auto output = colorGradingConfig.asSubpass ? colorPass->output : colorPass->color; - - // linear color buffer - blackboard["color"] = colorPass->color; - - return output; + return { + .linearColor = colorPass->color, + .tonemappedColor = colorPass->output, // can be null + .depth = colorPass->depth + }; } -std::pair, bool> RendererUtils::refractionPass( +std::optional RendererUtils::refractionPass( FrameGraph& fg, FEngine& engine, FView const& view, + ColorPassInput colorPassInput, ColorPassConfig config, PostProcessManager::ScreenSpaceRefConfig const& ssrConfig, PostProcessManager::ColorGradingConfig colorGradingConfig, RenderPass const& pass) noexcept { - auto& blackboard = fg.getBlackboard(); - - FrameGraphId output; - // find the first refractive object in channel 2 RenderPass::Command const* const refraction = std::partition_point(pass.begin(), pass.end(), [](auto const& command) { @@ -296,34 +290,35 @@ std::pair, bool> RendererUtils::refractionPass( // if there wasn't any refractive object, just skip everything below. if (UTILS_UNLIKELY(hasScreenSpaceRefraction)) { - PostProcessManager& ppm = engine.getPostProcessManager(); - - // clear the color/depth buffers, which will orphan (and cull) the color pass - blackboard.remove("color"); - blackboard.remove("depth"); - + assert_invariant(!colorPassInput.linearColor); + assert_invariant(!colorPassInput.depth); config.hasScreenSpaceReflectionsOrRefractions = true; - FrameGraphId const input = RendererUtils::colorPass(fg, - "Color Pass (opaque)", engine, view, { + PostProcessManager& ppm = engine.getPostProcessManager(); + auto const opaquePassOutput = RendererUtils::colorPass(fg, + "Color Pass (opaque)", engine, view, colorPassInput, { // When rendering the opaques, we need to conserve the sample buffer, // so create a config that specifies the sample count. .width = config.physicalViewport.width, .height = config.physicalViewport.height, .samples = config.msaa, .format = config.hdrFormat - }, config, { .asSubpass = false }, + }, + config, { .asSubpass = false, .customResolve = false }, pass.getExecutor(pass.begin(), refraction)); // generate the mipmap chain PostProcessManager::generateMipmapSSR(ppm, fg, - input, ssrConfig.refraction, true, ssrConfig); + opaquePassOutput.linearColor, + ssrConfig.refraction, + true, ssrConfig); // Now we're doing the refraction pass proper. - // This uses the same framebuffer (color and depth) used by the opaque pass. This happens - // automatically because these are set in the Blackboard (they were set by the opaque - // pass). For this reason, `desc` below is only used in colorPass() for the width and - // height. + // This uses the same framebuffer (color and depth) used by the opaque pass. + // For this reason, the `colorBufferDesc` parameter of colorPass() below is only used for + // the width and height. + colorPassInput.linearColor = opaquePassOutput.linearColor; + colorPassInput.depth = opaquePassOutput.depth; // Since we're reusing the existing target we don't want to clear any of its buffer. // Important: if this target ended up being an imported target, then the clearFlags @@ -331,7 +326,8 @@ std::pair, bool> RendererUtils::refractionPass( // and we'd end up clearing the opaque pass. This scenario never happens because it is // prevented in Renderer.cpp's final blit. config.clearFlags = TargetBufferFlags::NONE; - output = RendererUtils::colorPass(fg, "Color Pass (transparent)", engine, view, { + auto transparentPassOutput = RendererUtils::colorPass(fg, "Color Pass (transparent)", + engine, view, colorPassInput, { .width = config.physicalViewport.width, .height = config.physicalViewport.height }, config, colorGradingConfig, @@ -342,12 +338,13 @@ std::pair, bool> RendererUtils::refractionPass( // need to sample from 'output'. However, because we have MSAA, we know we're not // sampleable. And this is because in the SSR case, we had to use a renderbuffer to // conserve the multi-sample buffer. - output = ppm.resolve(fg, "Resolved Color Buffer", output, { .levels = 1 }); + transparentPassOutput.linearColor = ppm.resolve(fg, "Resolved Color Buffer", + transparentPassOutput.linearColor, { .levels = 1 }); } - // this becomes the screen-space reflection history - blackboard["color"] = output; + return transparentPassOutput; } - return { output, hasScreenSpaceRefraction }; + + return std::nullopt; } UTILS_NOINLINE diff --git a/filament/src/RendererUtils.h b/filament/src/RendererUtils.h index 9b7e93cbc60..34ea7b0eda9 100644 --- a/filament/src/RendererUtils.h +++ b/filament/src/RendererUtils.h @@ -21,14 +21,19 @@ #include "RenderPass.h" #include "fg/FrameGraphId.h" +#include "fg/FrameGraphTexture.h" + +#include #include #include +#include #include #include +#include #include namespace filament { @@ -71,15 +76,32 @@ class RendererUtils { bool screenSpaceReflectionHistoryNotReady; }; - static FrameGraphId colorPass( + struct ColorPassInput { + FrameGraphId linearColor; + FrameGraphId tonemappedColor; + FrameGraphId depth; + FrameGraphId shadows; + FrameGraphId ssao; + FrameGraphId ssr; + FrameGraphId structure; + }; + struct ColorPassOutput { + FrameGraphId linearColor; + FrameGraphId tonemappedColor; + FrameGraphId depth; + }; + + static ColorPassOutput colorPass( FrameGraph& fg, const char* name, FEngine& engine, FView const& view, + ColorPassInput const& colorPassInput, FrameGraphTexture::Descriptor const& colorBufferDesc, ColorPassConfig const& config, PostProcessManager::ColorGradingConfig colorGradingConfig, RenderPass::Executor passExecutor) noexcept; - static std::pair, bool> refractionPass( + static std::optional refractionPass( FrameGraph& fg, FEngine& engine, FView const& view, + ColorPassInput colorPassInput, ColorPassConfig config, PostProcessManager::ScreenSpaceRefConfig const& ssrConfig, PostProcessManager::ColorGradingConfig colorGradingConfig, diff --git a/filament/src/details/Renderer.cpp b/filament/src/details/Renderer.cpp index ee9857b87f6..bb743fca2bd 100644 --- a/filament/src/details/Renderer.cpp +++ b/filament/src/details/Renderer.cpp @@ -955,7 +955,6 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { .scale = aoOptions.resolution, .picking = view.hasPicking() }); - blackboard["structure"] = structure; const auto picking = picking_; @@ -1003,7 +1002,6 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { ssReflectionsOptions.enabled ? TextureFormat::RGBA16F : TextureFormat::R11F_G11F_B10F, view.getCameraUser().getFieldOfView(Camera::Fov::VERTICAL), config.scale); config.ssrLodOffset = ssrConfig.lodOffset; - blackboard["ssr"] = ssrConfig.ssr; // -------------------------------------------------------------------------------------------- // screen-space reflections pass @@ -1107,30 +1105,39 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { ); // the color pass itself + color-grading as subpass if needed - auto colorPassOutput = RendererUtils::colorPass(fg, "Color Pass", mEngine, view, + auto colorPassOutput = RendererUtils::colorPass(fg, "Color Pass", mEngine, view, { + .shadows = blackboard.get("shadows"), + .ssao = blackboard.get("ssao"), + .ssr = ssrConfig.ssr, + .structure = structure + }, colorBufferDesc, config, colorGradingConfigForColor, pass.getExecutor()); if (view.isScreenSpaceRefractionEnabled() && !pass.empty()) { // This cancels the colorPass() call above if refraction is active. // The color pass + refraction + color-grading as subpass if needed - const auto [output, enabled] = RendererUtils::refractionPass(fg, mEngine, view, + auto const output = RendererUtils::refractionPass(fg, mEngine, view, { + .shadows = blackboard.get("shadows"), + .ssao = blackboard.get("ssao"), + .ssr = ssrConfig.ssr, + .structure = structure + }, config, ssrConfig, colorGradingConfigForColor, pass); - hasScreenSpaceRefraction = enabled; - if (enabled) { - colorPassOutput = output; + + hasScreenSpaceRefraction = output.has_value(); + if (hasScreenSpaceRefraction) { + colorPassOutput = output.value(); } } - // Here, colorPassOutput can be either tonemapped or not; it's tonemapped if color gradding - // is done as a subpass. - if (colorGradingConfig.customResolve) { + assert_invariant(fg.getDescriptor(colorPassOutput.linearColor).samples <= 1); // TODO: we have to "uncompress" (i.e. detonemap) the color buffer here because it's used // by many other passes (Bloom, TAA, DoF, etc...). We could make this more // efficient by using ARM_shader_framebuffer_fetch. We use a load/store (i.e. // subpass) here because it's more convenient. - colorPassOutput = ppm.customResolveUncompressPass(fg, colorPassOutput); - blackboard["color"] = colorPassOutput; + colorPassOutput.linearColor = + ppm.customResolveUncompressPass(fg, colorPassOutput.linearColor); } // export the color buffer if screen-space reflections are enabled @@ -1148,8 +1155,7 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { builder.sideEffect(); // we can't use colorPassOutput here because it could be tonemapped - auto color = blackboard.get("color"); - data.history = builder.sample(color); // FIXME: an access must be declared for detach(), why? + data.history = builder.sample(colorPassOutput.linearColor); // FIXME: an access must be declared for detach(), why? }, [&view, projection](FrameGraphResources const& resources, auto const& data, backend::DriverApi&) { auto& history = view.getFrameHistory(); @@ -1159,7 +1165,14 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { }); } - FrameGraphId input = colorPassOutput; + // this is the output of the color pass / input to post processing, + // this is only used later for comparing it with the output after post-processing + FrameGraphId const postProcessInput = colorGradingConfig.asSubpass ? + colorPassOutput.tonemappedColor : + colorPassOutput.linearColor; + + // input can change below + FrameGraphId input = postProcessInput; fg.addTrivialSideEffectPass("Finish Color Passes", [&view](DriverApi& driver) { // Unbind SSAO sampler, b/c the FrameGraph will delete the texture at the end of the pass. view.cleanupRenderPasses(); @@ -1170,8 +1183,7 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { // if the depth is not used below or if the depth is not MS (e.g. it could have been // auto-resolved). // In practice, this is used on Vulkan and older Metal devices. - auto depth = blackboard.get("depth"); - depth = ppm.resolve(fg, "Resolved Depth Buffer", depth, { .levels = 1 }); + auto depth = ppm.resolve(fg, "Resolved Depth Buffer", colorPassOutput.depth, { .levels = 1 }); // Debug: CSM visualisation if (UTILS_UNLIKELY(engine.debug.shadowmap.visualize_cascades && @@ -1287,7 +1299,7 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { // The intermediate buffer is accomplished with a "fake" opaqueBlit (i.e. blit) operation. const bool outputIsSwapChain = - (input == colorPassOutput) && (viewRenderTarget == mRenderTargetHandle); + (input == postProcessInput) && (viewRenderTarget == mRenderTargetHandle); if (mightNeedFinalBlit) { if (blendModeTranslucent || xvp != svp || From ba8d429fcb75d02ee22872d2f1794198e67d8e5a Mon Sep 17 00:00:00 2001 From: Ben Doherty Date: Tue, 27 Aug 2024 10:49:35 -0700 Subject: [PATCH 20/26] Metal: annotate memory allocation failures with buffer tag (#8082) --- filament/backend/src/metal/MetalBuffer.h | 2 ++ filament/backend/src/metal/MetalBuffer.mm | 4 +-- filament/backend/src/metal/MetalDriver.h | 5 ++-- filament/backend/src/metal/MetalDriver.mm | 32 ++++++++++++++++++---- filament/backend/src/metal/MetalHandles.mm | 8 ------ 5 files changed, 33 insertions(+), 18 deletions(-) diff --git a/filament/backend/src/metal/MetalBuffer.h b/filament/backend/src/metal/MetalBuffer.h index bb3de6d27c9..265c2896238 100644 --- a/filament/backend/src/metal/MetalBuffer.h +++ b/filament/backend/src/metal/MetalBuffer.h @@ -160,6 +160,8 @@ class MetalBuffer { size_t size, bool forceGpuBuffer = false); ~MetalBuffer(); + [[nodiscard]] bool wasAllocationSuccessful() const noexcept { return mBuffer || mCpuBuffer; } + MetalBuffer(const MetalBuffer& rhs) = delete; MetalBuffer& operator=(const MetalBuffer& rhs) = delete; diff --git a/filament/backend/src/metal/MetalBuffer.mm b/filament/backend/src/metal/MetalBuffer.mm index 6070cf47fb3..a3eb80fa12d 100644 --- a/filament/backend/src/metal/MetalBuffer.mm +++ b/filament/backend/src/metal/MetalBuffer.mm @@ -53,8 +53,8 @@ mBuffer = { [context.device newBufferWithLength:size options:MTLResourceStorageModePrivate], TrackedMetalBuffer::Type::GENERIC }; } - FILAMENT_CHECK_POSTCONDITION(mBuffer) - << "Could not allocate Metal buffer of size " << size << "."; + // mBuffer might fail to be allocated. Clients can check for this by calling + // wasAllocationSuccessful(). } MetalBuffer::~MetalBuffer() { diff --git a/filament/backend/src/metal/MetalDriver.h b/filament/backend/src/metal/MetalDriver.h index e38b65b3755..6b9eac013e9 100644 --- a/filament/backend/src/metal/MetalDriver.h +++ b/filament/backend/src/metal/MetalDriver.h @@ -57,7 +57,6 @@ class MetalDriver final : public DriverBase { public: static Driver* create(MetalPlatform* platform, const Platform::DriverConfig& driverConfig); - void runAtNextTick(const std::function& fn) noexcept; private: @@ -73,10 +72,12 @@ class MetalDriver final : public DriverBase { /* * Tasks run regularly on the driver thread. + * Not thread-safe; tasks are run from the driver thead and must be enqueued from the driver + * thread. */ + void runAtNextTick(const std::function& fn) noexcept; void executeTickOps() noexcept; std::vector> mTickOps; - std::mutex mTickOpsLock; /* * Driver interface diff --git a/filament/backend/src/metal/MetalDriver.mm b/filament/backend/src/metal/MetalDriver.mm index b7d0a31deaa..b85d616c773 100644 --- a/filament/backend/src/metal/MetalDriver.mm +++ b/filament/backend/src/metal/MetalDriver.mm @@ -320,17 +320,40 @@ uint32_t vertexCount, Handle vbih) { MetalVertexBufferInfo const* const vbi = handle_cast(vbih); construct_handle(vbh, *mContext, vertexCount, vbi->bufferCount, vbih); + // No actual GPU memory is allocated here, so no need to check for allocation success. } void MetalDriver::createIndexBufferR(Handle ibh, ElementType elementType, uint32_t indexCount, BufferUsage usage) { - auto elementSize = (uint8_t) getElementTypeSize(elementType); - construct_handle(ibh, *mContext, usage, elementSize, indexCount); + auto elementSize = (uint8_t)getElementTypeSize(elementType); + auto* indexBuffer = + construct_handle(ibh, *mContext, usage, elementSize, indexCount); + auto& buffer = indexBuffer->buffer; + // If the allocation was not successful, postpone the error message until the next tick, to give + // Filament a chance to call setDebugTag on the handle; this way we get a nicer error message. + if (UTILS_UNLIKELY(!buffer.wasAllocationSuccessful())) { + const size_t byteCount = buffer.getSize(); + runAtNextTick([byteCount, this, ibh]() { + FILAMENT_CHECK_POSTCONDITION(false) + << "Could not allocate Metal index buffer of size " << byteCount + << ", tag=" << mHandleAllocator.getHandleTag(ibh.getId()).c_str_safe(); + }); + } } void MetalDriver::createBufferObjectR(Handle boh, uint32_t byteCount, BufferObjectBinding bindingType, BufferUsage usage) { - construct_handle(boh, *mContext, bindingType, usage, byteCount); + auto* bufferObject = + construct_handle(boh, *mContext, bindingType, usage, byteCount); + // If the allocation was not successful, postpone the error message until the next tick, to give + // Filament a chance to call setDebugTag on the handle; this way we get a nicer error message. + if (UTILS_UNLIKELY(!bufferObject->getBuffer()->wasAllocationSuccessful())) { + runAtNextTick([byteCount, this, boh]() { + FILAMENT_CHECK_POSTCONDITION(false) + << "Could not allocate Metal buffer of size " << byteCount + << ", tag=" << mHandleAllocator.getHandleTag(boh.getId()).c_str_safe(); + }); + } } void MetalDriver::createTextureR(Handle th, SamplerType target, uint8_t levels, @@ -2066,15 +2089,12 @@ } void MetalDriver::runAtNextTick(const std::function& fn) noexcept { - std::lock_guard const lock(mTickOpsLock); mTickOps.push_back(fn); } void MetalDriver::executeTickOps() noexcept { std::vector> ops; - mTickOpsLock.lock(); std::swap(ops, mTickOps); - mTickOpsLock.unlock(); for (const auto& f : ops) { f(); } diff --git a/filament/backend/src/metal/MetalHandles.mm b/filament/backend/src/metal/MetalHandles.mm index 9f3e8a502e8..938d4569d36 100644 --- a/filament/backend/src/metal/MetalHandles.mm +++ b/filament/backend/src/metal/MetalHandles.mm @@ -257,10 +257,6 @@ static inline MTLTextureUsage getMetalTextureUsage(TextureUsage usage) { } } -#ifndef FILAMENT_RELEASE_PRESENT_DRAWABLE_MAIN_THREAD -#define FILAMENT_RELEASE_PRESENT_DRAWABLE_MAIN_THREAD 1 -#endif - class PresentDrawableData { public: PresentDrawableData() = delete; @@ -279,14 +275,10 @@ static void maybePresentAndDestroyAsync(PresentDrawableData* that, bool shouldPr [that->mDrawable present]; } -#if FILAMENT_RELEASE_PRESENT_DRAWABLE_MAIN_THREAD == 1 // mDrawable is acquired on the driver thread. Typically, we would release this object on // the same thread, but after receiving consistent crash reports from within // [CAMetalDrawable dealloc], we suspect this object requires releasing on the main thread. dispatch_async(dispatch_get_main_queue(), ^{ cleanupAndDestroy(that); }); -#else - that->mDriver->runAtNextTick([that]() { cleanupAndDestroy(that); }); -#endif } private: From ba5413622f6ce5b918f54f9aa293014045565e1f Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Tue, 27 Aug 2024 15:33:31 -0700 Subject: [PATCH 21/26] Decouple subpass from buffer padding (#8085) Coupling subpass on/off with buffer padding has unintended consequences when we need to turn subpass off for tests. We uncouple them in this change. --- filament/src/details/Renderer.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/filament/src/details/Renderer.cpp b/filament/src/details/Renderer.cpp index bb743fca2bd..d91a9c42d36 100644 --- a/filament/src/details/Renderer.cpp +++ b/filament/src/details/Renderer.cpp @@ -698,13 +698,12 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) { CameraInfo cameraInfo = view.computeCameraInfo(engine); - // when colorgrading-as-subpass is active, we know that many other effects are disabled - // such as dof, bloom. Moreover, if fxaa and scaling are not enabled, we're essentially in - // a very fast rendering path -- in this case, we would need an extra blit to "resolve" the - // buffer padding (because there are no other pass that can do it as a side effect). - // In this case, it is better to skip the padding, which won't be helping much. - const bool noBufferPadding = (colorGradingConfig.asSubpass && !hasFXAA && !scaled) - || engine.debug.renderer.disable_buffer_padding; + // If fxaa and scaling are not enabled, we're essentially in a very fast rendering path -- in + // this case, we would need an extra blit to "resolve" the buffer padding (because there are no + // other pass that can do it as a side effect). In this case, it is better to skip the padding, + // which won't be helping much. + const bool noBufferPadding + = (!hasFXAA && !scaled) || engine.debug.renderer.disable_buffer_padding; // guardBand must be a multiple of 16 to guarantee the same exact rendering up to 4 mip levels. float const guardBand = guardBandOptions.enabled ? 16.0f : 0.0f; From 0e0f3a5518c05299b77ac2efd1c797d4701c70cd Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Tue, 27 Aug 2024 23:30:26 +0000 Subject: [PATCH 22/26] Release Filament 1.54.1 --- NEW_RELEASE_NOTES.md | 2 -- README.md | 4 ++-- RELEASE_NOTES.md | 4 ++++ android/gradle.properties | 2 +- ios/CocoaPods/Filament.podspec | 4 ++-- web/filament-js/package.json | 2 +- 6 files changed, 10 insertions(+), 8 deletions(-) diff --git a/NEW_RELEASE_NOTES.md b/NEW_RELEASE_NOTES.md index 01d998de3c5..4a1a9c7fa7e 100644 --- a/NEW_RELEASE_NOTES.md +++ b/NEW_RELEASE_NOTES.md @@ -7,5 +7,3 @@ for next branch cut* header. appropriate header in [RELEASE_NOTES.md](./RELEASE_NOTES.md). ## Release notes for next branch cut - -- Add a `name` API to Filament objects for debugging handle use-after-free assertions diff --git a/README.md b/README.md index 443168d2a6e..f0e9ca349de 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ repositories { } dependencies { - implementation 'com.google.android.filament:filament-android:1.54.0' + implementation 'com.google.android.filament:filament-android:1.54.1' } ``` @@ -51,7 +51,7 @@ Here are all the libraries available in the group `com.google.android.filament`: iOS projects can use CocoaPods to install the latest release: ```shell -pod 'Filament', '~> 1.54.0' +pod 'Filament', '~> 1.54.1' ``` ### Snapshots diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index c0680a55057..0bad42c9327 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,6 +7,10 @@ A new header is inserted each time a *tag* is created. Instead, if you are authoring a PR for the main branch, add your release note to [NEW_RELEASE_NOTES.md](./NEW_RELEASE_NOTES.md). +## v1.54.2 + +- Add a `name` API to Filament objects for debugging handle use-after-free assertions + ## v1.54.1 diff --git a/android/gradle.properties b/android/gradle.properties index f8284a4f77d..994ce214046 100644 --- a/android/gradle.properties +++ b/android/gradle.properties @@ -1,5 +1,5 @@ GROUP=com.google.android.filament -VERSION_NAME=1.54.0 +VERSION_NAME=1.54.1 POM_DESCRIPTION=Real-time physically based rendering engine for Android. diff --git a/ios/CocoaPods/Filament.podspec b/ios/CocoaPods/Filament.podspec index 8ec4c8d8cc2..1e87d19b7ee 100644 --- a/ios/CocoaPods/Filament.podspec +++ b/ios/CocoaPods/Filament.podspec @@ -1,12 +1,12 @@ Pod::Spec.new do |spec| spec.name = "Filament" - spec.version = "1.54.0" + spec.version = "1.54.1" spec.license = { :type => "Apache 2.0", :file => "LICENSE" } spec.homepage = "https://google.github.io/filament" spec.authors = "Google LLC." spec.summary = "Filament is a real-time physically based rendering engine for Android, iOS, Windows, Linux, macOS, and WASM/WebGL." spec.platform = :ios, "11.0" - spec.source = { :http => "https://github.com/google/filament/releases/download/v1.54.0/filament-v1.54.0-ios.tgz" } + spec.source = { :http => "https://github.com/google/filament/releases/download/v1.54.1/filament-v1.54.1-ios.tgz" } # Fix linking error with Xcode 12; we do not yet support the simulator on Apple silicon. spec.pod_target_xcconfig = { diff --git a/web/filament-js/package.json b/web/filament-js/package.json index 60f36d09e94..21ca5f2f834 100644 --- a/web/filament-js/package.json +++ b/web/filament-js/package.json @@ -1,6 +1,6 @@ { "name": "filament", - "version": "1.54.0", + "version": "1.54.1", "description": "Real-time physically based rendering engine", "main": "filament.js", "module": "filament.js", From f743bedef945420a6fcdf698dbb6995e07817ace Mon Sep 17 00:00:00 2001 From: Sungun Park Date: Tue, 27 Aug 2024 23:30:37 +0000 Subject: [PATCH 23/26] Bump version to 1.54.2 --- README.md | 4 ++-- android/gradle.properties | 2 +- ios/CocoaPods/Filament.podspec | 4 ++-- web/filament-js/package.json | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f0e9ca349de..c3f9a954aa2 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ repositories { } dependencies { - implementation 'com.google.android.filament:filament-android:1.54.1' + implementation 'com.google.android.filament:filament-android:1.54.2' } ``` @@ -51,7 +51,7 @@ Here are all the libraries available in the group `com.google.android.filament`: iOS projects can use CocoaPods to install the latest release: ```shell -pod 'Filament', '~> 1.54.1' +pod 'Filament', '~> 1.54.2' ``` ### Snapshots diff --git a/android/gradle.properties b/android/gradle.properties index 994ce214046..510e34eb0a4 100644 --- a/android/gradle.properties +++ b/android/gradle.properties @@ -1,5 +1,5 @@ GROUP=com.google.android.filament -VERSION_NAME=1.54.1 +VERSION_NAME=1.54.2 POM_DESCRIPTION=Real-time physically based rendering engine for Android. diff --git a/ios/CocoaPods/Filament.podspec b/ios/CocoaPods/Filament.podspec index 1e87d19b7ee..73fe0a78f81 100644 --- a/ios/CocoaPods/Filament.podspec +++ b/ios/CocoaPods/Filament.podspec @@ -1,12 +1,12 @@ Pod::Spec.new do |spec| spec.name = "Filament" - spec.version = "1.54.1" + spec.version = "1.54.2" spec.license = { :type => "Apache 2.0", :file => "LICENSE" } spec.homepage = "https://google.github.io/filament" spec.authors = "Google LLC." spec.summary = "Filament is a real-time physically based rendering engine for Android, iOS, Windows, Linux, macOS, and WASM/WebGL." spec.platform = :ios, "11.0" - spec.source = { :http => "https://github.com/google/filament/releases/download/v1.54.1/filament-v1.54.1-ios.tgz" } + spec.source = { :http => "https://github.com/google/filament/releases/download/v1.54.2/filament-v1.54.2-ios.tgz" } # Fix linking error with Xcode 12; we do not yet support the simulator on Apple silicon. spec.pod_target_xcconfig = { diff --git a/web/filament-js/package.json b/web/filament-js/package.json index 21ca5f2f834..7b502cf79ab 100644 --- a/web/filament-js/package.json +++ b/web/filament-js/package.json @@ -1,6 +1,6 @@ { "name": "filament", - "version": "1.54.1", + "version": "1.54.2", "description": "Real-time physically based rendering engine", "main": "filament.js", "module": "filament.js", From f5f1e5612391ea0c78ec6287ebbc7c1212e43582 Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Tue, 27 Aug 2024 23:17:26 -0700 Subject: [PATCH 24/26] Re-enable DebugRegistry::setProperty for release build (#8086) `DebugRegistry::setProperty` is no longer just applicable to debug builds. This change was previously added in 6c0bd36 but then reverted in a7317e7. We re-enable it and separate it from the shadow changes in this commit. --- filament/src/details/DebugRegistry.cpp | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/filament/src/details/DebugRegistry.cpp b/filament/src/details/DebugRegistry.cpp index c2f5d3d3fb6..8915cf312e1 100644 --- a/filament/src/details/DebugRegistry.cpp +++ b/filament/src/details/DebugRegistry.cpp @@ -28,12 +28,6 @@ #include #include -#ifndef NDEBUG -# define DEBUG_PROPERTIES_WRITABLE true -#else -# define DEBUG_PROPERTIES_WRITABLE false -#endif - using namespace filament::math; using namespace utils; @@ -79,17 +73,15 @@ bool FDebugRegistry::hasProperty(const char* name) const noexcept { template bool FDebugRegistry::setProperty(const char* name, T v) noexcept { - if constexpr (DEBUG_PROPERTIES_WRITABLE) { - auto info = getPropertyInfo(name); - T* const addr = static_cast(info.first); - if (addr) { - auto old = *addr; - *addr = v; - if (info.second && old != v) { - info.second(); - } - return true; + auto info = getPropertyInfo(name); + T* const addr = static_cast(info.first); + if (addr) { + auto old = *addr; + *addr = v; + if (info.second && old != v) { + info.second(); } + return true; } return false; } From b0d3f14243a3796d1e61dc3d3abcf477647536eb Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 30 Aug 2024 13:43:26 -0700 Subject: [PATCH 25/26] a handle with a tag would sometime return "no tag" this is because the key used to retrieve the tag was not "truncated" like it was when inserting the tag in the hash-map. --- .../include/private/backend/HandleAllocator.h | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/filament/backend/include/private/backend/HandleAllocator.h b/filament/backend/include/private/backend/HandleAllocator.h index b61f8e376f0..6a13b75e43b 100644 --- a/filament/backend/include/private/backend/HandleAllocator.h +++ b/filament/backend/include/private/backend/HandleAllocator.h @@ -208,14 +208,8 @@ class HandleAllocator { // TODO: for now, only pool handles check for use-after-free, so we only keep tags for // those if (isPoolHandle(id)) { - // Truncate the tag's age to N bits. - constexpr uint8_t N = 2; // support a history of 4 tags - constexpr uint8_t mask = (1 << N) - 1; - - uint8_t const age = (id & HANDLE_AGE_MASK) >> HANDLE_AGE_SHIFT; - uint8_t const newAge = age & mask; - uint32_t const key = (id & ~HANDLE_AGE_MASK) | (newAge << HANDLE_AGE_SHIFT); - + // Truncate the age to get the debug tag + uint32_t const key = id & ~(HANDLE_DEBUG_TAG_MASK ^ HANDLE_AGE_MASK); // This line is the costly part. In the future, we could potentially use a custom // allocator. mDebugTags[key] = std::move(tag); @@ -226,7 +220,8 @@ class HandleAllocator { if (!isPoolHandle(id)) { return "(no tag)"; } - if (auto pos = mDebugTags.find(id); pos != mDebugTags.end()) { + uint32_t const key = id & ~(HANDLE_DEBUG_TAG_MASK ^ HANDLE_AGE_MASK); + if (auto pos = mDebugTags.find(key); pos != mDebugTags.end()) { return pos->second; } return "(no tag)"; @@ -349,12 +344,24 @@ class HandleAllocator { } } - // we handle a 4 bits age per address - static constexpr uint32_t HANDLE_HEAP_FLAG = 0x80000000u; // pool vs heap handle - static constexpr uint32_t HANDLE_AGE_MASK = 0x78000000u; // handle's age - static constexpr uint32_t HANDLE_INDEX_MASK = 0x07FFFFFFu; // handle index - static constexpr uint32_t HANDLE_TAG_MASK = HANDLE_AGE_MASK; - static constexpr uint32_t HANDLE_AGE_SHIFT = 27; + // number if bits allotted to the handle's age (currently 4 max) + static constexpr uint32_t HANDLE_AGE_BIT_COUNT = 4; + // number if bits allotted to the handle's debug tag (HANDLE_AGE_BIT_COUNT max) + static constexpr uint32_t HANDLE_DEBUG_TAG_BIT_COUNT = 2; + // bit shift for both the age and debug tag + static constexpr uint32_t HANDLE_AGE_SHIFT = 27; + // mask for the heap (vs pool) flag + static constexpr uint32_t HANDLE_HEAP_FLAG = 0x80000000u; + // mask for the age + static constexpr uint32_t HANDLE_AGE_MASK = + ((1 << HANDLE_AGE_BIT_COUNT) - 1) << HANDLE_AGE_SHIFT; + // mask for the debug tag + static constexpr uint32_t HANDLE_DEBUG_TAG_MASK = + ((1 << HANDLE_DEBUG_TAG_BIT_COUNT) - 1) << HANDLE_AGE_SHIFT; + // mask for the index + static constexpr uint32_t HANDLE_INDEX_MASK = 0x07FFFFFFu; + + static_assert(HANDLE_DEBUG_TAG_BIT_COUNT <= HANDLE_AGE_BIT_COUNT); static bool isPoolHandle(HandleBase::HandleId id) noexcept { return (id & HANDLE_HEAP_FLAG) == 0u; @@ -369,7 +376,7 @@ class HandleAllocator { // a non-pool handle. if (UTILS_LIKELY(isPoolHandle(id))) { char* const base = (char*)mHandleArena.getArea().begin(); - uint32_t const tag = id & HANDLE_TAG_MASK; + uint32_t const tag = id & HANDLE_AGE_MASK; size_t const offset = (id & HANDLE_INDEX_MASK) * Allocator::getAlignment(); return { static_cast(base + offset), tag }; } @@ -384,7 +391,7 @@ class HandleAllocator { size_t const offset = (char*)p - base; assert_invariant((offset % Allocator::getAlignment()) == 0); auto id = HandleBase::HandleId(offset / Allocator::getAlignment()); - id |= tag & HANDLE_TAG_MASK; + id |= tag & HANDLE_AGE_MASK; assert_invariant((id & HANDLE_HEAP_FLAG) == 0); return id; } From 422fcea2cf0e5beadf854c668860b83522672364 Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Wed, 4 Sep 2024 09:36:54 -0700 Subject: [PATCH 26/26] Make builderMakeName public This call is used in the BuilderNameMixin template definition, which is a public API. --- filament/include/filament/FilamentAPI.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/filament/include/filament/FilamentAPI.h b/filament/include/filament/FilamentAPI.h index 7a6f16d6307..7fa19433847 100644 --- a/filament/include/filament/FilamentAPI.h +++ b/filament/include/filament/FilamentAPI.h @@ -55,7 +55,8 @@ class UTILS_PUBLIC FilamentAPI { template using BuilderBase = utils::PrivateImplementation; -void builderMakeName(utils::CString& outName, const char* name, size_t len) noexcept; +// This needs to be public because it is used in the following template. +UTILS_PUBLIC void builderMakeName(utils::CString& outName, const char* name, size_t len) noexcept; template class UTILS_PUBLIC BuilderNameMixin {