Skip to content

cudacodec::VideoReader: fix nv12 to bgr/bgra/grey conversion#3468

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
cudawarped:cudacodec_fix_colour_conversion
Apr 17, 2023
Merged

cudacodec::VideoReader: fix nv12 to bgr/bgra/grey conversion#3468
asmorkalov merged 1 commit intoopencv:4.xfrom
cudawarped:cudacodec_fix_colour_conversion

Conversation

@cudawarped
Copy link
Copy Markdown
Contributor

@cudawarped cudawarped commented Apr 5, 2023 •
edited
Loading

Fixes #3458.

Dependent on opencv/opencv_extra#1051

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

@cudawarped cudawarped changed the title cudacodec: fix nv12 to bgr/bgra/grey conversion cudacodec::VideoReader: fix nv12 to bgr/bgra/grey conversion Apr 5, 2023
@cudawarped cudawarped mentioned this pull request Apr 5, 2023
Merged
@cudawarped cudawarped force-pushed the cudacodec_fix_colour_conversion branch from 2ea6f4a to 2b476dd Compare April 5, 2023 17:35
@asmorkalov asmorkalov self-requested a review April 6, 2023 19:18
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 13, 2023

I see build issue with my CUDA 10.2 on Ubuntu:

      ^~~~~~~~~~~
In file included from /home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/precomp.hpp:57:0,
                 from /home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/video_reader.cpp:43:
/home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/video_reader.cpp:76:25: error: ‘nppiNV12ToBGR_709CSC_8u_P2C3R_Ctx’ was not declared in this scope
             nppSafeCall(nppiNV12ToBGR_709CSC_8u_P2C3R_Ctx(pSrc, decodedFrame.step, outFrame.data, outFrame.step, oSizeROI, nppStreamCtx));
                         ^
/home/alexander/Projects/OpenCV/opencv-master/modules/core/include/opencv2/core/private.cuda.hpp:162:52: note: in definition of macro ‘nppSafeCall’
 #define nppSafeCall(expr)  cv::cuda::checkNppError(expr, __FILE__, __LINE__, CV_Func)
                                                    ^~~~
/home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/video_reader.cpp:76:25: note: suggested alternative: ‘nppiNV12ToBGR_709HDTV_8u_P2C3R_Ctx’
             nppSafeCall(nppiNV12ToBGR_709CSC_8u_P2C3R_Ctx(pSrc, decodedFrame.step, outFrame.data, outFrame.step, oSizeROI, nppStreamCtx));

asmorkalov
asmorkalov reviewed Apr 13, 2023
View reviewed changes
modules/cudacodec/include/opencv2/cudacodec.hpp Outdated
CV_PROP_RW cv::Size targetSz;//!< Post-processed size of the output frame.
CV_PROP_RW cv::Rect srcRoi;//!< Region of interest decoded from video source.
CV_PROP_RW cv::Rect targetRoi;//!< Region of interest in the output frame containing the decoded frame.
CV_PROP_RW bool videoFullRangeFlag;//!< indicates if the black level, luma and chroma are using the full range 0-255.
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Apr 13, 2023

Choose a reason for hiding this comment

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

The flag corresponds to BGR 709 HDTV (true) / RGB 709 CSC (false) conversion. I propose to use it naming or at least to define it in documentation string. Also, it makes sense to mention, that the flag is set automatically from stream parser.

Copy link
Copy Markdown
Contributor Author

@cudawarped cudawarped Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The flag corresponds to BGR 709 HDTV (true) / RGB 709 CSC (false) conversion. I propose to use it naming or at least to define it in documentation string.

The flag indicates whether the luma and chroma samples are represented using the full range of 8 bit values (0-255) or a smaller range. From my understanding the limited range is the most common for video. This is applicable to BGRA, BGR and Gray conversion not just BGRA.

The name is to be consistent with the definition in Annex E of the ITU-T Specification (video_full_range_flag) and the name given internally in the Nvidia Video Codec SDK (videoFullRangeFlag). I can add these details to the documentation string but I think the name is valid?

Also, it makes sense to mention, that the flag is set automatically from stream parser.

Agreed.

Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Apr 13, 2023

Choose a reason for hiding this comment

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

Ok, let's presume naming, but add reference to BGR 709 HDTV / RGB 709 color spaces.

@asmorkalov asmorkalov self-assigned this Apr 13, 2023
@cudawarped
Copy link
Copy Markdown
Contributor Author

