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

Implement the xz Delta filter (filter 3) #101

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

terrence2
Copy link

@terrence2 terrence2 commented Jun 2, 2023

Pull Request Overview

This pull request adds a delta compression filter and fixes multi-filter decompression; this appears to be the first test case making use of multiple filters.

Terrain height data tends to have lots of periodic repeating patterns, so GeoTiffs compressed with GDAL's liblzma support tend to have lots of tiles that make use of delta filtering. I need a native rust implementation of lzma decompression for use in OpenFA, so that I can port it to run on the web.

Testing Strategy

This pull request was tested by...

  • Added relevant end-to-end tests (an .xz file with a delta filter).
  • Integrated into OpenFA's GeoTiff parser and demonstrated equivalent function to liblzma.
  • Ran cargo bench to verify there were no performance regressions.

Supporting Documentation and References

Code for the delta filter was adapted from the C code in xz-file-format.txt.

Copy link
Author

@terrence2 terrence2 left a comment

Choose a reason for hiding this comment

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

Adding some notes inline to aid review:

src/decode/delta.rs Outdated Show resolved Hide resolved
src/decode/xz.rs Outdated Show resolved Hide resolved
src/decode/xz.rs Outdated Show resolved Hide resolved
src/decode/xz.rs Show resolved Hide resolved
Copy link
Owner

@gendx gendx left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, and apologies for the delay as I was caught up in many other things this year. Overall this looks good, I left a few comments.

src/decode/delta.rs Outdated Show resolved Hide resolved
src/decode/delta.rs Outdated Show resolved Hide resolved
src/decode/xz.rs Outdated Show resolved Hide resolved
src/decode/xz.rs Outdated Show resolved Hide resolved
src/decode/xz.rs Show resolved Hide resolved
tests/files/delta-filter-3.dat.xz Outdated Show resolved Hide resolved
@gendx gendx added the waiting on author Waiting for pull request author to address review comments label Nov 8, 2023
@terrence2
Copy link
Author

Great feedback! I think the new diff is much cleaner and clearer than the previous.

Since I was touching decomp_big_file anyway, I took the liberty of replacing read_all_file with the new fs::read builtin while I was there.

Please give it another look, at your convenience.

@terrence2 terrence2 requested a review from gendx November 11, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on author Waiting for pull request author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants