imgcodecs: implement imencodemulti() by Kumataro · Pull Request #26211 · opencv/opencv

Kumataro · 2024-09-28T03:27:35Z

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

sturkmen72 · 2024-09-28T08:08:19Z

modules/imgcodecs/test/test_read_write.cpp

+{
+    vector<Mat> imgs;
+    Mat img(100, 100, CV_8UC1);
+    imgs.push_back(img);


@Kumataro could you test

vector<Mat> imgs; Mat img(100, 100, CV_8UC1); imgs.push_back(img); imgs.push_back(img.clone());

Thank you for your comment ! I mistakenly thought that it was enough to support multiple images for imencode().

But it seems that encoding multiple images requests reading callback function. After appending it, it works well.

Thank you for your comment ! I mistakenly thought that it was enough to support multiple images for imencode().

But it seems that encoding multiple images requests reading callback function. After appending it, it works well.

Thank you for your contribution. to me looks perfect.

modules/imgcodecs/src/loadsave.cpp

asmorkalov · 2024-10-14T06:08:36Z

modules/imgcodecs/src/loadsave.cpp

+        if( (!encoder->isFormatSupported(image.depth())) && (encoder->isFormatSupported(CV_8U)) )
        {
-            CV_Assert( encoder->isFormatSupported(CV_8U) );
+            CV_LOG_ONCE_WARNING(NULL, "Unsupported depth image for selected encoder is fallbacked to CV_8U.");
            image.convertTo( temp, CV_8U );


Log is good, but we get U.B., if the encoder does not support CV_8U by some reason.

Thank you for your review. This change comes from #26211 (comment) .

If there are no problem, I agree with revert it.

if( !encoder->isFormatSupported(image.depth() ) ) { CV_LOG_ONCE_WARNING(NULL, "Unsupported depth image for selected encoder is fallbacked to CV_8U."); CV_Assert( encoder->isFormatSupported(CV_8U) ); image.convertTo( temp, CV_8U );

Thank you for your review, I fixed it.

asmorkalov · 2024-10-14T06:10:49Z

@Kumataro Thanks a lot for the contribution! I hide the function implementation in cpp file and got rid of exceptions that are thrown outside of the function. The current image i/o contract is to be exception-safe and return bool status.

sturkmen72 · 2024-10-14T08:02:50Z

modules/imgcodecs/include/opencv2/imgcodecs.hpp


 @param ext File extension that defines the output format. Must include a leading period.
-@param img Image to be written.
+@param img Image to be compressed.


(Mat or vector of Mat) Image or Images to be compressed.

@asmorkalov imwrite() writes single and multiple images. AFAIK imwritemulti() is just for python. also imencode() behaves same as imwrite()

imwritemulti() definition is like

//! @brief multi-image overload for bindings CV_WRAP static inline bool imwritemulti(const String& filename, InputArrayOfArrays img, const std::vector<int>& params = std::vector<int>()) { return imwrite(filename, img, params); }

i think imwritemulti() also be defined as CV_EXPORTS_W and needs documentation

>>> help(cv2.imwritemulti) Help on built-in function imwritemulti: imwritemulti(...) imwritemulti(filename, img[, params]) -> retval .

Kumataro · 2024-10-14T21:11:58Z

I'm sorry some tests for AVIF are failed.
I'll analyze it later. Give me some time.

AVIF encoder called CV_CheckDepth() and exception is happened.
AVIF tests expects to it, but it is not able to be catched.

AVIF encoder calls CV_CheckType() with bit_depth == 6, img.depth() == CV_8U. cv::Exception will be thrown.

    CV_CheckType(
        img.type(),
        (bit_depth == 8 && img.depth() == CV_8U) ||
            ((bit_depth == 10 || bit_depth == 12) && img.depth() == CV_16U),
        "AVIF only supports bit depth of 8 with CV_8U input or "
        "bit depth of 10 or 12 with CV_16U input");

AVIF tests expect that cv::Exception throws. But they doesn't.

TEST_P(Imgcodecs_Avif_Image_EncodeDecodeSuite, imencode_imdecode) {
  const cv::Mat& img_original = get_img_original();
  ASSERT_FALSE(img_original.empty());

  // Encode.
  std::vector<unsigned char> buf;
  if (!IsBitDepthValid()) {
    EXPECT_THROW(cv::imencode(".avif", img_original, buf, encoding_params_),
                 cv::Exception);
    return;
  }

Maybe loadsave.cpp catches this exception, but it will not rethrow. So AVIF tests cannot catch.

    try {
        if (!isMultiImg)
            code = encoder->write(write_vec[0], params);
        else
            code = encoder->writemulti(write_vec, params);

        encoder->throwOnEror();
        CV_Assert( code );
    }
    catch (const cv::Exception& e)
    {
        CV_LOG_ERROR(NULL, "imencode(): can't encode data: " << e.what());
        code = false;
    }
    catch (...)
    {
        CV_LOG_ERROR(NULL, "imencode(): can't encode data: unknown exception");
        code = false;
    }

I think (1) updation AVIF tests to use return value instead of exception or (2) removing try and catch in loadsave.cpp is needed.

asmorkalov · 2024-10-15T06:41:45Z

Yes, I changed the implementation that makes code exception safe. I propose to tune tests to handle status.

asmorkalov · 2024-10-15T06:41:52Z

cc @vrabaud

Kumataro · 2024-10-15T09:59:40Z

Thank you for your comment, and I fixed to use status code instead of catching exception.

vrabaud · 2024-10-15T09:40:53Z

modules/imgcodecs/include/opencv2/imgcodecs.hpp


+/** @brief Encodes array of images into a memory buffer.
+
+The function is analog of cv::imencode for in-memory multi-page images compression.


The function is analog to cv::imencode for in-memory multi-page image compression.

Thank you for your review, I fixed this comment.

modules/imgcodecs/include/opencv2/imgcodecs.hpp

modules/imgcodecs/src/grfmt_tiff.cpp

asmorkalov · 2024-10-18T09:15:09Z

modules/imgcodecs/test/test_exr.impl.hpp

+TEST(Imgcodecs_EXR, imencode_regression_26207_extra)
+{
+    // CV_8U is not supported depth for EXR Encoder.
+    cv::Mat src(100, 100, CV_8UC1);


I propose to initialize mat with some values: some color or random data. It's applicable for the all new tests.

It makes test really deterministic

It makes sanitizers happy.

Thank you for your comment, and I agree with you !

I fixed it, and set const for their source images.

imgcodecs: implement imencodemulti() opencv#26211 Close opencv#26207 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [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 - [x] 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. - [x] The feature is well documented and sample code can be built with the project CMake

Kumataro added 2 commits September 28, 2024 12:19

imgcodecs: implement imencodemulti()

ec5342c

fix typo

1396537

sturkmen72 reviewed Sep 28, 2024

View reviewed changes

implement read function for TiffEncoderBufHelper

625efed

sturkmen72 reviewed Sep 28, 2024

View reviewed changes

modules/imgcodecs/src/loadsave.cpp Show resolved Hide resolved

Kumataro added 2 commits September 29, 2024 10:26

if encoder does not support CV_8U, do not fallback

194ed28

imgcodecs: throw Exception in encoders to user app

cc018c3

asmorkalov added test feature category: imgcodecs labels Oct 3, 2024

asmorkalov self-requested a review October 3, 2024 12:30

sturkmen72 mentioned this pull request Oct 10, 2024

Animated WebP and AVIF Support #25608

Merged

6 tasks

Review: do not throw exception im image i/o and hide imencodemulti impl.

f6b4df2

asmorkalov reviewed Oct 14, 2024

View reviewed changes

sturkmen72 reviewed Oct 14, 2024

View reviewed changes

asmorkalov and others added 2 commits October 14, 2024 11:04

Build fix.

65ce6c8

restore to fallback from unsupported depth to CV_8U

eae3893

avif: use status code instead of catch exception

86fbd81

vrabaud approved these changes Oct 15, 2024

View reviewed changes

Kumataro added 2 commits October 15, 2024 21:44

fix comment

c180235

use std::memcpy instead of memcpy in C

9da95de

asmorkalov added this to the 4.11.0 milestone Oct 15, 2024

asmorkalov self-assigned this Oct 15, 2024

asmorkalov reviewed Oct 18, 2024

View reviewed changes

initialize mat with some value

8056dca

asmorkalov approved these changes Oct 18, 2024

View reviewed changes

asmorkalov merged commit 35dbf32 into opencv:4.x Oct 18, 2024

asmorkalov mentioned this pull request Oct 23, 2024

5.x merge 4.x #26358

Merged


		/** @brief Encodes array of images into a memory buffer.

		The function is analog of cv::imencode for in-memory multi-page images compression.

Uh oh!

Conversation

Kumataro commented Sep 28, 2024

Pull Request Readiness Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Oct 14, 2024

Uh oh!

sturkmen72 Oct 14, 2024 • 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!

sturkmen72 Oct 14, 2024 • 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!

Kumataro commented Oct 14, 2024

Uh oh!

asmorkalov commented Oct 15, 2024

Uh oh!

asmorkalov commented Oct 15, 2024

Uh oh!

Kumataro commented Oct 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sturkmen72 Oct 14, 2024 •

edited

Loading

sturkmen72 Oct 14, 2024 •

edited

Loading