From 8db5e700d7e07ddc4eab769760344c446dc35926 Mon Sep 17 00:00:00 2001
From: Mathias Agopian <mathias@google.com>
Date: Fri, 29 Mar 2024 14:19:45 -0700
Subject: [PATCH] GL backend should only allocate renderbuffers when possible

There was several cases where the the gl backend would wrongly  use a
renderbuffer instead of a texture.


BUGS=[329491941]
---
 filament/backend/src/opengl/OpenGLDriver.cpp | 18 +++++++------
 filament/src/PostProcessManager.cpp          |  5 +---
 filament/src/RendererUtils.cpp               | 27 ++++++++++++++------
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp
index 196d1bd2e6..81cc54fb83 100644
--- a/filament/backend/src/opengl/OpenGLDriver.cpp
+++ b/filament/backend/src/opengl/OpenGLDriver.cpp
@@ -724,10 +724,19 @@ void OpenGLDriver::createTextureR(Handle<HwTexture> th, SamplerType target, uint
     GLenum internalFormat = getInternalFormat(format);
     assert_invariant(internalFormat);
 
-    if (UTILS_UNLIKELY(usage & TextureUsage::PROTECTED)) {
+    if (any(usage & TextureUsage::PROTECTED)) {
         // renderbuffers don't have a protected mode, so we need to use a texture
         // because protected textures are only supported on GLES 3.2, MSAA will be available.
         usage |= TextureUsage::SAMPLEABLE;
+    } else if (any(usage & TextureUsage::UPLOADABLE)) {
+        // if we have the uploadable flag, we also need to use a texture
+        usage |= TextureUsage::SAMPLEABLE;
+    } else if (target != SamplerType::SAMPLER_2D) {
+        // renderbuffers can only be 2D
+        usage |= TextureUsage::SAMPLEABLE;
+    } else if (levels > 1) {
+        // renderbuffers can't have mip levels
+        usage |= TextureUsage::SAMPLEABLE;
     }
 
     auto& gl = mContext;
@@ -803,13 +812,6 @@ void OpenGLDriver::createTextureR(Handle<HwTexture> th, SamplerType target, uint
             textureStorage(t, w, h, depth, bool(usage & TextureUsage::PROTECTED));
         }
     } else {
-        assert_invariant(any(usage & (
-                TextureUsage::COLOR_ATTACHMENT |
-                TextureUsage::DEPTH_ATTACHMENT |
-                TextureUsage::STENCIL_ATTACHMENT)));
-        assert_invariant(levels == 1);
-        assert_invariant(target == SamplerType::SAMPLER_2D);
-        assert_invariant(none(usage & TextureUsage::PROTECTED));
         t->gl.internalFormat = internalFormat;
         t->gl.target = GL_RENDERBUFFER;
         glGenRenderbuffers(1, &t->gl.id);
diff --git a/filament/src/PostProcessManager.cpp b/filament/src/PostProcessManager.cpp
index 3f21ce872d..3fbd7f4144 100644
--- a/filament/src/PostProcessManager.cpp
+++ b/filament/src/PostProcessManager.cpp
@@ -450,11 +450,8 @@ PostProcessManager::StructurePassOutput PostProcessManager::structure(FrameGraph
                         .levels = uint8_t(levelCount),
                         .format = isES2 ? TextureFormat::DEPTH24 : TextureFormat::DEPTH32F });
 
-                // workaround: since we have levels, this implies SAMPLEABLE (because of the gl
-                // backend, which implements non-sampleables with renderbuffers, which don't have levels).
-                // (should the gl driver revert to textures, in that case?)
                 data.depth = builder.write(data.depth,
-                        FrameGraphTexture::Usage::DEPTH_ATTACHMENT | FrameGraphTexture::Usage::SAMPLEABLE);
+                        FrameGraphTexture::Usage::DEPTH_ATTACHMENT);
 
                 if (config.picking) {
                     data.picking = builder.createTexture("Picking Buffer", {
diff --git a/filament/src/RendererUtils.cpp b/filament/src/RendererUtils.cpp
index 8c63be153e..3e1cafdbc9 100644
--- a/filament/src/RendererUtils.cpp
+++ b/filament/src/RendererUtils.cpp
@@ -16,17 +16,35 @@
 
 #include "RendererUtils.h"
 
+#include "PostProcessManager.h"
+
 #include "details/Engine.h"
 #include "details/View.h"
 
 #include "fg/FrameGraph.h"
 #include "fg/FrameGraphId.h"
 #include "fg/FrameGraphResources.h"
+#include "fg/FrameGraphTexture.h"
 
+#include <filament/Options.h>
+#include <filament/RenderableManager.h>
+#include <filament/Viewport.h>
+
+#include <backend/DriverEnums.h>
+#include <backend/Handle.h>
+#include <backend/PixelBufferDescriptor.h>
+
+#include <utils/BitmaskEnum.h>
 #include <utils/compiler.h>
 #include <utils/debug.h>
 #include <utils/Panic.h>
 
+#include <algorithm>
+#include <utility>
+
+#include <stddef.h>
+#include <stdint.h>
+
 namespace filament {
 
 using namespace backend;
@@ -153,16 +171,9 @@ FrameGraphId<FrameGraphTexture> RendererUtils::colorPass(
                 data.depth = builder.read(data.depth, FrameGraphTexture::Usage::DEPTH_ATTACHMENT);
 
                 data.color = builder.write(data.color, FrameGraphTexture::Usage::COLOR_ATTACHMENT);
+                data.depth = builder.write(data.depth, FrameGraphTexture::Usage::DEPTH_ATTACHMENT);
                 if (engine.getConfig().stereoscopicType == StereoscopicType::MULTIVIEW) {
-                    // Add sampleable usage flag for depth in multiview rendering, othewise it's
-                    // treated as renderbuffer in the backend and crashed.
-                    data.depth = builder.write(data.depth,
-                        FrameGraphTexture::Usage::DEPTH_ATTACHMENT |
-                        FrameGraphTexture::Usage::SAMPLEABLE);
                     layerCount = engine.getConfig().stereoscopicEyeCount;
-                } else {
-                    data.depth = builder.write(data.depth,
-                        FrameGraphTexture::Usage::DEPTH_ATTACHMENT);
                 }
 
                 /*