Skip to content

imgcodecs: ensure parameters are key-value pairs, fix HDR encoder#22830

Merged
asmorkalov merged 1 commit intoopencv:3.4from
alalek:issue_22752
Nov 22, 2022
Merged

imgcodecs: ensure parameters are key-value pairs, fix HDR encoder#22830
asmorkalov merged 1 commit intoopencv:3.4from
alalek:issue_22752

Conversation

@alalek
Copy link
Copy Markdown
Member

@alalek alalek commented Nov 18, 2022

resolves #22752

{
switch (params[i])
{
case CV_IMWRITE_JPEG_QUALITY:
Copy link
Copy Markdown
Contributor

@Kumataro Kumataro Nov 18, 2022

Choose a reason for hiding this comment

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

I feel that it is need to fix the conditional here.

-        case CV_IMWRITE_JPEG_QUALITY:
+        case IMWRITE_HDR_COMPRESSION:

Test code is following.

$ git --no-pager diff
diff --git a/modules/imgcodecs/src/grfmt_hdr.cpp b/modules/imgcodecs/src/grfmt_hdr.cpp
index 4c37549..6e2f565 100644
--- a/modules/imgcodecs/src/grfmt_hdr.cpp
+++ b/modules/imgcodecs/src/grfmt_hdr.cpp
@@ -147,7 +147,7 @@ bool HdrEncoder::write( const Mat& input_img, const std::vector<int>& params )
     {
         switch (params[i])
         {
-        case CV_IMWRITE_JPEG_QUALITY:
+        case IMWRITE_HDR_COMPRESSION:
             compression = params[i + 1];
             break;
         default:
diff --git a/modules/imgcodecs/test/test_grfmt.cpp b/modules/imgcodecs/test/test_grfmt.cpp
index 520ba4a..2e4130c 100644
--- a/modules/imgcodecs/test/test_grfmt.cpp
+++ b/modules/imgcodecs/test/test_grfmt.cpp
@@ -316,6 +316,27 @@ TEST(Imgcodecs_Hdr, regression)
         ASSERT_FALSE(max > DBL_EPSILON);
     }
     remove(tmp_file_name.c_str());
+
+    {
+        // Default
+        vector<uchar> buf_noparams;
+        imencode(".hdr", img_no_rle, buf_noparams);
+
+        // IMWRITE_HDR_COMPRESSION_NONE
+        param[0] = IMWRITE_HDR_COMPRESSION;
+        param[1] = IMWRITE_HDR_COMPRESSION_NONE;
+        vector<uchar> buf_comp_none;
+        imencode(".hdr", img_no_rle, buf_comp_none, param);
+
+        // IMWRITE_HDR_COMPRESSION_RLE
+        param[0] = IMWRITE_HDR_COMPRESSION;
+        param[1] = IMWRITE_HDR_COMPRESSION_RLE;
+        vector<uchar> buf_comp_rle;
+        imencode(".hdr", img_no_rle, buf_comp_rle, param);
+
+        ASSERT_EQ( buf_noparams, buf_comp_rle);
+        ASSERT_TRUE( buf_comp_rle.size() < buf_comp_none.size());
+    }
 }
 #endif

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@Kumataro Thank you! Fixed.
Also fixed imencode() and added tests.

BTW, C API is on hold. We don't modify it anymore.

Copy link
Copy Markdown
Contributor

@Kumataro Kumataro Nov 20, 2022

Choose a reason for hiding this comment

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

Thank you to fix it , and I agree with you.

CV_IMWRITE_HDR_COMPRESSION is not necessary and will not be defined.

@Kumataro
Copy link
Copy Markdown
Contributor

Thank you very much to fix it !!

(Perhaps I'm overthinking.)
If OpenCV 3.4 is included target to fix, is it better to add enum for C-API ?

enum
{
CV_IMWRITE_JPEG_QUALITY =1,
CV_IMWRITE_JPEG_PROGRESSIVE =2,
CV_IMWRITE_JPEG_OPTIMIZE =3,
CV_IMWRITE_JPEG_RST_INTERVAL =4,
CV_IMWRITE_JPEG_LUMA_QUALITY =5,
CV_IMWRITE_JPEG_CHROMA_QUALITY =6,
CV_IMWRITE_PNG_COMPRESSION =16,
CV_IMWRITE_PNG_STRATEGY =17,
CV_IMWRITE_PNG_BILEVEL =18,
CV_IMWRITE_PNG_STRATEGY_DEFAULT =0,
CV_IMWRITE_PNG_STRATEGY_FILTERED =1,
CV_IMWRITE_PNG_STRATEGY_HUFFMAN_ONLY =2,
CV_IMWRITE_PNG_STRATEGY_RLE =3,
CV_IMWRITE_PNG_STRATEGY_FIXED =4,
CV_IMWRITE_PXM_BINARY =32,
CV_IMWRITE_EXR_TYPE = 48,
CV_IMWRITE_WEBP_QUALITY =64,
CV_IMWRITE_PAM_TUPLETYPE = 128,
CV_IMWRITE_PAM_FORMAT_NULL = 0,
CV_IMWRITE_PAM_FORMAT_BLACKANDWHITE = 1,
CV_IMWRITE_PAM_FORMAT_GRAYSCALE = 2,
CV_IMWRITE_PAM_FORMAT_GRAYSCALE_ALPHA = 3,
CV_IMWRITE_PAM_FORMAT_RGB = 4,
CV_IMWRITE_PAM_FORMAT_RGB_ALPHA = 5,
};

    CV_IMWRITE_WEBP_QUALITY =64,
+   CV_IMWRITE_HDR_COMPRESSION =80,
    CV_IMWRITE_PAM_TUPLETYPE = 128,

@dan-masek
Copy link
Copy Markdown
Contributor

@Kumataro I don't think so. IIRC the C API was deprecated in 3.x, and now is history. Doesn't seem to make much sense to touch that.

Copy link
Copy Markdown
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.

👍 Looks good to me.

@asmorkalov
Copy link
Copy Markdown
Contributor

@Kumataro May I merge the PR?

@Kumataro
Copy link
Copy Markdown
Contributor

Yes, I think there are no problem,, thank you very much to fix !!

@asmorkalov asmorkalov merged commit 5db4f1f into opencv:3.4 Nov 22, 2022
@alalek alalek mentioned this pull request Dec 3, 2022
@alalek alalek mentioned this pull request Jan 8, 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.

imwrite() with params which has odd elements may make out-of-boundary memory access .

4 participants