Adding possibility to select the compression type for the OpenEXR format by hallonterror · Pull Request #19295 · opencv/opencv

hallonterror · 2021-01-08T16:09:54Z

There are compression modes other than the default that are more suited for certain data. Mainly grainy/noisy data.

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 other license that is incompatible with OpenCV
The PR is proposed to proper branch
[-] There is reference to 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

…mat. There are compression modes other than the default that are more suited for certain data. Mainly grainy/noisy data.

asmorkalov · 2021-01-11T12:34:16Z

@hallonterror Thanks for the useful option. Couple of proposals for the solution:

OpenCV team stores test data in extra repository: https://github.com/opencv/opencv_extra/. You can add your test image there, or, better re-use one of the images, if it's applicable. So you do not need #ifndef in test code. If you decided to add new image to opencv_extra please submit PR there and add opencv_extra=<your branch_name> to the PR rescription body to apply it on BuildBot automatically.
The test just checks that the image is loaded correctly, but not option itself. As soon as compression option introduces significant size change I propose to add size check as simple heuristic. Please do not forget to add comment there to cover the heuristic purpose.

alalek · 2021-01-09T03:43:15Z

modules/imgcodecs/include/opencv2/imgcodecs/legacy/constants_c.h

    CV_IMWRITE_PNG_STRATEGY_FIXED =4,
    CV_IMWRITE_PXM_BINARY =32,
    CV_IMWRITE_EXR_TYPE = 48,
+    CV_IMWRITE_EXR_COMPRESSION = 49,


Legacy C files should not be touched anymore.
Please remove this change.

alalek · 2021-01-09T03:43:50Z

modules/imgcodecs/src/grfmt_exr.cpp

+            header.compression() = DWAB_COMPRESSION;
+            break;
+          default:
+            throw std::runtime_error("IMWRITE_EXR_COMPRESSION is invalid or not supported");


CV_Error(Error::StsBadArg, ...)

Should this be done although the CV_IMWRITE_EXR_TYPE throws? See line 592.

Reverting change to legacy/constants_c.h.

hallonterror · 2021-01-13T16:41:24Z

@hallonterror Thanks for the useful option. Couple of proposals for the solution:

OpenCV team stores test data in extra repository: https://github.com/opencv/opencv_extra/. You can add your test image there, or, better re-use one of the images, if it's applicable. So you do not need #ifndef in test code. If you decided to add new image to opencv_extra please submit PR there and add opencv_extra=<your branch_name> to the PR rescription body to apply it on BuildBot automatically.

The test just checks that the image is loaded correctly, but not option itself. As soon as compression option introduces significant size change I propose to add size check as simple heuristic. Please do not forget to add comment there to cover the heuristic purpose.

I know of the test data in opencv_extra. I think it should be sufficient to use one of the already existing image files (like it is in the added unit test (modules/imgcodecs/test/test_exr.impl.hpp), even though it wont show a compression advantage on those files.

For the second point you are absolutely right. I did verify that the parameter had effect exactly this way, by checking that the file size was not the same as the previously existing unit test without the flag set. It's of course reasonable to add this as a check in the test suite. I will look into that!

Adding size check that ensures that the selection of PIZ compression has an effect on file size. For this particular file it gets larger. For other suitable image content the file should be smaller.

modules/imgcodecs/test/test_exr.impl.hpp

asmorkalov · 2021-01-20T08:42:24Z

modules/imgcodecs/test/test_exr.impl.hpp

    ASSERT_EQ(CV_32FC1,img.type());

    ASSERT_TRUE(cv::imwrite(filenameOutput, img));
+    ASSERT_EQ(getFileSize(filenameOutput), 396);


Please add comment why you check file size here. It's important for test, but not obvious for other contributors.

asmorkalov · 2021-01-20T08:42:37Z

modules/imgcodecs/test/test_exr.impl.hpp

+  ASSERT_EQ(CV_32FC1, img.type());
+
+  ASSERT_TRUE(cv::imwrite(filenameOutput, img, params));
+  ASSERT_EQ(getFileSize(filenameOutput), 849);


The same for file size here.

asmorkalov · 2021-01-20T08:43:23Z

modules/imgcodecs/src/grfmt_exr.cpp

+            header.compression() = DWAB_COMPRESSION;
+            break;
+          default:
+            throw std::runtime_error("IMWRITE_EXR_COMPRESSION is invalid or not supported");


asmorkalov · 2021-01-25T17:44:27Z

@hallonterror Please take a look on CI status. BuildBot reports build issue:

/build/precommit_linux64/opencv/modules/imgcodecs/src/grfmt_exr.cpp: In member function 'virtual bool cv::ExrEncoder::write(const cv::Mat&, const std::vector<int>&)':
/build/precommit_linux64/opencv/modules/imgcodecs/src/grfmt_exr.cpp:595:27: error: 'CV_IMWRITE_EXR_COMPRESSION' was not declared in this scope
         if ( params[i] == CV_IMWRITE_EXR_COMPRESSION )

asmorkalov · 2021-02-05T08:05:43Z

@hallonterror Friendly reminder.

asmorkalov · 2021-02-10T15:00:44Z

@hallonterror Do you have a chance to finish the patch?

asmorkalov · 2021-02-16T09:56:57Z

@hallonterror Friendly reminder.

asmorkalov · 2021-02-16T10:32:09Z

@hallonterror Unfortunatelly I cannot push changes to you PR branch as you do it in master. I cherry picked the commits and created new PR with code review fixes: #19540

Adding possibility to select the compression type for the OpenEXR for…

1ca0f90

…mat. There are compression modes other than the default that are more suited for certain data. Mainly grainy/noisy data.

asmorkalov changed the title ~~Adding possibility to select the compression type for the OpenEXR for…~~ Adding possibility to select the compression type for the OpenEXR format Jan 11, 2021

asmorkalov added the category: imgcodecs label Jan 11, 2021

asmorkalov self-requested a review January 11, 2021 12:51

alalek reviewed Jan 11, 2021

View reviewed changes

Update constants_c.h

852526b

Reverting change to legacy/constants_c.h.

Update test_exr.impl.hpp

9aed9f9

Adding size check that ensures that the selection of PIZ compression has an effect on file size. For this particular file it gets larger. For other suitable image content the file should be smaller.

asmorkalov requested changes Jan 20, 2021

View reviewed changes

asmorkalov mentioned this pull request Feb 16, 2021

OpenEXR compression options #19540

Merged

6 tasks

asmorkalov closed this Feb 16, 2021

Uh oh!

Conversation

hallonterror commented Jan 8, 2021 • edited by asmorkalov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

asmorkalov commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

alalek Jan 9, 2021

Choose a reason for hiding this comment

Uh oh!

hallonterror Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmorkalov Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

hallonterror commented Jan 13, 2021

Uh oh!

Uh oh!

asmorkalov Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

asmorkalov Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

asmorkalov Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmorkalov commented Feb 5, 2021

Uh oh!

asmorkalov commented Feb 10, 2021

Uh oh!

asmorkalov commented Feb 16, 2021

Uh oh!

asmorkalov commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hallonterror commented Jan 8, 2021 •

edited by asmorkalov

Loading

asmorkalov commented Jan 11, 2021 •

edited

Loading

hallonterror Jan 13, 2021 •

edited

Loading

asmorkalov commented Jan 25, 2021 •

edited

Loading