Extend the signature of imdecodemulti() #24405
Conversation
Add an overload of imdecodemulti() to decode a single, selected image from a buffer in memory that holds a multi-page image
|
@kochanczyk thanks a lot for the PR. Could you add test like this: https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/test/test_tiff.cpp#L968. |
|
@kochanczyk, thanks for the patch! It looks like a useful addition. I suggest however not to add extra entry, but add an optional Also, please, add a test for this functionality |
…ges of multi-page images Be default, all pages are decoded. If used, the additional argument may specify a continuous selection of pages to decode.
|
Dear Alex and Vadim, as per your requests, I used cv::Range and provided a separate test. Thank you for your interest and feedback. @vpisarev Update: It seems that the recommended use of Update: In commit 18894cf I've tried passing the range by value, not by reference, however this did not fix the issue for the iOS. I have no experience with this framework. Are you able to advise how this can be fixed? |
modules/imgcodecs/test/test_tiff.cpp
Outdated
| for (size_t start = 0; start < page_count; start++) | ||
| { | ||
| for (size_t end = start + 1; end <= page_count; end++) | ||
| { | ||
| vector<Mat> pages_from_imdecodemulti; | ||
| Range const range(start, end); | ||
| bool res = imdecodemulti(buf, mode, pages_from_imdecodemulti, range); | ||
| ASSERT_TRUE(res == true); | ||
| ASSERT_EQ(static_cast<size_t>(range.size()), pages_from_imdecodemulti.size()); | ||
|
|
||
| for (int i = range.start; i < range.end; i++) | ||
| { | ||
| EXPECT_PRED_FORMAT2(cvtest::MatComparator(0, 0), | ||
| pages_from_imread[i], | ||
| pages_from_imdecodemulti[i - range.start]); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I do not think that we need all combination here. It's enough to call imdecodemulti once with trange like (1, size-2) to check not default behaviour. Other cases and tiff specific are indirectly tested with imreadmulti.
There was a 8000 problem hiding this comment.
I have revised the code as requested, but please note that, strictly speaking, the proposed test did not check all combinations, which would be nonsensical, but all contiguous subsequences. If you have a sequence of length n, there are n × (n + 1)/2 ~ O(n2) contiguous subsequences, which for the current n = 6 gives 21.
Also, in test decode_multipage_use_memory_buffer_all_pages, binary file reading is revised to be identical to that in test decode_multipage_use_memory_buffer_selected_pages.
This change fixes building OpenCV under iOS Framework.
modules/imgcodecs/src/loadsave.cpp
Outdated
| else | ||
| { | ||
| CV_Assert( range.start >= 0 ); | ||
| CV_Assert( range.size() > 0 ); |
There was a problem hiding this comment.
CV_CheckGE
CV_CheckGT
The reverted change (1) did not fix building OpenCV under iOS Framework (contrary to wishful thinking in the reverted commit message) and additionally (2) created an opportunity for object slicing. This reverts commit 18894cf.
|
Updated skip list with imreadmulti in ABI/API checker on CI. |
|
The iOS issue is related to Obj-C/Swift bindings issue: @VadimLevin @komakai could you help with it? |
|
The issue lies on generator side, because Obj-C doesn't support default function arguments. As a workaround you can create 2 functions: CV_EXPORTS_W bool imdecodemulti(InputArray buf, int flags, CV_OUT std::vector<Mat>& mats) {
return imdecodemulti(buf, flags, mats, Range::all());
}
CV_EXPORTS_W bool imdecodemulti(InputArray buf, int flags, CV_OUT std::vector<Mat>& mats,
const cv::Range& range);Or this should be handled on generator side: For each function with default arguments create a trampoline function with and without defaulted argument. I'm not familiar with Obj-C generator, maybe it is already working that way. |
|
@asmorkalov @VadimLevin |
This is an attempt to fix a building issue under iOS caused by a recently added default-valued argument in imdecodemulti().
modules/core/misc/objc/gen_dict.json
Outdated
| @@ -167,7 +167,9 @@ | |||
| "from_cpp": "[Point3i fromNative:%(n)s]" | |||
| }, | |||
| "Range": { | |||
| "objc_type": "Range*" | |||
| "objc_type": "Range*", | |||
| "to_cpp": "%(n)s.nativeRef", | |||
There was a problem hiding this comment.
Looks good - let me know if that works OK - if it doesn't then I'll take a closer look
There was a problem hiding this comment.
@komakai This change unfortunately still does not fix the issue:
/Users/opencv-cn/GHA-OCV-1/_work/opencv/opencv/ios/build/build-armv7-iphoneos/modules/objc_bindings_generator/ios/gen/objc/imgcodecs/Imgcodecs.mm:124:77: error: property 'nativeRef' not found on object of type 'Range *'
bool retVal = cv::imdecodemulti(buf.nativeRef, flags, matsVector, range.nativeRef);
^
1 error generated.
https://github.com/opencv/opencv/actions/runs/6644615646/job/18069638575?pr=24405#step:7:7279
There was a problem hiding this comment.
@kochanczyk OK - let me take a closer look
|
iOS build: |
|
@kochanczyk @asmorkalov Can you try with PR #24454 ? That should solve it |
Refactor ObjectiveC Range class #24454 ### Pull Request Readiness Checklist - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to the proper branch Fix for build issue in #24405
|
@komakai Thanks a lot for the quick solution. The related PR has been merged. |
|
Dear Giles, Alex, and Vadim, thank you very much for all your assistance! I am happy to see the pull request merged. |
Refactor ObjectiveC Range class opencv#24454 ### Pull Request Readiness Checklist - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to the proper branch Fix for build issue in opencv#24405
Extend the signature of imdecodemulti() opencv#24405 (Edited after addressing Reviewers' comments.) Add an argument to `imdecodemulti()` to enable optional selection of pages of multi-page images. Be default, all pages are decoded. If used, the additional argument may specify a continuous selection of pages to decode. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] 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 - [X] I agree to contribute to the project under Apache 2 License. - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [x] 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
Refactor ObjectiveC Range class opencv#24454 ### Pull Request Readiness Checklist - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to the proper branch Fix for build issue in opencv#24405
Extend the signature of imdecodemulti() opencv#24405 (Edited after addressing Reviewers' comments.) Add an argument to `imdecodemulti()` to enable optional selection of pages of multi-page images. Be default, all pages are decoded. If used, the additional argument may specify a continuous selection of pages to decode. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] 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 - [X] I agree to contribute to the project under Apache 2 License. - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [x] 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
Refactor ObjectiveC Range class opencv#24454 ### Pull Request Readiness Checklist - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [x] The PR is proposed to the proper branch Fix for build issue in opencv#24405
Extend the signature of imdecodemulti() opencv#24405 (Edited after addressing Reviewers' comments.) Add an argument to `imdecodemulti()` to enable optional selection of pages of multi-page images. Be default, all pages are decoded. If used, the additional argument may specify a continuous selection of pages to decode. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] 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 - [X] I agree to contribute to the project under Apache 2 License. - [X] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [x] 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
(Edited after addressing Reviewers' comments.)
Add an argument to
imdecodemulti()to enable optional selection of pages of multi-page images.Be default, all pages are decoded. If used, the additional argument may specify a continuous selection of pages to decode.
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.