Added Exif parsing for PNG Issue 16579 by raaldrid · Pull Request #19439 · opencv/opencv

raaldrid · 2021-02-01T20:39:50Z

Merge with extra: opencv/opencv_extra#843

Added Exif parsing for PNG files to support Exif orientation tag. Moved decoder specific Exif parsing to JPEG and PNG decoders, respectively. Issue 16579

resolves #16579

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

opencv_extra=raaldrid-exif_PNG_test_files

asmorkalov · 2021-02-02T11:20:04Z

@rrrapha Thanks for the contribution! Please pay attention on CI builds. It looks like your patch breaks build:

[ 30%] [ 30%] /build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp: In function 'void* cv::imread_(const cv::String&, int, int, cv::Mat*)':
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp:511:51: error: invalid initialization of non-const reference of type 'cv::ExifReader&' from an rvalue of type 'cv::ExifReader'
         ApplyExifOrientation(decoder->exif(), *mat);
                                                   ^
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp:357:13: error: in passing argument 1 of 'void cv::ApplyExifOrientation(cv::ExifReader&, cv::Mat&)'
 static void ApplyExifOrientation(ExifReader& exif, Mat& img)
             ^
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp: In function 'bool cv::imreadmulti_(const cv::String&, int, std::vector<cv::Mat>&)':
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp:611:54: error: invalid initialization of non-const reference of type 'cv::ExifReader&' from an rvalue of type 'cv::ExifReader'
             ApplyExifOrientation(decoder->exif(), mat);
                                                      ^
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp:357:13: error: in passing argument 1 of 'void cv::ApplyExifOrientation(cv::ExifReader&, cv::Mat&)'
 static void ApplyExifOrientation(ExifReader& exif, Mat& img)
             ^
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp: In function 'void* cv::imdecode_(const cv::Mat&, int, int, cv::Mat*)':
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp:906:51: error: invalid initialization of non-const reference of type 'cv::ExifReader&' from an rvalue of type 'cv::ExifReader'
         ApplyExifOrientation(decoder->exif(), *mat);
                                                   ^
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/loadsave.cpp:357:13: error: in passing argument 1 of 'void cv::ApplyExifOrientation(cv::ExifReader&, cv::Mat&)'
 static void ApplyExifOrientation(ExifReader& exif, Mat& img)

alalek · 2021-02-02T08:17:21Z

modules/imgcodecs/src/grfmt_base.hpp

    int width() const { return m_width; }
    int height() const { return m_height; }
    virtual int type() const { return m_type; }
+    ExifReader exif() const { return m_exif; }


ExifReader

reference: ExifReader&?

Thank you @alalek. My bad. I changed that code slightly to return an ExifEntry_t copy instead, as returning an exif reference that could be altered seemed poor design. The decoder should be the only class able to edit the ExifReader.

The current PR contains squashed fix, and appears to be failing the build now only due to missing the png exif orientation test image files to execute the new test cases. The test files are in a PR on opencv_extra: opencv/opencv_extra#843

asmorkalov · 2021-02-03T08:11:30Z

@raaldrid Build is still broken:

/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/grfmt_jpeg.cpp:247:46: error: 'AppMarkerTypes' is not a class or namespace
             jpeg_save_markers(&state->cinfo, AppMarkerTypes::APP1, 0xffff);
                                              ^
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/grfmt_jpeg.cpp: In member function 'virtual bool cv::JpegDecoder::readData(cv::Mat&)':
/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/grfmt_jpeg.cpp:465:40: error: 'AppMarkerTypes' is not a class or namespace
                 if (cmarker->marker == AppMarkerTypes::APP1)
                                        ^
make[2]: *** [modules/imgcodecs/CMakeFiles/opencv_imgcodecs.dir/src/grfmt_jpeg.cpp.o] Error 1

raaldrid · 2021-02-04T19:47:11Z

Hi @alalek, apologies for the build issues. I need to configure my local setup to use CMake natively. I am using VS CMake plugin which is obviously not mirroring the build process accurately. While I'm doing that, I wanted to bring to your attention the latest error(s):

/build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/grfmt_png.cpp:291:50: error: 'PNG_INFO_eXIf' was not declared in this scope if( png_get_valid(png_ptr, info_ptr, PNG_INFO_eXIf) ) ^ /build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/grfmt_png.cpp:292:67: error: 'png_get_eXIf_1' was not declared in this scope png_get_eXIf_1(png_ptr, info_ptr, &num_exif, &exif); ^ /build/precommit_linux64/3.4/opencv/modules/imgcodecs/src/grfmt_png.cpp:294:67: error: 'png_get_eXIf_1' was not declared in this scope png_get_eXIf_1(png_ptr, end_info, &num_exif, &exif);

This stumped me for a little. What I believe to be happening is that the build is using version 1.2.50 of libpng. Exif support was added to libpng in version 1.6.31 (revised in 1.6.32, so that version or greater is desired). From the CMake stdio log:

-- Found PNG: /usr/lib/x86_64-linux-gnu/libpng.so (found version "1.2.50") -- Looking for /usr/include/libpng/png.h -- Looking for /usr/include/libpng/png.h - found

Is it possible to update the libpng version in 3.4 branch to 1.6.32 or greater? Thanks.