cudawarped commented Apr 13, 2023

I see build issue with my CUDA 10.2 on Ubuntu:

      ^~~~~~~~~~~
In file included from /home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/precomp.hpp:57:0,
                 from /home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/video_reader.cpp:43:
/home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/video_reader.cpp:76:25: error: ‘nppiNV12ToBGR_709CSC_8u_P2C3R_Ctx’ was not declared in this scope
             nppSafeCall(nppiNV12ToBGR_709CSC_8u_P2C3R_Ctx(pSrc, decodedFrame.step, outFrame.data, outFrame.step, oSizeROI, nppStreamCtx));
                         ^
/home/alexander/Projects/OpenCV/opencv-master/modules/core/include/opencv2/core/private.cuda.hpp:162:52: note: in definition of macro ‘nppSafeCall’
 #define nppSafeCall(expr)  cv::cuda::checkNppError(expr, __FILE__, __LINE__, CV_Func)
                                                    ^~~~
/home/alexander/Projects/OpenCV/opencv_contrib/modules/cudacodec/src/video_reader.cpp:76:25: note: suggested alternative: ‘nppiNV12ToBGR_709HDTV_8u_P2C3R_Ctx’
             nppSafeCall(nppiNV12ToBGR_709CSC_8u_P2C3R_Ctx(pSrc, decodedFrame.step, outFrame.data, outFrame.step, oSizeROI, nppStreamCtx));

Apologies, it looks like it wasn't introduced until CUDA 11.0, I will add build guards for this. In my defence Nvidia's documentation doesn't mention this anywhere in the release notes, you have to manually go through the NPP documentation to see that it wasn't in CUDA 10.2 and was in CUDA 11.0, come on Nvidia!! I promise I will be more rigorous next time.

@cudawarped cudawarped force-pushed the cudacodec_fix_colour_conversion branch from 2b476dd to d996792 Compare April 13, 2023 11:48
@cudawarped
Copy link
Copy Markdown
Contributor Author

cudawarped commented Apr 13, 2023

Updated and tested on Windows 11 built with VS2019 against CUDA toolkit 10.2 and the latest version of Nvidia Video Codec SDK (12.0). All cudacodec tests passing on RTX 3070 Ti.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 13, 2023

Works for me with CUDA 10.2 and Ubuntu 18.04.

asmorkalov
asmorkalov reviewed Apr 13, 2023
View reviewed changes
modules/cudacodec/src/video_reader.cpp
nppSafeCall(nppiNV12ToBGR_709HDTV_8u_P2C3R_Ctx(pSrc, decodedFrame.step, outFrame.data, outFrame.step, oSizeROI, nppStreamCtx));
else {
#if (CUDART_VERSION < 11000)
CV_LOG_DEBUG(NULL, "Color reproduction may be inaccurate due CUDA version <= 11.0, for better results upgrade CUDA runtime or try ColorFormat::BGRA.");
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Apr 13, 2023

Choose a reason for hiding this comment

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

I propose to make the check in constructor, and use LOG_INFO to not generate spam. M.b. could you also add own conversion kernel for BGR in the same way as BGRA?

Copy link
Copy Markdown
Contributor Author

@cudawarped cudawarped Apr 13, 2023

Choose a reason for hiding this comment

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

I propose to make the check in constructor, and use LOG_INFO to not generate spam.

Unfortunately that information isn't available until a frame has been run through the decoder. This is something I've wanted to change for a while and was going to add in another PR. Should I add that change to this PR or simply remove the debug output which is not really necessary?

M.b. could you also add own conversion kernel for BGR in the same way as BGRA?

Actually I would prefer to remove the BGRA kernel. Much as I am not a fan of the NPP libs (buggy, performance oscillates depending on the CUDA version etc.), they should be the best option for performance on future hardware + they should get more user testing than the OpenCV CUDA modules.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 13, 2023

Let's stay it as is then. Let's merge things as is for now and tune the behavior later.

@asmorkalov asmorkalov merged commit bd546fa into opencv:4.x Apr 17, 2023
@cudawarped cudawarped mentioned this pull request Apr 28, 2023
Merged
6 tasks
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
Merged
@cudawarped cudawarped mentioned this pull request Nov 22, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: cuda

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The frame extraction results of the GPU and CPU are different.

2 participants

@cudawarped @asmorkalov