[GSoC 2022] spng encoder/decoder added as optional png codec by ocpalo · Pull Request #22226 · opencv/opencv

ocpalo · 2022-07-10T21:12:30Z

Merge with extra: opencv/opencv_extra#995

This pull request contains the implementation of PngEncoder using spng library. One thing that I am not sure about is the CMake configuration. I appreciate any help on CMake configuration

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

modules/imgcodecs/src/grfmt_spng.cpp

vpisarev · 2022-07-12T18:55:28Z

modules/imgcodecs/src/grfmt_spng.cpp

+            if(!ret) {
+                image_width = image_size / m_height;
+
+                ret = spng_decode_image(png_ptr, nullptr, 0, fmt, SPNG_DECODE_PROGRESSIVE | decode_flags);


is it necessary to call this function? You decode the image row by row after it in any case.

spng_decode_row() requires the decoder to be initialized by calling spng_decode_image() with the SPNG_DECODE_PROGRESSIVE flag set. This function call does not decode image. It just sets internal flags. It is necessary.

modules/imgcodecs/src/grfmt_spng.cpp

vpisarev · 2022-07-12T18:58:38Z

modules/imgcodecs/src/grfmt_spng.cpp

+        volatile bool result = false;
+        close();
+
+        spng_ctx *ctx = spng_ctx_new(0);


where is ctx released?

The following lines should be

if(!ctx) { spng_ctx_free(ctx); return false; }

I missed that. Will fix it!

Done!

vpisarev · 2022-07-12T18:59:53Z

modules/imgcodecs/src/grfmt_spng.cpp

+            }
+        }
+
+        close();


the API is unstable. You call close() after data is read. User may want, for some reason, call readData several times.

I don't see a reason why a user wants to read the same data twice. The current png decoder is implemented in this way too. After reading the data, the user can just copy it.

Reference to the current PNG codec.

https://github.com/opencv/opencv/blob/4.x/modules/imgcodecs/src/grfmt_png.cpp#L307

Mat mat; decoder.readData(mat) Mat matOther; matOther = mat;

Users should favor that in case of performance. But I think you are right. If the user calls twice in a row, its behavior is undefined. We also should remove this from the current png codec implementation too.

I changed it to

if(!result) close();

Should I create a quick patch for the current PNG decoder too?

vpisarev · 2022-07-12T19:02:01Z

modules/imgcodecs/src/grfmt_spng.cpp

+    }
+
+
+    int SPngEncoder::writeDataToBuf(void *ctx, void *user, void *dst_src, size_t length)


I do not see where proper encoding to memory is implemented. SPngEncoder::write() only writes image to disk

If if understand it correctly, there are related tests in OpenCV for that. It encodes to the buffer and compares with some Vector. All the test cases is passed.

The answer to your question is in line 407, it checks if the encoder should write it to buffer or file. I copied that part from the libpng codec.

if( m_buf ) { spng_set_png_stream(ctx,(spng_rw_fn*)writeDataToBuf, this); } else { f = fopen( m_filename.c_str(), "wb" ); if( f ) spng_set_png_file( ctx, f ); }

Here spng_set_png_stream function call does that.

asmorkalov · 2022-07-20T06:24:12Z

modules/imgcodecs/src/grfmt_spng.cpp

