CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Added Exif parsing for PNG Issue 16579 #19439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Exif parsing for PNG Issue 16579 #19439
Conversation
@rrrapha Thanks for the contribution! Please pay attention on CI builds. It looks like your patch breaks build:
|
modules/imgcodecs/src/grfmt_base.hpp
Outdated
@@ -64,6 +65,7 @@ class BaseImageDecoder | |||
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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExifReader
reference: ExifReader&
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
51924b5
to
ff104a9
Compare
@raaldrid Build is still broken:
|
a0944da
to
6ae58eb
Compare
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):
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:
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: |
This is a system dependency. It can't be upgraded. You should add version check through |
4af7460
to
98d3012
Compare
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. |
jenkins cn please retry a build |
f88ba27
to
cf9381d
Compare
@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.) |
@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
45f175a
to
650836d
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! Thank you for great contribution 👍
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
Patch to opencv_extra has the same branch name.