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.

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

cudacodec::VideoWriter add flag for writing full range luma chroma video #3868

Merged

Conversation

cudawarped
Copy link
Contributor

@cudawarped cudawarped commented Jan 9, 2025

Currently the meta data in all encoded video's is not updated to reflect the range (full of limited as defined in Annex E of the ITU-T Specification) of encoded luma/chroma values. With all video's indicating that they use the limited range. This means that when a full range YUV source is encoded the decoder will incorrectly decode it to a limited range resulting in poor color reproduction.

This PR correctly sets this meta data field for all encoded video's.

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 force-pushed the cudacodec_videowriter_add_full_range_flag branch from dacc035 to c8ff1a7 Compare January 15, 2025 11:25
@cudawarped
Copy link
Contributor Author

@asmorkalov I've added a warning when the codec is not h264 or h265.

Note: Because I can't test AV1 encoding (I don't have access to a card with 8th Gen NVDEC) the cudacodec module does not currently support AV1 encoding. This is why I haven't also added AV1 support for the videoFullRangeFlag (NV_ENC_CONFIG::encodeConfig->encodeCodecConfig.av1Config.colorRange) flag in this PR.

@asmorkalov
Copy link
Contributor

You actually can submit the PR with GF 4x support. I have a device for test.

@cudawarped
Copy link
Contributor Author

That's good to know although I'll need access to one first to test everything locally first before submitting a PR.

@asmorkalov asmorkalov self-assigned this Jan 15, 2025
… the yuv frames passed to VideoWriter occupy the full or limited range as defined by the Annex E of the ITU-T Specification.
@asmorkalov
Copy link
Contributor

@cudawarped There is conflict with the previous PR please rebase. Also videoFullRangeFlag looks ugly for me. It's bool and it's obviously flag. I propose to rename it to something like encodeVideoFullRange.

@cudawarped
Copy link
Contributor Author

@asmorkalov Correct, I copied the name from the SDK where its a flag (unsigned char video_full_range_flag in NVDEC and uint32_t videoFullRangeFlag in NVENC). Additionaly as I mentioned above this is refered to as uint32_t colorRange for AV1 encoding so a differently named bool here would make sense. I tried to keep the naming the same as the SDK for transparency but I can change this if its less confusing?

For consistency should I also rename it in VideoReader to videoFullRangeSource or similar?

@asmorkalov
Copy link
Contributor

videoFullRangeSource looks good.

@asmorkalov
Copy link
Contributor

Oh, got the issue. Let it be videoFullRangeFlag everywhere to not break code and confuse users.

@cudawarped cudawarped force-pushed the cudacodec_videowriter_add_full_range_flag branch from c8ff1a7 to b4a91cd Compare January 15, 2025 13:41
@asmorkalov asmorkalov merged commit 2af5458 into opencv:4.x Jan 16, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants