Proposed Changes

I'm trying to fix issue #27557 where invalid parameters like {-1, -1} are silently accepted by imencode and imwrite functions.

What I added:

1. s_formatParamRanges - A lookup table that has all the valid parameters for different image formats (jpg, png, webp, etc.) and their allowed value ranges.

2. getCleanExtension() - A helper function to clean up file extensions (removes dots, makes lowercase) so we can look them up properly.

3. validateEncodingParams() - The main function that checks:

   • Parameters come in pairs (ID, value)
   • Not too many parameters
   • Parameter IDs are not negative
   • Parameters are supported for the format
   • Values are in the right range

How it works:

• For unknown formats: Just checks that parameter IDs aren't negative
• For formats like BMP/GIF (no special params): Allows anything
• For formats with params (JPEG, PNG, etc.): Does full validation and shows clear errors
• For unsupported params: Shows warnings instead of errors

Before vs After:

Before (Silent acceptance):

.jpg imencode result: 1
.jpg imwrite result: 1

After (Clear error reporting):

terminate called after throwing an instance of 'cv::Exception'
  what():  OpenCV(4.13.0-dev) .../loadsave.cpp:203: error: (-5:Bad argument) 
  Invalid parameter ID: -1. Parameter IDs must be non-negative. 
  in function 'validateEncodingParams'

Test Code:

#include <iostream>
#include <vector>
#include <opencv2/opencv.hpp>
using namespace cv;

int main(int argc, const char** argv)
{
    std::vector<std::string> exts = {
        ".bmp", ".gif", ".ppm", ".ras", ".png", 
        ".tiff", ".jpg", ".webp", ".jp2"
    };
    
    Mat image = Mat(100, 100, CV_8UC3, Scalar(0, 255, 0));
    
    for (size_t i = 0; i < exts.size(); i++)
    {
        std::vector<uchar> buf;
        bool enc_result = imencode(exts[i], image, buf, { -1, -1 });
        bool write_result = imwrite("test" + exts[i], image, { -1, -1 });
        
        std::cout << exts[i] << " imencode result: " << enc_result << std::endl;
        std::cout << exts[i] << " imwrite result: " << write_result << std::endl;
    }
    return 0;
}

Testing:

I tested it with the code from the original issue and now instead of silently succeeding, it properly throws an error.

Fixes #27557

Please suggest if it needs improvement or any changes in the logic.

Hello, thank you for your pull request. This comment is by one of contributors, not maintainers.

I believe it is better to validate encode param (the pairs of key and value) on each Encoders implementation.
It will be easier to expand if loadsave.c and each Encoder are kept sparse.

My idea is Kumataro@52cad13

• Encoders has isValidaParam(const int key and const int value) function to validate encode params.
• loadsave.c calls it on current encoder. If it returns true, it is valid parameter.
• Otherwise, loadsave.c calls it on other encoders. If one of them return true, it is valid parameter.
• If no encoders return true, it is invalid parameter. imwrite() and its family functions will be return false without encoding.

However, this mechanism doesn't accept unused invalid parameters. The validation may be a bit excessive.

@Kumataro Thanks for the feedback! I'm still getting to know the workings of how to structure things properly in bigger projects, so this is really helpful.

You make a good point about the distributed validation. I went with the centralized thing because it seemed straightforward to implement, but yeah, having to update that lookup table every time someone adds a format isn't great. Plus I didn't really think about the string processing cost as it was not much noticeable when dealing with single images, but it could add up when handling a large batch.

Your approach makes more sense - let each encoder worry about its own stuff. Much cleaner that way.

I saw you mentioned Kumataro/opencv@52cad13 - did you already code this up? If you're planning to make a PR and the maintainers like it better, no worries on my end. I can close this one.

I guess I should wait to see what the maintainers think? My main thing was just stopping those invalid parameters from getting silently ignored, so whatever works best for the project is fine by me.

Thanks for explaining all this - definitely helps me think about these design trade-offs better.

Thanks for your reply!

With my method, there's an issue where the same code may or may not throw an error depending on the user's build configuration.

For example, if JPEG and WebP encoding parameters are specified simultaneously in a single vector, an error will occur as the parameter specification is invalid only if WebP is disabled.

I believe this is an extremely rare case. However, there is a slight risk of it causing problems later.

To prevent this, centralized management (loadsave.c) is preferable to decentralized management (individual encoders).

I'll await the maintainer's comments on whether decentralized management is acceptable.

Closed in favor of #27621