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 #2151

Closed
wants to merge 14 commits into from
Closed

Conversation

piponazo
Copy link
Collaborator

@piponazo piponazo commented Mar 15, 2022

This PR fix #2147

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.

@cgilles I would appreciate if you could double check if this branch fixes your issues.

@piponazo piponazo added bug enhancement feature / functionality enhancements labels Mar 15, 2022
@piponazo piponazo self-assigned this Mar 15, 2022
@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #2151 (3117967) into main (b2b6d83) will increase coverage by 0.00%.
The diff coverage is 75.60%.

@@           Coverage Diff           @@
##             main    #2151   +/-   ##
=======================================
  Coverage   63.25%   63.26%           
=======================================
  Files          97       98    +1     
  Lines       19154    19200   +46     
  Branches     9722     9743   +21     
=======================================
+ Hits        12116    12146   +30     
- Misses       4779     4796   +17     
+ Partials     2259     2258    -1     
Impacted Files Coverage Δ
app/actions.cpp 64.25% <ø> (+0.29%) ⬆️
include/exiv2/image.hpp 100.00% <ø> (ø)
src/jpgimage.cpp 70.83% <40.00%> (-0.06%) ⬇️
src/jp2image.cpp 69.03% <74.90%> (+0.67%) ⬆️
src/image.cpp 70.44% <100.00%> (ø)
src/jp2image_int.cpp 100.00% <100.00%> (ø)
src/pngimage.cpp 60.05% <100.00%> (ø)
src/tiffimage.cpp 70.54% <0.00%> (-0.78%) ⬇️
src/tiffcomposite_int.cpp 73.39% <0.00%> (-0.52%) ⬇️
src/basicio.cpp 49.63% <0.00%> (-0.49%) ⬇️
... and 3 more

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 b2b6d83...3117967. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Mar 15, 2022

This pull request fixes 2 alerts when merging bab0de5 into b2b6d83 - view on LGTM.com

fixed alerts:

  • 2 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Mar 15, 2022

This pull request fixes 2 alerts when merging 3117967 into b2b6d83 - view on LGTM.com

fixed alerts:

  • 2 for FIXME comment

0x00, 0x14, 0x00, 0x40, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0xff, 0x93, 0xcf, 0xb4, 0x04, 0x00, 0x80, 0x80, 0x80, 0x80, 0x80, 0xff, 0xd9};

struct Jp2BoxHeader
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might make sense to use std::pair here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide an example of what you have in mind?

@cgilles
Copy link
Collaborator

cgilles commented Mar 16, 2022

Hi Luis,

I will compile your branch and check.

Note : i don't receive a notification from github. Strange...

Gilles

@cgilles
Copy link
Collaborator

cgilles commented Mar 16, 2022

Luis,

I can confirm that digiKam recompiled with Exiv2 from your branch "mainFixJp2" do not corrupt JP2000 file when exported from JPEG in Image Editor:

https://i.imgur.com/FF3iMeN.png

Best

Gilles

@cgilles
Copy link
Collaborator

cgilles commented Mar 16, 2022

Luis, did you back-port your JPEG2000 fix to the 0.27-maintenance branch ?

@piponazo
Copy link
Collaborator Author

@cgilles great, thanks for double checking. I did not port yet these changes to 0.27-maintenance but I will do it.

@cgilles
Copy link
Collaborator

cgilles commented Mar 16, 2022

Thanks Luis for the feedback.

The digiKam version that i use to test your branch is the future 8.0.0 where code is open to new implementations. This version is in alpha stage. I patched the Exiv2 interface in digiKam to compile with Exiv2 1.0.0 API.
The digiKam stable version 7.x is maintained in a dedicated branch and this code do not compile with Exiv2 1.0.0. It uses 0.27-maintenance API.
If you back-port your branch to 0.27-maintenance, i will not back-port my Exiv2 Interface changes from digiKam 8 to 7. It will be safe as changes introduced in digiKam 8 are important.

Best

piponazo added a commit that referenced this pull request Mar 16, 2022
This has been backported from the work done in 'main' (#2151)
@piponazo piponazo added this to the v1.00 milestone Mar 16, 2022
piponazo added a commit that referenced this pull request Mar 16, 2022
This has been backported from the work done in 'main' (#2151)
piponazo added a commit that referenced this pull request Mar 16, 2022
This has been backported from the work done in 'main' (#2151)
@piponazo
Copy link
Collaborator Author

I am closing this PR because it was too painful to deal with the clanf-format changes here. New version of the PR is #2155

@piponazo piponazo closed this Mar 18, 2022
@neheb neheb deleted the mainFixJp2 branch March 18, 2022 18:58
antermin pushed a commit to antermin/exiv2 that referenced this pull request Mar 16, 2023
This has been backported from the work done in 'main' (Exiv2#2151)
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)
3 participants