[GSoC 2022] new imread api with named parameters and outputarray #22431

ocpalo · 2022-08-26T18:26:34Z

This proposed new imread api based on named parameters.

There are a few things that needs to be discussed. I have not seen enum class usage in openCV. Hence i prepared this pull request using plain enum. Imo, enum classes are better to express intention hence should be favor when usable. I propose to use

enum class ImreadError {
      OK = 0,
      FILE_NOT_FOUND,
      UNRECOGNIZED_FORMAT,
      SIZE_LIMIT_EXCEEDED,
      INVALID_HEADER,
      INVALID_DATA,
     };

It is more readable without IMREAD_ERROR_ prefix. And user can safely check error type with ImreadError::xxxx without implicit conversion.

The other thing is i created a new findDecoder method. This is kinda duplicate function. I could implement it via

static ImageDecoder findDecoder( string const& filename, ImreadError* error = nullptr) {
   ....
   if(error) {
        *error = IMREAD_OK;
   }
}

This is fine and works with the existing api, however it adds 2 extra if check. So if user reads thousand of images and does not use the new api, it adds 2 extra if check. I am not sure if this is ok or not.

Other than that, C++20 users can use designated initializers. Aggregate initialization does not permit default member initialization in C++11, this restriction removed at C++14.

In C++11 :

struct Test {
    int a;
    int b = 5;
    int c = 6;
};

int main()
{
    int a;
    Test t{a};
}

This code sample does not compile, but it compiles with C++14 version

Sample usage :

import cv2

ret,img = cv2.imread2("/home/ocpalo/Desktop/aaa.png", flags = 1)
print(ret) # 0
ret = cv2.imread2("/home/ocpalo/Desktop/aaa.png", maxSize = (100,100))
print(ret) # 3, aaa.png size is 3024x4032

