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

1/4 TIFF processing simplification by 'shifting' base of random access reader #581

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

don-vip
Copy link

@don-vip don-vip commented Jun 14, 2022

This is the first step towards solving #278

Port of this commit: drewnoakes/metadata-extractor-dotnet@2281cea

@don-vip
Copy link
Author

don-vip commented Jun 14, 2022

(not ready yet, I got test failures to fix)

@drewnoakes
Copy link
Owner

Looking good so far. The best way to validate correctness is to clone https://github.com/drewnoakes/metadata-extractor-images and run your updated code against all those files to check for regressions. It's not a process I've documented, sorry, so I can do that for you if you like, but doing it yourself would give you a tighter feedback loop.

@don-vip
Copy link
Author

don-vip commented Jun 15, 2022

@drewnoakes thank you! I'll do it :) And probably create a PR to improve the documentation as well :)

@drewnoakes
Copy link
Owner

drewnoakes commented Jun 16, 2022

Here's an overview of the process.

  • There is a .NET program here: https://github.com/drewnoakes/metadata-extractor-images/tree/master/src/dotnet
  • It assumes you have sibling repositories for metadata-extractor, metadata-extractor-dotnet and metadata-extractor-images
  • That .NET solution references the source from metadata-extractor-dotnet (here), so it'll build the .NET code as part of its build. You must currently manually build the Java code separately however.
  • Running that .NET application will update all the metadata/*.txt files, and compute updated diffs.

@don-vip don-vip force-pushed the simplify-tiff-shifting branch 5 times, most recently from 1502c65 to 0d4d9f9 Compare June 18, 2022 10:20
… than passing TIFF header offsets around everywhere.
@don-vip don-vip marked this pull request as ready for review June 18, 2022 11:50
@don-vip
Copy link
Author

don-vip commented Jun 18, 2022

thank you! now the PR is ready for review :)

@don-vip don-vip changed the title Port TIFF processing simplification by 'shifting' base of indexed reader from .NET 1/4 TIFF processing simplification by 'shifting' base of random access reader Jun 19, 2022
Copy link
Owner

@drewnoakes drewnoakes 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! Thanks very much for these nicely structured PRs. It makes them very easy to review, which I appreciate.

I'm adding some comments with initial thoughts to your PRs. Don't feel you have to take any action on them, especially as changes to the first PR require rebasing other PRs etc. I'm going to pull down all your changes locally and try them out. I can make small tweaks in an additional commit at the end if needed. The comments on your PRs are just my notes and thoughts rather than requests of you.

{
assert(segmentType == JpegSegmentType.APP1);

for (byte[] segmentBytes : segments) {
// Segment must have the expected preamble
if (startsWithJpegExifPreamble(segmentBytes)) {
extract(new ByteArrayReader(segmentBytes), metadata, JPEG_SEGMENT_PREAMBLE.length());
extract(new ByteArrayReader(segmentBytes).withShiftedBaseOffset(JPEG_SEGMENT_PREAMBLE.length()), metadata);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
extract(new ByteArrayReader(segmentBytes).withShiftedBaseOffset(JPEG_SEGMENT_PREAMBLE.length()), metadata);
extract(new ByteArrayReader(segmentBytes, JPEG_SEGMENT_PREAMBLE.length()), metadata);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants