From cf7360bf8b9400478ed8190bf1389ee3bba7e556 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 8 Nov 2024 13:00:51 -0800 Subject: [PATCH] fix uninitialized variable on ES2 devices On ES2 devices (or in forceES2 mode), we emulate the sRGB swapchain in the shader if the h/w doesn't support it. In that case, the emulation is controlled by a uniform that technically lives in the frameUniforms block. However, the frameUniforms buffer is not updated, instead, the uniform is manually set. Unfortunately, the UBO emulation overrides it with the uninitialized variable. BUGS=[377913730] --- .../filament/hellotriangle/MainActivity.kt | 12 +++++-- filament/backend/src/opengl/OpenGLContext.cpp | 18 +++++----- filament/backend/src/opengl/OpenGLDriver.cpp | 33 +++++++++---------- filament/backend/src/opengl/OpenGLProgram.cpp | 4 ++- 4 files changed, 36 insertions(+), 31 deletions(-) diff --git a/android/samples/sample-hello-triangle/src/main/java/com/google/android/filament/hellotriangle/MainActivity.kt b/android/samples/sample-hello-triangle/src/main/java/com/google/android/filament/hellotriangle/MainActivity.kt index ed6a507a766b..a799acac52cb 100644 --- a/android/samples/sample-hello-triangle/src/main/java/com/google/android/filament/hellotriangle/MainActivity.kt +++ b/android/samples/sample-hello-triangle/src/main/java/com/google/android/filament/hellotriangle/MainActivity.kt @@ -112,7 +112,13 @@ class MainActivity : Activity() { } private fun setupFilament() { - engine = Engine.Builder().featureLevel(Engine.FeatureLevel.FEATURE_LEVEL_0).build() + val config = Engine.Config() + //config.forceGLES2Context = true + + engine = Engine.Builder() + .config(config) + .featureLevel(Engine.FeatureLevel.FEATURE_LEVEL_0) + .build() renderer = engine.createRenderer() scene = engine.createScene() view = engine.createView() @@ -123,7 +129,9 @@ class MainActivity : Activity() { scene.skybox = Skybox.Builder().color(0.035f, 0.035f, 0.035f, 1.0f).build(engine) // post-processing is not supported at feature level 0 - view.isPostProcessingEnabled = false + if (engine.activeFeatureLevel == Engine.FeatureLevel.FEATURE_LEVEL_0) { + view.isPostProcessingEnabled = false + } // Tell the view which camera we want to use view.camera = camera diff --git a/filament/backend/src/opengl/OpenGLContext.cpp b/filament/backend/src/opengl/OpenGLContext.cpp index 826994c3656d..fc76f5e38d80 100644 --- a/filament/backend/src/opengl/OpenGLContext.cpp +++ b/filament/backend/src/opengl/OpenGLContext.cpp @@ -614,7 +614,7 @@ FeatureLevel OpenGLContext::resolveFeatureLevel(GLint major, GLint minor, featureLevel = FeatureLevel::FEATURE_LEVEL_2; if (gets.max_texture_image_units >= MAX_FRAGMENT_SAMPLER_COUNT && gets.max_combined_texture_image_units >= - (MAX_FRAGMENT_SAMPLER_COUNT + MAX_VERTEX_SAMPLER_COUNT)) { + (MAX_FRAGMENT_SAMPLER_COUNT + MAX_VERTEX_SAMPLER_COUNT)) { featureLevel = FeatureLevel::FEATURE_LEVEL_3; } } @@ -623,15 +623,13 @@ FeatureLevel OpenGLContext::resolveFeatureLevel(GLint major, GLint minor, # ifndef IOS // IOS is guaranteed to have ES3.x else if (UTILS_UNLIKELY(major == 2)) { // Runtime OpenGL version is ES 2.x -# if defined(BACKEND_OPENGL_LEVEL_GLES30) - // mandatory extensions (all supported by Mali-400 and Adreno 304) - assert_invariant(exts.OES_depth_texture); - assert_invariant(exts.OES_depth24); - assert_invariant(exts.OES_packed_depth_stencil); - assert_invariant(exts.OES_rgb8_rgba8); - assert_invariant(exts.OES_standard_derivatives); - assert_invariant(exts.OES_texture_npot); -# endif + // note: mandatory extensions (all supported by Mali-400 and Adreno 304) + // OES_depth_texture + // OES_depth24 + // OES_packed_depth_stencil + // OES_rgb8_rgba8 + // OES_standard_derivatives + // OES_texture_npot featureLevel = FeatureLevel::FEATURE_LEVEL_0; } # endif // IOS diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp index 521b23ac634e..e5d535f036f3 100644 --- a/filament/backend/src/opengl/OpenGLDriver.cpp +++ b/filament/backend/src/opengl/OpenGLDriver.cpp @@ -394,24 +394,21 @@ void OpenGLDriver::bindTexture(GLuint unit, GLTexture const* t) noexcept { } bool OpenGLDriver::useProgram(OpenGLProgram* p) noexcept { - if (UTILS_UNLIKELY(mBoundProgram == p)) { - // program didn't change, don't do anything. - return true; - } - - // compile/link the program if needed and call glUseProgram - bool const success = p->use(this, mContext); - assert_invariant(success == p->isValid()); - - if (success) { - // TODO: we could even improve this if the program could tell us which of the descriptors - // bindings actually changed. In practice, it is likely that set 0 or 1 might not - // change often. - decltype(mInvalidDescriptorSetBindings) changed; - changed.setValue((1 << MAX_DESCRIPTOR_SET_COUNT) - 1); - mInvalidDescriptorSetBindings |= changed; - - mBoundProgram = p; + bool success = true; + if (mBoundProgram != p) { + // compile/link the program if needed and call glUseProgram + success = p->use(this, mContext); + assert_invariant(success == p->isValid()); + if (success) { + // TODO: we could even improve this if the program could tell us which of the descriptors + // bindings actually changed. In practice, it is likely that set 0 or 1 might not + // change often. + decltype(mInvalidDescriptorSetBindings) changed; + changed.setValue((1 << MAX_DESCRIPTOR_SET_COUNT) - 1); + mInvalidDescriptorSetBindings |= changed; + + mBoundProgram = p; + } } if (UTILS_UNLIKELY(mContext.isES2() && success)) { diff --git a/filament/backend/src/opengl/OpenGLProgram.cpp b/filament/backend/src/opengl/OpenGLProgram.cpp index 5f3c8d50802c..b4552d5c21c6 100644 --- a/filament/backend/src/opengl/OpenGLProgram.cpp +++ b/filament/backend/src/opengl/OpenGLProgram.cpp @@ -255,7 +255,9 @@ void OpenGLProgram::updateUniforms( for (size_t i = 0, c = records.uniforms.size(); i < c; i++) { Program::Uniform const& u = records.uniforms[i]; GLint const loc = records.locations[i]; - if (loc < 0) { + // mRec709Location is special, it is handled by setRec709ColorSpace() and the corresponding + // entry in `buffer` is typically not initialized, so we skip it. + if (loc < 0 || loc == mRec709Location) { continue; } // u.offset is in 'uint32_t' units