+        spng_ctx *ctx = spng_ctx_new(0);
+
+        if(!ctx) {
+            spng_ctx_free(ctx);


Please check if spng_ctx_free with NULL is expected behaviour.

void spng_ctx_free(spng_ctx *ctx) { if(ctx == NULL) return;

It is expected behavior but it seems unnecessary. For the future releases of spng, it might be useful to keep this line.

asmorkalov · 2022-07-20T06:25:41Z

modules/imgcodecs/src/grfmt_spng.cpp

+        m_ctx = ctx;
+        spng_set_crc_action(ctx, SPNG_CRC_USE, SPNG_CRC_USE);
+
+        size_t limit = 1024 * 1024 * 64;


Author of the libspng wrote this in the example for the setting the chunk limit.

/* Set memory usage limits for storing standard and unknown chunks, this is important when reading untrusted files! */

I thought it was a good idea. But this limits the chunks size. Now I think it is better if I remove it. This methods equivalent is png_set_chunk_malloc_max.

Should i keep this or remove it?

I do not have strong opinion here. It's ok to have chunk limit. Please add note why you use the particular value or link to original code, if it's inherited behaviour.

ocpalo · 2022-07-25T20:57:14Z

I did some performance tests and right now it performs worse than the current png codec. I am going to look more at how I could improve it. Let's not merge this one yet.

Edit: I managed to improve a little bit but i think because of my computer, test results are unreliable.

The code:

    const string root = cvtest::TS::ptr()->get_data_path();
    const string imgName = root + "../cv/shared/lena.png";
    cv::TickMeter tick;
    tick.start();
    for(int i = 0; i< 1000; ++i) {
      Mat original_image = imread(imgName);
    }
    tick.stop();
    std::cout << tick.getAvgTimeMilli()/1000 << "\n";

I run this 4 time for each codec. Here are the results:
--------PNG--------SPNG--------

9.16589 ---- 10.267
10.6545 ---- 10.7098
8.33594 ---- 8.49084
7.30177 ---- 7.3179

Let me know what you think

Decode
Perf Test Result:
SPNG: [ PERFSTAT ] (samples=100 mean=10.11 median=7.19 min=7.01 stddev=4.59 (45.4%))

PNG: [ PERFSTAT ] (samples=100 mean=8.30 median=5.18 min=4.93 stddev=5.25 (63.2%))

Encode
Perf Test Result:
SPNG: [ PERFSTAT ] (samples=100 mean=22.68 median=19.70 min=15.70 stddev=7.55 (33.3%))

PNG: [ PERFSTAT ] (samples=100 mean=22.84 median=22.66 min=12.51 stddev=5.07 (22.2%))

ocpalo · 2022-07-26T22:22:15Z

SPNG perf:
[ PERFSTAT ] (samples=13 mean=11.52 median=11.42 min=11.25 stddev=0.29 (2.5%))

PNG perf:
[ PERFSTAT ] (samples=10 mean=13.22 median=13.19 min=12.99 stddev=0.18 (1.4%))

Well interesting. I upgraded my Ubuntu to 22.04 and it seems SPNG slightly performs better now.

And i changed only one line. spng_ctx_new( SPNG_CTX_IGNORE_ADLER32 ); I ignore ADLER32 checksum in deflate streams.

Edit: About ignoring ADLER32 checksum, there is another library that uses libspng, libvips, also does the same thing. And since deflate streams lossless compressions, there is no need to check checksum in deflate streams.

ocpalo · 2022-07-29T06:51:00Z

Potential performance improvement: Convert RGB to BGR while decoding image row-by-row. All test cases are passed but ROI still might not work.

 ret = spng_decode_row(png_ptr, img.data + row_info.row_num * image_width, image_width);

Add img.step here.

ocpalo · 2022-08-02T15:32:18Z

imread(img, IMREAD_GRAYSCALE) tests fail. I think the reason is png RGB->Gray conversion is different than OpenCV RGB->Gray.

Edit: Note to myself, change project name SPNG_LIBRARY to PNG_LIBRARY

ocpalo · 2022-08-09T18:12:50Z

@asmorkalov can you run the workflow? Lets see if spng works on other platforms.

ocpalo · 2022-08-10T21:12:10Z

With the 3024x4032 size and 11mb image, the perf results are:

[ RUN      ] PNG.decode
[ PERFSTAT ]    (samples=10   mean=179.53   median=178.60   min=177.07   stddev=2.56 (1.4%))

cmake release mode spng
[ RUN      ] PNG.decode
[ PERFSTAT ]    (samples=10   mean=151.74   median=150.96   min=150.39   stddev=1.60 (1.1%))

vpisarev · 2022-08-11T10:47:16Z

@ocpalo, I tested it on my machine; works well well 👍

ocpalo · 2022-08-11T10:51:59Z

I temporarily changed cmake changes to test spng decoder in CI. Before merge, let me revert cmake changes and make spng optional decoder.

ocpalo · 2022-08-11T13:05:20Z

Ubuntu2004-x64 - macOS-ARM/x64 Error:

[ RUN      ] Test_ONNX_layers.ReduceSum/0, where GetParam() = OCV/CPU
unknown file: Failure
C++ exception with description "OpenCV(4.6.0-dev) /home/ci/opencv/modules/ts/src/ts.cpp:1064: error: (-2:Unspecified error) OpenCV tests: Can't find required data file: dnn/onnx/models/reduce_sum_axis_dynamic_batch.onnx in function 'findData'
" thrown in the test body.

Windows Error:
Fails to build spng library.

I need to figure out how to compile libspng in Windows environment

sturkmen72 · 2022-08-11T14:23:48Z

reminder:
i think in build information we should see if linspng used for png codec

currently we see
in linux
PNG: /usr/lib/aarch64-linux-gnu/libpng.so (ver 1.6.37)

in windows
PNG: build (ver 1.6.37)

ocpalo · 2022-08-26T22:33:22Z

I rebased all of my commits with the changes that alalek requested on CMakeLists. I am working on to update tests for pngsuite. I will probably finish by the noon, tomorrow.

ocpalo · 2022-08-26T23:10:21Z

Few linux commands did the trick. Here we go. Thanks to @sturkmen72 for separate xml files. He was generated those files while ago and shared with me. @alalek @vpisarev could you review if this is ok now?

modules/imgcodecs/test/test_png.cpp

ocpalo · 2022-08-28T08:53:52Z

@alalek I have updated the test_png files. But as i said above, i did not understand how to handle zlib dependency handling if ZLIB is disabled. If you provide me more information, i am happy to do it :)

alalek

Thank you for contribution!

ocpalo · 2022-08-28T21:04:15Z

fyi, i force pushed to fix docs warnings(whitespaces)

ocpalo · 2022-08-29T08:22:41Z

It seems the runflow fails on some flows. However, when i check the build logs, it builds with the libpng since i have made it libspng as optional. I think the workflow fails are not related with my pullrequest.

edit: i made spng as optional so it does not build during the workflow.

ocpalo · 2022-08-29T09:05:02Z

You can check this workflow where i enabled libspng. Since then, i have not made any changes in the code except alalek cmake and test requests. @asmorkalov .

vpisarev · 2022-08-30T08:07:50Z

@asmorkalov, looks like the failures have no relation with the patch. Can we finally merge it?

asmorkalov · 2022-08-31T10:26:10Z

I tested the PR manually with ARMv7 with Ubuntu 14.04 and x86_64 with Ubuntu 18.04.

Pushed new commit to enable EXIF rotation test. It's supported by libspng correctly.
I continuously get 2 test failures with my Ubuntu 18.04. 2 images from corrupted list are decoded

[ RUN      ] Imgcodecs_Png_PngSuite_Corrupted.decode/3, where GetParam() = "xcsn0g01"
/mnt/projects/Projects/OpenCV/opencv-master/modules/imgcodecs/test/test_png.cpp:385: Failure
Value of: src.empty()
  Actual: false
Expected: true
[  FAILED  ] Imgcodecs_Png_PngSuite_Corrupted.decode/3, where GetParam() = "xcsn0g01" (0 ms)
[ RUN      ] Imgcodecs_Png_PngSuite_Corrupted.decode/4, where GetParam() = "xd0n2c08"
[       OK ] Imgcodecs_Png_PngSuite_Corrupted.decode/4 (0 ms)
[ RUN      ] Imgcodecs_Png_PngSuite_Corrupted.decode/5, where GetParam() = "xd3n2c08"
[       OK ] Imgcodecs_Png_PngSuite_Corrupted.decode/5 (0 ms)
[ RUN      ] Imgcodecs_Png_PngSuite_Corrupted.decode/6, where GetParam() = "xd9n2c08"
[       OK ] Imgcodecs_Png_PngSuite_Corrupted.decode/6 (0 ms)
[ RUN      ] Imgcodecs_Png_PngSuite_Corrupted.decode/7, where GetParam() = "xdtn0g01"
[       OK ] Imgcodecs_Png_PngSuite_Corrupted.decode/7 (0 ms)
[ RUN      ] Imgcodecs_Png_PngSuite_Corrupted.decode/8, where GetParam() = "xhdn0g08"
/mnt/projects/Projects/OpenCV/opencv-master/modules/imgcodecs/test/test_png.cpp:385: Failure
Value of: src.empty()
  Actual: false
Expected: true
[  FAILED  ] Imgcodecs_Png_PngSuite_Corrupted.decode/8, where GetParam() = "xhdn0g08" (0 ms)

xcsn0g01.png works well with OpenCV (imshow) and my KDE image viewer.
xhdn0g08.png shows blury lines with OpenCV and KDE viewer reports corrupted image.
The issue is reproducible with both system zlib and self build (-DBUILD_ZLIB=ON).

ocpalo · 2022-08-31T10:42:40Z

@asmorkalov that is a very good catch. I was not able to catch this because I have not tested after changing unit test structure. I assumed it will pass as before but it seems alalek was right to change to catch this kind of issues.

About the issue, in the implementation, I ignore the checksum of IHDR and IDAT. I prefer to do it that way because many libraries ignores it in their implementation. Since it does not calculate the checksum, it decodes the images into mat object. How should I approach this situation? Ignore the calculation of checksum and change unit tests or should I modify the existing code to calculate the checksum? I think it is better you, opencv team, decide how to handle this situation. Downside of calculation checksum is it reduces the performance.

asmorkalov · 2022-08-31T13:21:10Z

It's interesting question. I do not see issue with Ubuntu 14.04 on my jetson tk1. First of all we need deterministic behavior everywhere. I'm looking on Ubuntu 20.04 right now to find correlation.

sturkmen72 · 2022-08-31T14:48:01Z

for information see http://www.schaik.com/pngsuite/pngsuite_xxx_png.html

asmorkalov · 2022-08-31T14:57:20Z

The issue is reproducible with Ubuntu 20.04.

ocpalo · 2022-08-31T17:45:06Z

@asmorkalov to keep the existing behaviour of the current png codec, i have changed to calculate to checksum. Sorry to bother you with this. I have tested on Ubuntu 22.04 and tests are passed now. Could you please review/re-run your tests?

As a side note, we can provide some kind of flags to users to set these. For example, in my latest pull request about new imread function with named-parameters, we can add some kind of flags, boolean, enum etc, to let user to control it. I am not expert at codecs but i believe most of the codecs have this kind of checks, flags etc.

vpisarev · 2022-09-02T10:09:34Z

@asmorkalov, can you check this patch once again and see if the problem is solved on your Ubuntu machine?

asmorkalov

👍 Great job! Thanks a lot for the contribution!

opencv#22226 libspng, pngsuite and png image for perf test

ocpalo force-pushed the libspng branch 3 times, most recently from 27ab72d to c204f95 Compare July 12, 2022 12:26

vpisarev reviewed Jul 12, 2022

View reviewed changes

modules/imgcodecs/src/grfmt_spng.cpp Outdated Show resolved Hide resolved

vpisarev reviewed Jul 12, 2022

View reviewed changes

modules/imgcodecs/src/grfmt_spng.cpp Outdated Show resolved Hide resolved

vpisarev reviewed Jul 12, 2022

View reviewed changes

ocpalo force-pushed the libspng branch 3 times, most recently from 9456e7b to 5f15415 Compare July 12, 2022 23:43

ocpalo requested a review from vpisarev July 12, 2022 23:52

ocpalo force-pushed the libspng branch from 9f972eb to 699ffca Compare July 15, 2022 19:59

asmorkalov reviewed Jul 20, 2022

View reviewed changes

ocpalo force-pushed the libspng branch 3 times, most recently from 5c9a09b to b3a36ec Compare July 26, 2022 22:21

ocpalo changed the title ~~[GSoC 2022] spng encoder/decoder added as optional png codec~~ [GSoC 2022][WIP] spng encoder/decoder added as optional png codec Aug 7, 2022

vpisarev approved these changes Aug 11, 2022

View reviewed changes

ocpalo force-pushed the libspng branch from 16cadc2 to 5e68ff8 Compare August 26, 2022 23:10

ocpalo requested a review from alalek August 26, 2022 23:12

alalek reviewed Aug 27, 2022

View reviewed changes

modules/imgcodecs/test/test_png.cpp Outdated Show resolved Hide resolved

modules/imgcodecs/test/test_png.cpp Outdated Show resolved Hide resolved

modules/imgcodecs/test/test_png.cpp Outdated Show resolved Hide resolved

ocpalo force-pushed the libspng branch from 5e68ff8 to 017defa Compare August 28, 2022 08:52

ocpalo requested a review from alalek August 28, 2022 08:53

alalek approved these changes Aug 28, 2022

View reviewed changes

spng encoder/decoder added as optional png codec

3929e26

ocpalo force-pushed the libspng branch from 017defa to 3929e26 Compare August 28, 2022 21:04

Enabled EXIF orientation tests for libspng.

5185544

asmorkalov self-requested a review August 31, 2022 09:58

decode chunks and calculate checksums

c4a6e1f

asmorkalov approved these changes Sep 5, 2022

View reviewed changes

asmorkalov merged commit 448e3a7 into opencv:4.x Sep 5, 2022

alalek mentioned this pull request Jan 8, 2023

(5.x) Merge 4.x #23113

Merged

savuor pushed a commit to nickyu-zhu/opencv that referenced this pull request Oct 27, 2023

\ opencv#22226 libspng, pngsuite and png image for perf test

a4333fe

savuor pushed a commit to nickyu-zhu/opencv that referenced this pull request Oct 27, 2023

Merge pull request opencv#995 from ocpalo:libspng

86a2a38

opencv#22226 libspng, pngsuite and png image for perf test

		}


		int SPngEncoder::writeDataToBuf(void ctx, void user, void *dst_src, size_t length)

