Fixing the example with gpu+imshow [+opengl] #25579
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The existing code has a memory leak but the sample doesn't throw an exception. By passing a Wouldn't it be better to remove all the references to OpenGL as the workaround for #22328 causes a memory leak so it is not recommended to pass |
|
If you compile with QT6+opengl+cuda, passing in the gpumat for rendering does work. It took me a while to debug to figure out why sometimes the rendering won't work, and sometimes it would. |
Can the check also test for the presence of QT6? |
|
I don't know which test you are referring to, but here is the build info: |
cudawarped
reviewed
May 14, 2024
samples/gpu/video_reader.cpp
Outdated
| @@ -20,24 +21,26 @@ int main(int argc, const char* argv[]) | |||
| return -1; | |||
|
|
|||
| const std::string fname(argv[1]); | |||
| cv::VideoCapture reader(fname); | |||
| cv::Size imageSize{(int)reader.get(cv::CAP_PROP_FRAME_WIDTH), | |||
| (int)reader.get(cv::CAP_PROP_FRAME_HEIGHT)}; | |||
|
|
|||
| cv::namedWindow("CPU", cv::WINDOW_NORMAL); | |||
| #if defined(HAVE_OPENGL) | |||
There was a problem hiding this comment.
Should this (#if defined(HAVE_OPENGL)) also check for the presence of QT6 if this crashes the sample without it?
There was a problem hiding this comment.
The reader nor the imageSize is dependent on the QT6. Is it crashing on this?
There was a problem hiding this comment.
The reader is moved in earlier from below from the previous sample code, to extract the image size of the frames.
There was a problem hiding this comment.
#if defined(HAVE_OPENGL)
cv::imshow("GPU", d_frame);
#else
d_frame.download(frame);
cv::imshow("GPU", frame);
#endif
According to your previous comment, if OpenCV is built against OpenGL but not against QT6 `cv::imshow("GPU", d_frame) will cause the sample to crash. To prevent this you need to be able to check for QT6 at compile or runtime. e.g.
#if defined(HAVE_OPENGL) && defined(HAVE_QT6)
There was a problem hiding this comment.
I haven't checked for other GUI backends and I was assuming to work similarly. But I can add the check to enforce QT6 guard.
There was a problem hiding this comment.
Where is HAVE_QT6 defined? It is not in #include "cvconfig.h".
There was a problem hiding this comment.
Where is
HAVE_QT6defined? It is not in#include "cvconfig.h".
I don't think it is, it was an example of the test that needs to be performed.
The point of my comments is that you have taken a sample which doen't crash when run and converted it into a sample that will crash when using the default GUI backend which most users will have compiled OpenCV against. Do you think this will be useful to people who run the sample who don't realize they need QT6, or do you think they will see the crash and assume the cudacodec::VideoReader class has a bug?
There was a problem hiding this comment.
Maybe I should split the sample into two then, one purely for demonstrating cudacodec::VideoReader parsing, and other using opengl+gpumat for imshow. Would this be clearer?
There was a problem hiding this comment.
If the new sample still crashes then I don't think its clearer.
There are several options, not limited to:
- Find a way to check for working GUI backends at compile/runtime. If the GUI's are plugins then I would have thought you could do this at runtime.
- Check for exceptions and print an informative message combined with a flag to enable or disable calling
cv::imshowwith aGpuMat. - ...
There was a problem hiding this comment.
I have now introduced a new flag HAVE_QT_OPENGL for checking both qt and opengl in the new commit. Please have a look.
|
@bjornborg Looking at the code I'm not sure if the inteded behaviour is for this to work when OpenGL is used in combination with QT, it looks like it works under these circumstances becauase it falls through the initial checks when there is a different back end (not Win32) but I may be wrong. @opencv-alalek is this correct? If so I would remove all code which uses |
|
Well, it beats me if it wasn't intended to work. It has been working for some time when OpenGL + QT + cuda combination is enabled on Linux, and it is quite efficient at rendering as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Patches the example of passing gpumat to GUI.
It does not fix the underlying issue with imshow, opengl, and cuda gpumat when trying to render the GPU data to the window, but by specifying the texture size (through window resize) the rendering will be correct.
Furthermore,
cv::imshow("GPU", cv::ogl::Texture2D(d_frame));should be avoided since it causes memory leak.Relates to
#22336
#4644
#22328
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.