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::VideoReader add reconfigure decoder functionality #3453

Merged

Conversation

cudawarped
Copy link
Contributor

@cudawarped cudawarped commented Mar 3, 2023

Add decoder reconfiguration functionality on resolution change, see Nvidia Docs.

PR test is dependent on multi-resolution test video from opencv/opencv_extra#1047

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

Copy link
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍 Well done! Tested with Ubuntu 18.04 and CUDA 10.2.

@asmorkalov
Copy link
Contributor

@cudawarped I made experiments with cv::VideoCapture and ported your test. VideoCapture class behaves differently and changes frame resolution on the go. What if make the behavior optional?

@cudawarped
Copy link
Contributor Author

cudawarped commented Mar 20, 2023

@asmorkalov I just ran a quick test and VideoCapure maintains the same frame size and places the new smaller frame in a ROI starting at the top left corner, shown below.

videocapture_reconfig

As you can see VideoCapture copies the new smaller frame to this smaller ROI leaving the rest of the outpur frame the same as it was. To me this looks buggy.

Personally I like the resize option for the reasons mentioned in the Nvidia docs,

The API can be used in scenarios where the bitstream undergoes changes in resolution, for e.g. when the encoder (on server side) changes image resolution frequently to adhere to Quality of Service(QoS) constraints.

but I understand the importants of having the same behaviour accross OpenCV's video reading interfaces.

Therefore I can add default the behaviour to do the same thing as VideoCapture but first zeroing out the image (so it looks less buggy) or I could place the image in the centre and letterbox it, which would you prefer?

I am happy either way, the main reason for this PR was to avoid VideoReader hanging when there is a new call to cv::cudacodec::detail::VideoParser::HandleVideoSequence which is the case currently because the decoder is destroyed while it is being used in another thread.

@asmorkalov
Copy link
Contributor

Let's do the following then:

  • You'll add option to CUDA branch to make the behavior controlled.
  • I'll take a look on VideoCapture back-ends. We need to ensure consistent behaviour across platforms.
  • I'll put the issue to our core meeting agenda to discuss the cases with other team members and define fault behaviour.

@cudawarped cudawarped force-pushed the cudacodec_add_reconfigure_decoder branch from 2c2e39e to f0fa5bb Compare March 21, 2023 11:45
@cudawarped
Copy link
Contributor Author

  • You'll add option to CUDA branch to make the behavior controlled.

Added a ResolutionChangeMode enum with default behaviour to produce the same as output as cv::VideoCapture + reduced the number of frames in the test file to 40, 20 at the higher and 20 at the lower resolution.

@cudawarped cudawarped force-pushed the cudacodec_add_reconfigure_decoder branch from f0fa5bb to ea1a5be Compare April 17, 2023 09:54
Comment on lines 197 to 199
if (!decodeResChange)
return 1;

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be before block with autolock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look at this next week, I need to find the test footage I was using where the number of decode surfaces changed but the resolution didn't to check if this can be moved.

@@ -305,16 +305,27 @@ enum DeinterlaceMode
Adaptive = 2
};

/** @brief Output format for a decoded frame when the resolution of the source is reduced by the encoder. In all cases the size of the output frame remains the same.
* @param Default Use the approach adopted by cv::VideoCapture, i.e. maintain the same frame size by placing the smaller output in the top left corner.
Copy link
Contributor

Choose a reason for hiding this comment

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

I made experiment with your test stream and VideoCapture follows frame resolution according to video:

Frame size: 700x400
Resolution: 700x400
Frame size: 700x400
Resolution: 700x400
Frame size: 700x400
[swscaler @ 0x55a1797812e0] Slice parameters 0, 400 are invalid
Resolution: 672x384
Frame size: 672x384
[swscaler @ 0x55a1797812e0] Slice parameters 0, 400 are invalid
Resolution: 672x384
Frame size: 672x384
Resolution: 672x384
Frame size: 672x384
Resolution: 672x384
Frame size: 672x384
Resolution: 672x384

Frame size: cols & rows for Mat. Resolution: CAP_PROP_FRAME_WIDTH and CAP_PROP_FRAME_HEIGHT. IMHO, it should be default behavior for consistency.

Copy link
Contributor Author

@cudawarped cudawarped Jun 8, 2023

Choose a reason for hiding this comment

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

This appears to depend on the version of FFMpeg. If I use the prebuilt windows plugin

-- FFMPEG: YES (prebuilt binaries)
-- avcodec: YES (58.134.100)
-- avformat: YES (58.76.100)
-- avutil: YES (56.70.100)
-- swscale: YES (5.9.100)
-- avresample: YES (4.0.0)

then I get the output you described, shown below

avcodec: 58.134.100
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[swscaler @ 00000220c64f4140] Slice parameters 0, 400 are invalid
[672 x 384]
[swscaler @ 00000220c64f4140] Slice parameters 0, 400 are invalid
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]
[672 x 384]

however when I link to a more recent version of FFmpeg, e.g.

-- FFMPEG: YES (find_package)
-- avcodec: YES (59.18.100)
-- avformat: YES (59.16.100)
-- avutil: YES (57.17.100)
-- swscale: YES (6.4.100)

I get the previous output I showed in the screen shot with consistent sizes and no

[swscaler @ 00000220c64f4140] Slice parameters 0, 400 are invalid

error message.

avcodec: 59.18.100
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]
[700 x 400]

IMHO, it should be default behavior for consistency

Unfortunately the output from the Nvidia decoder will always be the same resolution as the original/first decoded frame (700x400) because the reconfigure functionality is designed for QOS use where you would want to maintain the same output resolution.

What would you like me to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to rename enum items and remove reference to cv::VideoCature as soon as it behaves slightly different depending on backend.

  • "default" -> "copy"
  • "QoS" -> "scale" or "upscale"
    It makes behaviour more obvious even without documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, just a thought would it be cleaner to remove ResolutionChangeMode completely and just default to QOS as the copy behaviour may not be a sensible default anymore (FFmpeg could change again so why copy them)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, works for me then.

@cudawarped cudawarped force-pushed the cudacodec_add_reconfigure_decoder branch from ea1a5be to 045a9b0 Compare June 9, 2023 06:19
Copy link
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍 Thanks a lot for the contribution!

@asmorkalov
Copy link
Contributor

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

@asmorkalov asmorkalov merged commit 353e37e into opencv:4.x Jun 9, 2023
7 checks passed
@asmorkalov asmorkalov mentioned this pull request Jul 12, 2023
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.

None yet

2 participants