Fix null pointer access and avoid nested allocations with QOpenGLTexture - #13043
Conversation
|
I don't see the problem with using a unique_ptr, but I do not oppose this change. It can be integrated in both 2.4 and main IMO. Thanks for taking care of this while I was away! Approved with one request for a comment. I propose you merge it yourself, @daschuer, if you also want to take care of 2.4. |
| #include <QSharedPointer> | ||
|
|
||
| class Paintable; | ||
|
|
There was a problem hiding this comment.
maybe add a comment that explains that this is an extension of QOpenGLTexture, with additional methods to set the texture data, and default settings for filter and wrap mode.
| std::unique_ptr<QOpenGLTexture> generateTexture(float markerLength, | ||
| void generateTexture(OpenGLTexture2D* pTexture, |
There was a problem hiding this comment.
I really don't see how the tradeoff in code quality makes sense. There is no reason to introduce an out-parameter here IMO. If the extra allocation bothers you, why not return it by value directly?
There was a problem hiding this comment.
This is this no out parameter which may be considered as bad code quality these days. It just updates the texture via pointer because OpenGLTexture2D has no copy constructor . I kept just to avoid moving code around. I will now make it a normal member function as usual and rename it to avoid future confusion.
There was a problem hiding this comment.
Refactored to become drawPrerollImage() returning the temporary QImage.
|
I tested this on Windows 11 and it works stable. The review comment by Swiftb0y is addressed. Code LGTM! Thank you for taking care! |
4 participants
This adds a missing null check in ec4bf17 to fix #12580
During testing I have noticed that QOpenGLTexture itself is only a smart pointer to the texture storage. This can also potentially be null. So I refactored the code to avoid a second allocation to store a pointer to QOpenGLTexture.
The last commit is only refactoring. Can this also be integrated in 2.4 or should I split it out for main?