Skip to content

Add strict validation for encoding parameters #27621

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
Merged
merged 10 commits into from
Aug 12, 2025
Merged

Add strict validation for encoding parameters #27621

merged 10 commits into from
Aug 12, 2025

Conversation

Kumataro
Copy link
Contributor

@Kumataro Kumataro commented Aug 2, 2025

Close #27557

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

@Kumataro
Copy link
Contributor Author

Kumataro commented Aug 2, 2025 •
edited
Loading

videoio_images.* tests are failed. This is potential issue.

Before OpenCV3 supports cvSaveImage() C-API function.
And after OpenCV 3.2.0, image_params is used with dummy key and value (0,0).

opencv/modules/videoio/src/cap_images.cpp

Lines 377 to 384 in af32659

CV_Assert(!filename_pattern.empty());
cv::String filename = cv::format(filename_pattern.c_str(), (int)currentframe);
CV_Assert(!filename.empty());
std::vector<int> image_params = params;
image_params.push_back(0); // append parameters 'stop' mark
image_params.push_back(0);
int ret = cvSaveImage(filename.c_str(), image, &image_params[0]);

OpenCV4 drop this C-API, and it is replaced to imwrite() C++ API.

opencv/modules/videoio/src/cap_images.cpp

Lines 388 to 397 in 52bed3c

CV_Assert(!filename_pattern.empty());
cv::String filename = cv::format(filename_pattern.c_str(), (int)currentframe);
CV_Assert(!filename.empty());
std::vector<int> image_params = params;
image_params.push_back(0); // append parameters 'stop' mark
image_params.push_back(0);
cv::Mat img = image.getMat();
cv::imwrite(filename, img, image_params);

We do not have to use params for C++ API. It is better.

Kumataro
Kumataro commented Aug 3, 2025
View reviewed changes
Kumataro
Kumataro commented Aug 3, 2025
View reviewed changes
@asmorkalov asmorkalov self-requested a review August 4, 2025 05:37
@asmorkalov asmorkalov mentioned this pull request Aug 8, 2025
Closed
6 tasks
@asmorkalov asmorkalov added this to the 4.13.0 milestone Aug 8, 2025
@asmorkalov
Copy link
Contributor

@Kumataro Thanks a lot for the contribution. We discussed the PR on the core team meeting. Summary:

  • Macro guard is not needed.
  • Let's print warning with CV_LOG_WARNING for the cases, when parameter value is out of range or the parameter is unknown or not supported for the particular codec.
  • Let's saturate values that are out of range.
  • Let's continue file writing and return true, even, if some parameters are not correct. We do not want to break existing code, even if it's not accurate.

@Kumataro
Copy link
Contributor Author

Kumataro commented Aug 8, 2025

Thank you very much for your discussion about this pull request !!

I feel it will be hard to get finished right away.
I would appreciate it if you could wait a little while.

@Kumataro Kumataro marked this pull request as draft August 10, 2025 10:05
@asmorkalov asmorkalov self-assigned this Aug 11, 2025
@Kumataro Kumataro marked this pull request as ready for review August 11, 2025 10:14
@asmorkalov
Copy link
Contributor

Thanks a lot for the great job!

@asmorkalov asmorkalov merged commit c5bb6a0 into opencv:4.x Aug 12, 2025
27 of 28 checks passed
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: imgcodecs feature
Projects
None yet
Milestone
4.13.0
Development

Successfully merging this pull request may close these issues.

Proposal: Add strict validation for encoding parameters in OpenCV 5
2 participants
@Kumataro @asmorkalov