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.

Sign up for GitHub

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

Jump to bottom

Account for header bytes for Exif and XMP boxes #2234

Merged
merged 2 commits into from
May 19, 2022
Merged

Account for header bytes for Exif and XMP boxes #2234

merged 2 commits into from
May 19, 2022

Conversation

kmilos
Copy link
Collaborator

@kmilos kmilos commented May 11, 2022

Fixes #2233

1div0
1div0 reviewed May 11, 2022
View reviewed changes
Copy link
Collaborator

@1div0 1div0 left a comment

Choose a reason for hiding this comment

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

Looks good 2 M at the 1st sight. Will try l8r in the evening.

@kmilos
Copy link
Collaborator Author

kmilos commented May 11, 2022

Wonderful, this now breaks AVIF/HEIF tests 😠

Unfortunately I won't have time to look into this in more detail any time soon...

@kmilos kmilos force-pushed the fix_2233 branch 3 times, most recently from f69cbb9 to 27e9deb Compare May 11, 2022 13:53
@kmilos
Copy link
Collaborator Author

kmilos commented May 11, 2022

Ok, better with the approach changed to handle just the boxes in question.

P.S. No idea what's up w/ the archlinux jobs...

@codecov
Copy link

codecov bot commented May 11, 2022 •
edited
Loading

Codecov Report

Merging #2234 (4f9bed2) into main (1ff0950) will increase coverage by 0.00%.
The diff coverage is 64.70%.

@@           Coverage Diff           @@
##             main    #2234   +/-   ##
=======================================
  Coverage   63.42%   63.42%           
=======================================
  Files         117      117           
  Lines       19593    19592    -1     
  Branches     9551     9550    -1     
=======================================
  Hits        12426    12426           
  Misses       5098     5098           
+ Partials     2069     2068    -1
Impacted Files Coverage Δ
src/bmffimage.cpp 76.19% <64.70%> (+0.18%) ⬆️

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 1ff0950...4f9bed2. Read the comment docs.

@kmilos kmilos added bug samples imageHandler Anything related to specific ImageHandlers labels May 12, 2022
@kmilos kmilos requested review from 1div0 and hassec May 19, 2022 08:28
piponazo
piponazo approved these changes May 19, 2022
View reviewed changes
Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this.

I just made a small suggestion, feel free to implement it or ignore it.

Regarding the failing CI jobs on windows, I will try to retrigger them. It seems to be a network hiccup while downloading conan packages.

src/bmffimage.cpp
auto lengthSizeT = static_cast<size_t>(length);
DataBuf xmp(lengthSizeT + 1);
xmp.write_uint8(lengthSizeT, 0); // ensure xmp is null terminated!
if (io_->read(xmp.data(), lengthSizeT) != lengthSizeT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] I would suggest to use here BasicIo::readOrThrow() to replace lines 533-536. It is not a big deal, but it reduces some LoC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then it would be good address all the other instances of io_->read() in this file which I'm not inclined to address right now, so this minimal fix is hopefully ok for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, maybe it makes more sense to address such changes as a whole in a different branch 👍

@piponazo
Copy link
Collaborator

It seems that the problems with Conan are related to an upgrade in zlib:

  zlib.vcxproj -> D:\a\exiv2\exiv2\conanCache\zlib\1.2.11\_\_\build\5a61a86bb3e07ce4262c80e1510f9c05e9b6d48b\build_subfolder\lib\zlib.lib
  -- Install configuration: "Release"
  -- Installing: D:/a/exiv2/exiv2/conanCache/zlib/1.2.11/_/_/package/5a61a86bb3e07ce4262c80e1510f9c05e9b6d48b/lib/zlib.lib
  -- Installing: D:/a/exiv2/exiv2/conanCache/zlib/1.2.11/_/_/package/5a61a86bb3e07ce4262c80e1510f9c05e9b6d48b/include/zconf.h
  -- Installing: D:/a/exiv2/exiv2/conanCache/zlib/1.2.11/_/_/package/5a61a86bb3e07ce4262c80e1510f9c05e9b6d48b/include/zlib.h
WARN: Remotes registry file missing, creating default one in C:\Users\runneradmin\.conan\remotes.json
WARN: Could not download from the URL https://zlib.net/zlib-1.2.11.tar.gz: Not found: https://zlib.net/zlib-1.2.11.tar.gz. Trying another mirror.
ERROR: zlib/1.2.11: Error in package() method, line 127
	self._rename_libraries()

The tarball for zlib-1.2.11 is not available anymore in the webpage. I'll check how to deal with this situation in another branch.

@kmilos
Copy link
Collaborator Author

kmilos commented May 19, 2022

Ok to merge then?

@piponazo
Copy link
Collaborator

Ok to merge then?

Yes, I think I can deal with those issues in other moment.

@kmilos kmilos merged commit d4c7e11 into main May 19, 2022
@kmilos kmilos deleted the fix_2233 branch May 19, 2022 12:47
kmilos added a commit that referenced this pull request May 19, 2022
@kmilos
Account for header bytes for Exif and XMP boxes (backport #2234)
f9e3ca1
kmilos added a commit that referenced this pull request May 19, 2022
@kmilos
Account for header bytes for Exif and XMP boxes (backport #2234)
847a5e8
kmilos added a commit that referenced this pull request May 19, 2022
@kmilos
Merge pull request #2239 from Exiv2/backport_2234
d6e39d6 
Account for header bytes for Exif and XMP boxes (backport #2234)
antermin pushed a commit to antermin/exiv2 that referenced this pull request Mar 16, 2023
@kmilos @antermin
Account for header bytes for Exif and XMP boxes (backport Exiv2#2234)
eb4f0c4
@nehaljwani nehaljwani mentioned this pull request Apr 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piponazo piponazo piponazo approved these changes

@kevinbackhouse kevinbackhouse Awaiting requested review from kevinbackhouse

@1div0 1div0 Awaiting requested review from 1div0

@hassec hassec Awaiting requested review from hassec

Assignees
No one assigned
Labels
bug imageHandler Anything related to specific ImageHandlers samples
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem reading metadata from a JPEG XL file
3 participants
@kmilos @piponazo @1div0