// after c++20
cv::Mat mat;
auto ret = imread2("filename.png", mat, {.flags = IMREAD_COLOR, .maxSize {1024,1024});

// after c++14
cv::Mat mat2;
auto ret2 = imread2("filename.png", mat2, {IMREAD_COLOR});

================================================================================================

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

alalek · 2022-09-02T09:10:52Z

modules/imgcodecs/src/loadsave.cpp

+
+    // read the file signature
+    String signature(maxlen, ' ');
+    maxlen = fread( (void*)signature.c_str(), 1, maxlen, f );


c_str() returns const char* for a reason. Casting and writing to that pointer should be avoided (it could be a shadow copy).

Mutable std::vector<uint8_t> or cv::AutoBuffer<int8_t> should be used instead (perhaps including .checkSignature())

alalek · 2022-09-02T09:16:05Z

modules/imgcodecs/include/opencv2/imgcodecs.hpp

+enum ImreadError {
+      IMREAD_OK = 0,
+      IMREAD_ERROR_FILE_NOT_FOUND,
+      IMREAD_ERROR_UNRECOGNIZED_FORMAT,
+      IMREAD_ERROR_SIZE_LIMIT_EXCEEDED,
+      IMREAD_ERROR_INVALID_HEADER,
+      IMREAD_ERROR_INVALID_DATA,
+     };


indentation is 4 spaces.

Other enum definitions in the same file have 7 spaces. Should i keep 7 spaces or change to 4 spaces?

modules/imgcodecs/include/opencv2/imgcodecs.hpp

alalek · 2022-09-02T09:20:25Z

modules/imgcodecs/src/loadsave.cpp

+        if( flags & IMREAD_REDUCED_GRAYSCALE_2 )
+            scale_denom = 2;
+        else if( flags & IMREAD_REDUCED_GRAYSCALE_4 )
+            scale_denom = 4;
+        else if( flags & IMREAD_REDUCED_GRAYSCALE_8 )
+            scale_denom = 8;


Semantically this should be a direct field of ImreadParams.
Otherwise there is unresolved mess between flags and ImreadParams in designed API.

@alalek Do you mean that i should split flags into scale_denom and loadGdal variable and put it inside ImreadParams?

modules/imgcodecs/src/loadsave.cpp

modules/imgcodecs/test/test_read_write.cpp

…fault initialize

sturkmen72 · 2022-09-03T09:59:10Z

modules/imgcodecs/src/loadsave.cpp

+    }
+    if (!success)
+    {
+        image.release();


i have doubts about image.release();

asmorkalov · 2022-10-04T14:14:09Z

modules/imgcodecs/include/opencv2/imgcodecs.hpp

@@ -160,6 +167,32 @@ enum ImwritePAMFlags {
       IMWRITE_PAM_FORMAT_RGB_ALPHA       = 5
     };

+
+enum ImreadError {


ImreadStatus should be better, because some status are not errors.

asmorkalov · 2022-10-04T14:16:18Z

modules/imgcodecs/src/loadsave.cpp

+#endif
+
+    /// if no decoder was found, return nothing.
+    if( !decoder && error != IMREAD_OK){


error is not defined for GDAL case.

It should be ||, not &&. I have updated this one too.

asmorkalov · 2022-10-04T14:22:08Z

modules/python/test/test_imgcodecs.py

+    def test_imread2(self):
+        path = self.find_file('python/images/baboon.jpg', [os.environ.get('OPENCV_TEST_DATA_PATH')])
+        ret, img = cv.imread2(path, maxSize = (512, 512))
+        self.assertEqual(0, ret)


return codes should be available in python too. magic numbers look very strange.

They are available. It was my bad to not use it :). Thanks for pointing out.

asmorkalov · 2022-10-04T14:37:20Z

modules/imgcodecs/src/loadsave.cpp

@@ -257,6 +257,46 @@ static ImageDecoder findDecoder( const String& filename ) {
    return ImageDecoder();
 }

+static ImageDecoder findDecoder( const std::string& filename, ImreadError& error ) {


The function duplicates the original function, but adds more statuses. The old function should re-call the new one to not duplicate code.

asmorkalov · 2022-10-04T14:40:42Z

modules/imgcodecs/src/loadsave.cpp

+ *
+*/
+static ImreadError
+imread_2( String const& filename, OutputArray image, ImreadParams params)


The function duplicates the original function, but adds more statuses. The old function should re-call the new one to not duplicate code.

I will work on this one. Since I have separate flags variable into flags and scaleDenom, I need to find a way to merge them back together.

ocpalo · 2022-10-09T10:05:03Z

I think IGNORE_ORIENTATION should be a separate flag too.

Previous code is :

    if (!image_data.empty() && (flags & IMREAD_IGNORE_ORIENTATION) == 0 && flags != IMREAD_UNCHANGED )
    {
        ApplyExifOrientation(decoder->getExifTag(ORIENTATION), image_data);
    }

It would be better if

    if (!image_data.empty() && ignore_orientation && flags != IMREAD_UNCHANGED )
    {
        ApplyExifOrientation(decoder->getExifTag(ORIENTATION), image_data);
    }

Where ignore_orientation is a boolean variable.

ocpalo force-pushed the imreadapiv2 branch 5 times, most recently from 8995d78 to 934df25 Compare August 27, 2022 20:05

new imread api with named parameters and outputarray

9e8d2ff

ocpalo force-pushed the imreadapiv2 branch from 934df25 to 9e8d2ff Compare August 27, 2022 20:47

asmorkalov requested a review from vpisarev August 29, 2022 10:57

asmorkalov added category: imgproc feature labels Aug 29, 2022

asmorkalov added this to the 5.0 milestone Aug 29, 2022

alalek reviewed Sep 2, 2022

View reviewed changes

ocpalo added 3 commits September 2, 2022 22:39

python tests added, set flags to IMREAD_COLOR. Python api does not de…

460ccf9

…fault initialize

fix docs and EXPECT_EQ expected, actual

4cfb961

python test find_file fix

b2812eb

sturkmen72 reviewed Sep 3, 2022

View reviewed changes

modules/imgcodecs/src/loadsave.cpp

}

if (!success)

{

image.release();

Copy link

Contributor

sturkmen72 Sep 3, 2022

Choose a reason for hiding this comment

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

i have doubts about image.release();

seperated scale_denom from flags

0bacd9d

asmorkalov reviewed Oct 4, 2022

View reviewed changes

ImreadError to ImreadStatus, reuse findDecoder and update tests

e68113e

ocpalo mentioned this pull request Jul 4, 2023

is that possible to change default image from BGR to RGB at 5.0 version? #23906

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSoC 2022] new imread api with named parameters and outputarray #22431

[GSoC 2022] new imread api with named parameters and outputarray #22431

ocpalo commented Aug 26, 2022 •

edited

alalek Sep 2, 2022

alalek Sep 2, 2022

ocpalo Sep 2, 2022

alalek Sep 2, 2022

ocpalo Sep 3, 2022

sturkmen72 Sep 3, 2022

asmorkalov Oct 4, 2022

asmorkalov Oct 4, 2022

ocpalo Oct 5, 2022

asmorkalov Oct 4, 2022

ocpalo Oct 5, 2022

asmorkalov Oct 4, 2022

ocpalo Oct 5, 2022

asmorkalov Oct 4, 2022

ocpalo Oct 5, 2022

ocpalo commented Oct 9, 2022

[GSoC 2022] new imread api with named parameters and outputarray #22431

Are you sure you want to change the base?

[GSoC 2022] new imread api with named parameters and outputarray #22431

Conversation

ocpalo commented Aug 26, 2022 • edited

Pull Request Readiness Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocpalo commented Oct 9, 2022

ocpalo commented Aug 26, 2022 •

edited