Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cudacodec: Fix build warnings #3517

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from
Open

cudacodec: Fix build warnings #3517

wants to merge 1 commit into from

Conversation

cudawarped
Copy link
Contributor

@cudawarped cudawarped commented Jun 30, 2023 •
edited
Loading

cudacodec produces a number of build warnings, mainly introduced by myself:facepalm:.

PR removes this pollution from the build output.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@asmorkalov asmorkalov self-requested a review July 4, 2023 12:38
@asmorkalov asmorkalov self-assigned this Jul 4, 2023
asmorkalov
asmorkalov reviewed Jul 4, 2023
View reviewed changes
modules/cudacodec/src/NvEncoder.cpp Show resolved Hide resolved
modules/cudacodec/src/NvEncoder.h Outdated Show resolved Hide resolved
modules/cudacodec/test/test_video.cpp Outdated Show resolved Hide resolved
modules/cudacodec/test/test_video.cpp Outdated Show resolved Hide resolved
modules/cudacodec/src/video_source.hpp Show resolved Hide resolved
modules/cudacodec/src/video_reader.cpp Outdated
return true;
case VideoReaderProps::PROP_RAW_MODE:
propertyVal = videoSource_->RawModeEnabled();
return true;
case VideoReaderProps::PROP_LRF_HAS_KEY_FRAME: {
const int iPacket = propertyVal - rawPacketsBaseIdx;
if (videoSource_->RawModeEnabled() && iPacket >= 0 && iPacket < rawPackets.size()) {
const int iPacket = static_cast<int>(propertyVal) - rawPacketsBaseIdx;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propertyVal is function output and could contain any value here. Looks like it's a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its output and input, not sure what I was thinking here will alter this so that it makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document "input" logic then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmorkalov I have added a new method rawPackageHasKeyFrame() to avoid the input output argument confusion in the get() method but now I am wondering if it is the right time to make some more alterations. Specifically those below which I "wrongly" introduced to mirror the VideoCapture class:

  • remove grab()/retrieve() for the decoded frame. This combination is completely unecessary grab() doesn't save any processing, it only remove the need to pass a GpuMat when you don't want to retrieve the decoded frame.
  • propertyVal of double in get(const VideoReaderProps propertyId, CV_OUT double& propertyVal) results in a lot of needless casting when the value will always be an integer.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmorkalov I have pushed changes to demonstrate the removal of grab() and making propertyVal a size_t what do you think?

@asmorkalov
Copy link
Contributor

Tested manually with Ubuntu 18.04, CUDA 10.2 and GF 1080.

@cudawarped cudawarped force-pushed the cudacodec_warnings branch 4 times, most recently from 00ef267 to 2526e88 Compare July 7, 2023 11:17
asmorkalov
asmorkalov reviewed Jul 7, 2023
View reviewed changes
modules/cudacodec/include/opencv2/cudacodec.hpp Outdated
Comment on lines 383 to 394
/** @brief Grabs the next frame from the video source.

@param stream Stream for the asynchronous version.
@return `true` (non-zero) in the case of success.

The method/function grabs the next frame from video file or camera and returns true (non-zero) in
the case of success.

The primary use of the function is for reading both the encoded and decoded video data when rawMode is enabled. With rawMode enabled
retrieve() can be called following grab() to retrieve all the data associated with the current video source since the last call to grab() or the creation of the VideoReader.
*/
CV_WRAP virtual bool grab(Stream& stream = Stream::Null()) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you removed grab? It breaks compatibility and it's still mentioned in other methods.

Copy link
Contributor Author

@cudawarped cudawarped Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I am wondering if it is the right time to make some more alterations. Specifically those below which I "wrongly" introduced to mirror the VideoCapture class:

  • remove grab()/retrieve() for the decoded frame. This combination is completely unecessary grab() doesn't save any processing, it only remove the need to pass a GpuMat when you don't want to retrieve the decoded frame.
  • propertyVal of double in get(const VideoReaderProps propertyId, CV_OUT double& propertyVal) results in a lot of needless casting when the value will always be an integer.

What do you think?

@asmorkalov I have pushed changes to demonstrate the removal of grab() and making propertyVal a size_t what do you think?

I removed grab() in that commit so you could take a look. It was an unecessary addition which adds a lot of code mess to VideoReader and was "wrongly" introduced to mirror VideoCapture without actually mirroring the funcitonality. I can back it out and place it in another PR if you think agree with its removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmorkalov Added grab() back into to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmorkalov Should this be included in 4.9.0?

@cudawarped cudawarped force-pushed the cudacodec_warnings branch 2 times, most recently from 720e0b8 to a10e30b Compare August 2, 2023 05:35
@cudawarped cudawarped mentioned this pull request Oct 26, 2023
Merged
6 tasks
@cudawarped cudawarped force-pushed the cudacodec_warnings branch 2 times, most recently from d7c3402 to 50c79b1 Compare December 4, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmorkalov asmorkalov Awaiting requested review from asmorkalov

Assignees

@asmorkalov asmorkalov

Labels
category: build/install category: cuda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cudawarped @asmorkalov @opencv-alalek