From 61c4df95039b71b1c66d55b18209e86f7a7812f8 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 ed6a507a766..a799acac52c 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 826994c3656..fc76f5e38d8 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 883100e2997..676b539be7e 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 5f3c8d50802..b4552d5c21c 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