References for libpng version 1.6.31 and 1.6.32 exif support/changes:
http://www.libpng.org/pub/png/libpng-manual.txt
https://fossies.org/linux/libpng/CHANGES

alalek · 2021-02-04T19:53:16Z

/usr/lib/x86_64-linux-gnu/libpng.so

This is a system dependency. It can't be upgraded.

You should add version check through #if and disable functionality in #else branch for unsupported libpng versions.

raaldrid · 2021-02-05T22:30:17Z

I added #if version check to the PR. Interestingly it looks like the included header version is different from the libpng.so file for 3 of the builds which appear to be failing due to the new test cases. The new test cases I also #if version checked so they should not be running unless a newer version of libpng is being used, thus my theory that the included header doesn't match the .so version. Ideas? I #if checked the new test cases because they will fail if an old lippng is in use.

asmorkalov · 2021-02-07T11:40:46Z

jenkins cn please retry a build

raaldrid · 2021-02-08T18:44:31Z

@asmorkalov I don't understand the meaning behind "jenkins cn". I did retry a build. The Jenkins builds are still failing, tho buildbot seems fine. The Jenkins builds are now failing due to other test cases not being able to find files:

Test Result (4 failures / ±0)opencv_test_dnn / Test_ONNX_layers.NormalizeFusionSubgraph/0opencv_test_dnn / Test_ONNX_layers.Mish/0opencv_test_dnn / Test_ONNX_layers.CalculatePads/0opencv_test_dnn / Test_TensorFlow_layers.leaky_relu/0

Question: I am puzzled why the buildbot builds are all passing when my PR for opencv_extra hasn't been approved? (opencv/opencv_extra#843) The test files for png exif shouldn't exist in the repo so I am puzzled why those test cases are succeeding. (And per the debug output they are succeeding, not being omitted due to an old libpng version.)

asmorkalov · 2021-02-09T07:30:16Z

@raaldrid Sorry for confusion. Jenkins is very young and is not well aligned with BuildBot's strategy for opencv_extra PRs. We are working on that right now. Please ignore Jenkins status for now, only BuildBot pass is required.

…oder specific Exif parsing to JPEG and PNG decoders, respectively. Issue 16579

raaldrid · 2021-02-09T19:04:34Z

@asmorkalov Okay. Thank you for the clarification. I assume my PR is under code review, then. I made one final edit (tested previously) to switch from comparing #if PNG_LIBPNG_VER >= 1.6.31 to #ifdef PNG_eXIf_SUPPORTED (defined in pnglibconf.h). I think the latter is more self explanatory and better aligned with the intended use of libpng. From I can see both have the same end result.

If you have a suggestion for an issue for me to tackle next I would appreciate it. Otherwise I will dig around.

alalek

Well done! Thank you for great contribution 👍

modules/imgcodecs/test/test_png.cpp

raaldrid mentioned this pull request Feb 1, 2021

OpenCV does not respect exif orientation in PNG file #16579

Closed

asmorkalov added category: imgcodecs feature test labels Feb 2, 2021

alalek reviewed Feb 2, 2021

View reviewed changes

raaldrid force-pushed the Exif_support_for_PNG_issue_16579 branch 2 times, most recently from 51924b5 to ff104a9 Compare February 3, 2021 01:46

raaldrid force-pushed the Exif_support_for_PNG_issue_16579 branch 2 times, most recently from a0944da to 6ae58eb Compare February 4, 2021 04:13

raaldrid force-pushed the Exif_support_for_PNG_issue_16579 branch 4 times, most recently from 4af7460 to 98d3012 Compare February 5, 2021 19:43

raaldrid force-pushed the Exif_support_for_PNG_issue_16579 branch 2 times, most recently from f88ba27 to cf9381d Compare February 8, 2021 18:22

Exif parsing for PNG files to support Exif orientation tag. Moved dec…

650836d

…oder specific Exif parsing to JPEG and PNG decoders, respectively. Issue 16579

raaldrid force-pushed the Exif_support_for_PNG_issue_16579 branch from 45f175a to 650836d Compare February 9, 2021 18:36

raaldrid requested a review from alalek February 10, 2021 18:51

alalek approved these changes Feb 10, 2021

View reviewed changes

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

opencv-pushbot merged commit 93783df into opencv:3.4 Feb 10, 2021

alalek mentioned this pull request Feb 12, 2021

Merge 3.4 #19517

Merged

alalek mentioned this pull request Feb 16, 2021

imgcodecs(test): don't include png.h #19543

Merged

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

asmorkalov mentioned this pull request May 18, 2022

cv2.imread changes behaviour between 62 and 64 release opencv/opencv-python#662

Closed

Uh oh!

Conversation

raaldrid commented Feb 1, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

asmorkalov commented Feb 2, 2021

Uh oh!

alalek Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

raaldrid Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Feb 3, 2021

Uh oh!

raaldrid commented Feb 4, 2021

Uh oh!

alalek commented Feb 4, 2021

Uh oh!

raaldrid commented Feb 5, 2021

Uh oh!

asmorkalov commented Feb 7, 2021

Uh oh!

raaldrid commented Feb 8, 2021

Uh oh!

asmorkalov commented Feb 9, 2021

Uh oh!

raaldrid commented Feb 9, 2021

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

raaldrid commented Feb 1, 2021 •

edited by alalek

Loading