Uh oh!

Conversation

ocpalo commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocpalo Jul 12, 2022 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocpalo Jul 12, 2022 • 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

ocpalo commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocpalo commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocpalo commented Jul 29, 2022

Uh oh!

ocpalo commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocpalo commented Aug 9, 2022

Uh oh!

ocpalo commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpisarev commented Aug 11, 2022

Uh oh!

ocpalo commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocpalo commented Aug 11, 2022

Uh oh!

sturkmen72 commented Aug 11, 2022

Uh oh!

ocpalo commented Aug 26, 2022

Uh oh!

ocpalo commented Aug 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocpalo commented Aug 28, 2022

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

ocpalo commented Aug 28, 2022

Uh oh!

ocpalo commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ocpalo commented Jul 10, 2022 •

edited

Loading

ocpalo Jul 12, 2022 •

edited

Loading

ocpalo Jul 12, 2022 •

edited

Loading

ocpalo commented Jul 25, 2022 •

edited

Loading

ocpalo commented Jul 26, 2022 •

edited

Loading

ocpalo commented Aug 2, 2022 •

edited

Loading

ocpalo commented Aug 10, 2022 •

edited

Loading

ocpalo commented Aug 11, 2022 •

edited

Loading

ocpalo commented Aug 29, 2022 •

edited

Loading