Skip to content
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

Fix in Jp2 metadata writing & improvements in reading (2) #2155

Merged
merged 5 commits into from
Mar 23, 2022
Merged

Conversation

piponazo
Copy link
Collaborator

This PR fix #2147 ( This is a rework of #2151. I wanted to squash some of the commits and also to deal with the recent clang-format changes introduced in main).

After repeating the steps described by @cgilles I could easily reproduce the issue. Even by just creating a JP2 from scratch and then trying to read the metadata, the problem was reproducible (see unit test Jp2Image.canWriteMetadataAndReadAfterwards).

The main issue was present at Jp2Image::encodeJp2Header when encoding the newlen of the Colour Specification Box. At that point we were indicating that the box was a bit bigger than it was in reality and the method Jp2Image::readMetadata would detect that afterwads.

I also did some improvements in Jp2Image::readMetadata and Jp2Image::printStructure so that we perform more checks as described in the JP2 standard. Due to some of these changes, I had to adapt some of the python tests.

I will probably spend some more time in the future to make more improvements in the Jp2 support but I will do that in other branches.

- add debug info when parsing Signature box
- Move definitions & static stuff to anonymous namespace
- cleanup while studying code
- Make exceptions more similar to other formats
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #2155 (1ad9e52) into main (5d5354e) will increase coverage by 0.06%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##             main    #2155      +/-   ##
==========================================
+ Coverage   62.65%   62.72%   +0.06%     
==========================================
  Files          97       98       +1     
  Lines       19721    19756      +35     
  Branches     9710     9722      +12     
==========================================
+ Hits        12357    12391      +34     
- Misses       5132     5133       +1     
  Partials     2232     2232              
Impacted Files Coverage Δ
src/jp2image.cpp 69.58% <82.73%> (+2.68%) ⬆️
src/jp2image_int.cpp 100.00% <100.00%> (ø)
src/tiffimage.cpp 69.62% <0.00%> (-0.75%) ⬇️
src/tiffcomposite_int.cpp 73.75% <0.00%> (-0.52%) ⬇️
src/tiffvisitor_int.cpp 76.33% <0.00%> (-0.34%) ⬇️
app/actions.cpp 63.62% <0.00%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d5354e...1ad9e52. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Mar 18, 2022

This pull request fixes 2 alerts when merging 1ad9e52 into 5d5354e - view on LGTM.com

fixed alerts:

  • 2 for FIXME comment

@kmilos
Copy link
Collaborator

kmilos commented Mar 18, 2022

This doesn't impact the 0.27 backport, right?

I will probably spend some more time in the future to make more improvements in the Jp2 support but I will do that in other branches.

I remember someone having the grand idea of merging jp2 and bmff image handlers at some point, that would be something ;) (w/ write support of course)

@piponazo
Copy link
Collaborator Author

This doesn't impact the 0.27 backport, right?

No, the changes are the same. I do not need to re-do the work on 0.27 because we did not do the whole project clang-formatting there.

I will keep in mind the possible merging of jp2 & BMPFF but I need to learn much more to address such task 😉

@piponazo piponazo self-assigned this Mar 20, 2022
@piponazo piponazo added bug enhancement feature / functionality enhancements labels Mar 20, 2022
@piponazo piponazo merged commit 5ed9fb4 into main Mar 23, 2022
@piponazo
Copy link
Collaborator Author

I used my admin powers to merge this PR without approval after 1 week without any feedback.

@piponazo piponazo deleted the mainFixJp2_2 branch March 23, 2022 15:47
@kmilos
Copy link
Collaborator

kmilos commented Mar 23, 2022

Sorry, I meant to approve after you confirmed no more changes are needed! 🤦

@piponazo
Copy link
Collaborator Author

No worries, all of us have exiv2 as a side project. I just have some more spare time lately and I wanted to move on 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement feature / functionality enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metacopy CLI tool corrupt JPEG2000 (regression with Exiv2 >= 0.27.5)
2 participants