From ae50a504aebbcfa3f970dad86fba7bc474b03cc0 Mon Sep 17 00:00:00 2001 From: "Eunki, Hong" Date: Thu, 8 Dec 2022 18:29:52 +0900 Subject: [PATCH] Fix attribute cache bug 1. It is possible that GL_ACTIVE_ATTRIBUTES value is smaller than location of attributes. If it happend, crashed due to mVertexInputAttributes.insert call. To fix this crash issue, make mVertexInputAttributes size relative with location value, not relative with GL_ACTIVE_ATTRIBUTES. Also, It will guarantee that attribute's location can be index of it's real attrubutes. (Since vector.insert operation shift the index, it was depend on the location input order. and also slow) 2. Ensure the enable of vertex attributes. Since vertexInputState.attributes generated by Geometry in pipeline-cache system, Change the attribute's list of geometry at the same shader was not worked. TODO : Make VAO caching system works well. Currently, VAO doesn't useful. We should found a way to EnableVertexAttribArray only with shader's information, not depend on Geometry information. or, we might need to calculate vertex attribute location list's hash, and use it as key. Change-Id: I9f2c15617d279cfefbe1f8d72e5a9cbe43a7c564 --- .../dali-test-suite-utils/test-gl-abstraction.h | 3 +++ dali/internal/graphics/gles-impl/gles-context.cpp | 10 ++++++++++ .../graphics/gles-impl/gles-graphics-reflection.cpp | 20 +++++++++++++++----- .../graphics/gles-impl/gles-graphics-reflection.h | 4 ++-- 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/automated-tests/src/dali-adaptor/dali-test-suite-utils/test-gl-abstraction.h b/automated-tests/src/dali-adaptor/dali-test-suite-utils/test-gl-abstraction.h index 5e5fde3..7ba7834 100644 --- a/automated-tests/src/dali-adaptor/dali-test-suite-utils/test-gl-abstraction.h +++ b/automated-tests/src/dali-adaptor/dali-test-suite-utils/test-gl-abstraction.h @@ -885,6 +885,9 @@ public: case GL_ACTIVE_ATTRIBUTE_MAX_LENGTH: *params = 100; break; + case GL_ACTIVE_ATTRIBUTES: + *params = static_cast(mAttribLocs.size()); + break; } } diff --git a/dali/internal/graphics/gles-impl/gles-context.cpp b/dali/internal/graphics/gles-impl/gles-context.cpp index 1d981bc..b75b08b 100644 --- a/dali/internal/graphics/gles-impl/gles-context.cpp +++ b/dali/internal/graphics/gles-impl/gles-context.cpp @@ -64,6 +64,13 @@ struct Context::Impl mProgramVAOCurrentState = iter->second; gl.BindVertexArray(iter->second); } + + // We should re-check enable attribute usage because geometry might be changed. + // @todo : We can remove this loop if we enable vertex attrib by shader's information. + for(const auto& attr : vertexInputState.attributes) + { + gl.EnableVertexAttribArray(attr.location); + } return; } @@ -71,6 +78,9 @@ struct Context::Impl gl.GenVertexArrays(1, &vao); gl.BindVertexArray(vao); mProgramVAOMap[program] = vao; + + // @todo : Enable vertex attrib only by shader's information, not with Geometry. + // Currently, vertexInputState.attributes depend on Geometry's VertexBuffer. for(const auto& attr : vertexInputState.attributes) { gl.EnableVertexAttribArray(attr.location); diff --git a/dali/internal/graphics/gles-impl/gles-graphics-reflection.cpp b/dali/internal/graphics/gles-impl/gles-graphics-reflection.cpp index 05776c2..abb066b 100644 --- a/dali/internal/graphics/gles-impl/gles-graphics-reflection.cpp +++ b/dali/internal/graphics/gles-impl/gles-graphics-reflection.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2021 Samsung Electronics Co., Ltd. + * Copyright (c) 2022 Samsung Electronics Co., Ltd. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -220,6 +220,8 @@ void Reflection::BuildVertexAttributeReflection() mVertexInputAttributes.clear(); mVertexInputAttributes.resize(nAttribs); + int maximumLocation = nAttribs - 1; + name = new GLchar[maxLength]; for(int i = 0; i < nAttribs; i++) { @@ -228,13 +230,21 @@ void Reflection::BuildVertexAttributeReflection() if(location >= 0) { + if(maximumLocation < location) + { + maximumLocation = location; + // Increate continer size s.t. we can use maximumLocation as index. + mVertexInputAttributes.resize(maximumLocation + 1u); + } + AttributeInfo attributeInfo; - attributeInfo.location = location; - attributeInfo.name = name; - attributeInfo.format = GetVertexAttributeTypeFormat(type); - mVertexInputAttributes.insert(mVertexInputAttributes.begin() + location, attributeInfo); + attributeInfo.location = location; + attributeInfo.name = name; + attributeInfo.format = GetVertexAttributeTypeFormat(type); + mVertexInputAttributes[location] = std::move(attributeInfo); } } + delete[] name; } diff --git a/dali/internal/graphics/gles-impl/gles-graphics-reflection.h b/dali/internal/graphics/gles-impl/gles-graphics-reflection.h index 06cfba1..d1bca95 100644 --- a/dali/internal/graphics/gles-impl/gles-graphics-reflection.h +++ b/dali/internal/graphics/gles-impl/gles-graphics-reflection.h @@ -2,7 +2,7 @@ #define DALI_GRAPHICS_GLES_REFLECTION_H /* - * Copyright (c) 2021 Samsung Electronics Co., Ltd. + * Copyright (c) 2022 Samsung Electronics Co., Ltd. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -254,7 +254,7 @@ private: struct AttributeInfo { - uint32_t location{}; + uint32_t location{ERROR_ATTRIBUTE_NOT_FOUND}; std::string name{}; Dali::Graphics::VertexInputAttributeFormat format{}; }; -- 2.7.4