From d5a274fa25dd85f127578db58bc7da1ee02f772d Mon Sep 17 00:00:00 2001 From: Ben Doherty Date: Tue, 20 Aug 2024 12:28:13 -0700 Subject: [PATCH 1/3] 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 9654545825..a7933407bb 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 3b4724b095..2972480e71 100644 --- a/filament/backend/src/opengl/OpenGLContext.h +++ b/filament/backend/src/opengl/OpenGLContext.h @@ -316,12 +316,6 @@ public: // 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 @@ private: { 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 0c01d383b3..eb6876859b 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 2/3] 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 d465a03288..c4f14e4ce1 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 3/3] 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 8747f59d8d..f0d907a0f7 100644 --- a/libs/viewer/include/viewer/AutomationEngine.h +++ b/libs/viewer/include/viewer/AutomationEngine.h @@ -224,6 +224,9 @@ public: */ 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 19d23ef168..810df99dbb 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 b5c8e1b708..e3dc4363a3 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;