bugfix #540 : immutable texture can not be resized by glTexImage2D
#541
Conversation
WalkthroughThe pull request refines texture management and error logging throughout the project. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TextureObj as TextureObject::resize
participant GL as GL API
Caller->>TextureObj: call resize(buffer, width, height, format)
alt Texture is valid, dimensions match, and buffer is null
TextureObj->>Caller: return false
else Width or height is zero
TextureObj->>Caller: assert & return false
else Texture is zero or dimensions differ
TextureObj->>TextureObj: Delete existing texture (if any)
TextureObj->>GL: Generate new texture using buffer
TextureObj->>Caller: update texture size & return success
else Texture exists & dimensions match
TextureObj->>GL: Bind texture and update with glTexSubImage2D
TextureObj->>Caller: return success
end
sequenceDiagram
participant VP as CGEVideoPlayerYUV420P
participant GL as GL API
VP->>VP: Constructor: bind attributes, retrieve uniform locations
VP->>VP: Call updateVideoFrame(frameData, linesize)
alt Texture Y location is zero or linesize changed
VP->>GL: Generate new textures from framebuffer data
VP->>VP: Update uniform values accordingly
else
VP->>GL: Update existing textures
end
VP->>VP: Log error if any uniform location is invalid (in DEBUG)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
library/src/main/jni/cge/common/cgeGLFunctions.cpp (1)
276-280: Improved input validation for width and heightAdding explicit validation for zero dimensions is a good practice. However, the error message could be more specific about which parameter must not be zero.
- assert(0 && (void*)"TextureObject::resize must not be 0!"); + assert(0 && (void*)"TextureObject::resize - width and height must not be 0!");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
library/src/main/jni/cge/common/cgeGLFunctions.cpp(2 hunks)library/src/main/jni/cge/common/cgeGLFunctions.h(1 hunks)library/src/main/jni/cge/common/cgeShaderFunctions.h(1 hunks)library/src/main/jni/interface/cgeVideoPlayer.cpp(3 hunks)library/src/main/jni/interface/cgeVideoPlayer.h(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
library/src/main/jni/interface/cgeVideoPlayer.cpp (1)
library/src/main/jni/interface/cgeVideoPlayer.h (6) (6)
CGEVideoPlayerYUV420P(46-46)CGEVideoPlayerYUV420P(47-47)m_texYUV(74-74)m_texYUV(75-75)m_texYUV(76-76)m_linesize(80-80)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
library/src/main/jni/cge/common/cgeGLFunctions.h (1)
53-54: Code style improvement: Consistent comment formattingAdded spaces at the beginning of comments to improve readability and maintain consistent style throughout the codebase.
library/src/main/jni/cge/common/cgeShaderFunctions.h (1)
227-230: Improved error logging by using explicit conditional compilationReplaced
CGE_LOG_CODEmacro with a direct#ifdef DEBUGconditional to make the error logging behavior more explicit. This ensures that error messages about missing uniforms only appear in debug builds and are completely excluded from release builds, improving performance.library/src/main/jni/interface/cgeVideoPlayer.cpp (1)
72-100: Improved constructor implementation by moving initialization to class definitionRemoved redundant member initializations from the constructor since they're now properly initialized in the class definition. This is a cleaner approach that ensures all members have defined default values.
Additionally, the uniform assignments were moved inside an else block to only execute if the texture locations are valid, improving error handling.
library/src/main/jni/interface/cgeVideoPlayer.h (2)
51-51: Code style improvement: Added space in commentAdded space after comment delimiter for improved readability and consistent style.
84-92: Improved initialization of member variablesAdded in-class initialization for member variables to ensure they have defined default values before the constructor runs. This approach is more robust as it guarantees initialization even if constructor code changes, and follows modern C++ best practices.
- Texture IDs initialize to zero with
{}- Texture locations explicitly initialize to -1
- Pointers initialize to nullptr
- Other numeric values initialize to zero
This change provides better protection against undefined behavior and complements the constructor changes.
library/src/main/jni/cge/common/cgeGLFunctions.cpp (4)
271-274: Good early return optimization for texture resizingThis change correctly implements an early return when no resize is necessary (texture exists, dimensions match, and no new buffer data). This guards against unnecessary texture operations, making the code more efficient.
282-312: Fixed texture recreation handling for dimension changesThis change properly addresses the core issue with immutable textures. When dimensions change, the code now correctly deletes the existing texture and creates a new one instead of trying to resize an immutable texture with glTexImage2D, which isn't allowed by OpenGL specifications.
313-318: Correct usage of glTexSubImage2D for content updatesUsing glTexSubImage2D to update the content of an existing texture (when dimensions are unchanged) follows OpenGL best practices. This approach avoids the expensive operation of recreating textures when only the content changes.
320-320: Proper function return valueEnsuring the function returns true when texture operations are successfully performed is correct and maintains consistency with the function's contract.
…Image2D` can not be used to change the texture size.
wysaid
force-pushed
the
fix_texture_resize
branch
from
3562e5f to
90578d3
Compare
bugfix:
immutable textureis used when creating textures, soglTexImage2Dcan not be used to change the texture size.#540
Summary by CodeRabbit