Add support for LZ4 compressed elevation tiles. #3401

lseelenbinder · 2021-11-13T11:58:56Z

~~This is not fully baked, but since I'm by no means a CMake or C++ expert, I wanted to ensure this was a reasonable start.~~

~~I'll still need to add tests and some additional documentation.~~

According to the tests, it does what it says on the cover. 😄

Issue

Adds drop-in support for lz4 elevation tiles. Briefly discussed here: #3071 (comment)

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog
If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

kevinkreiser · 2021-11-13T17:44:27Z

We'll want to add some unit tests just to exorcise the code paths, you can find some here for gzip, a similar approach can be used to check that lz4 is working properly: https://github.com/valhalla/valhalla/blob/master/test/sample.cc

kevinkreiser · 2021-11-13T17:47:54Z

src/skadi/sample.cc


 class cache_item_t {
 private:
  format_t format;
  valhalla::midgard::mem_map<char> data;
  int usages;
-  const char* unpacked;


this is unfortunate but i assume its an artifact of LZ4F_decompress taking a non const parameter

the good news is you dont need all of these removals of const because when we create the bytes where we unpack the data, we create it as non-const. so we can const cast it back before you make that call. ill make a suggestion below and then we can remove a lot of the changes here

Ah, great! I cleaned it all up as suggested.

kevinkreiser · 2021-11-13T17:53:43Z

src/skadi/sample.cc

+      size_t dest_size = HGT_BYTES;
+      size_t result;
+      do {
+        result = LZ4F_decompress(decode, this->unpacked, &dest_size, data.get(), &src_size, &options);


we can revert all the changes you made to remove constness from this and just cast the const away at the time when we need to do that. this is ok because when we create unpacked we do so without const.

Suggested change

result = LZ4F_decompress(decode, this->unpacked, &dest_size, data.get(), &src_size, &options);

result = LZ4F_decompress(decode, const_cast<char *>(this->unpacked), &dest_size, data.get(), &src_size, &options);

kevinkreiser · 2021-11-13T17:55:46Z

src/skadi/sample.cc

@@ -353,6 +390,7 @@ tile_data cache_t::source(uint16_t index) {
      }
    }
  }
+
  if (!unpacked) {
    unpacked = (char*)malloc(HGT_BYTES);


^^ this is why we can use const_cast, because here unpacked is not actually const so casting it away will not be undefined behavior

kevinkreiser · 2021-11-13T17:56:20Z

@lseelenbinder this looks generally good, i think you can revert the changes to constness and add a unit test and we can get it reviewed. thanks!

lseelenbinder · 2021-11-13T18:33:43Z

@kevinkreiser I'm having difficulty tracking down where the test data files are for elevation. I found the test, but test/data/(sample|samplegz) don't exist on my branch so the current tests fail, as well? Is there some automated process for generating them, or should I download these from somewhere?

kevinkreiser · 2021-11-18T00:30:21Z

@lseelenbinder its kind of tricky but we create this header file called test/pixels.h which has the binary data of a real tile (though not all of it) as an array of pixel values. we include this header with the vector and then in the test we turn it into elevation tiles: https://github.com/valhalla/valhalla/blob/master/test/sample.cc#L30-L70 then the actual tests check those.

so you'll want to lz4 compress that data in that same function and write it to another location. then you can add a unit test that checks that it loads ok like the other tests do: https://github.com/valhalla/valhalla/blob/master/test/sample.cc#L102-L104

lseelenbinder · 2021-11-28T16:41:51Z

Hey @kevinkreiser, I think this is ready for a final review now. Thanks for your help finishing it up!

kevinkreiser · 2021-11-30T01:56:57Z

src/skadi/sample.cc

+      LZ4F_decompressionContext_t decode;
+      LZ4F_decompressOptions_t options;
+      LZ4F_createDecompressionContext(&decode, LZ4F_VERSION);


can you refresh my memory, does this also support LZ4_HC or other variants? i vaguely remember there were 3 formats, one was roughly normal but not fixed endian, another was fixed endian (probably network order) and one was "high compression". does this support all of them automatically or just one of them, and if the latter, which one? it was 3 years ago when i implemented it and i think i ended up going with the first one becuase its what the command line tool spits out maybe the others were containerless and more for streaming situations? sorry i just cant remember

No worries, @kevinkreiser.

I just went with whatever the command line tool spit out. There's other formats that seem to be more for streaming, but it seemed the most logical target was the command line results. I made a few mistakes on the decompression side using lz4.h functions instead of lz4_frame.h functions, so it appears at least the lz4 command line outputs a framed format?

Should we just state we support files compressed with lz4 command line in the release notes? The command line doesn't seem to offer any options for this (https://linuxcommandlibrary.com/man/lz4).

nope thats all good. i just wanted to confirm my recollection. ill review this tonight hopefully when i have a free moment. thanks for the efforts!

kevinkreiser · 2021-12-06T15:33:01Z

this looks good to me but you need to resolve the conflicts, i would do it for you but you have that setting turned off

kevinkreiser · 2021-12-06T15:34:22Z

src/skadi/sample.cc

@@ -334,7 +376,7 @@ tile_data cache_t::source(uint16_t index) {
  }

  // item in cache is already unpacked
-  const char* unpacked = item.get_unpacked();


please revert this change, or use const auto *. auto is great and all but when we use it to hide qualifiers it makes everyones lives harder

Whoops, that was an oversight. Good catch!

lseelenbinder · 2021-12-06T17:21:06Z

Thanks again @kevinkreiser. Assuming tests pass, this should be good to go!

…ng) test.

* Add rudimentary support for LZ4 compressed elevation tiles. * Tidy up code. * Revert unnecessary const removals. Update CHANGELOG.md and add (failing) test. * Add missing free for LZ4 context. * Add attempt at compressing LZ4 elevation tiles. * Add working lz4 elevation test. * Cleanup formatting and pin lz4 to 1.9.3. * Revert unnecessary `auto` usage.

lseelenbinder force-pushed the add-lz4-elevation-support branch from e241aed to f0eb0f0 Compare November 13, 2021 12:00

lseelenbinder marked this pull request as draft November 13, 2021 12:01

kevinkreiser self-requested a review November 13, 2021 17:44

kevinkreiser reviewed Nov 13, 2021

View reviewed changes

lseelenbinder marked this pull request as ready for review November 26, 2021 23:12

lseelenbinder changed the title ~~Draft: Add rudimentary support for LZ4 compressed elevation tiles.~~ Add rudimentary support for LZ4 compressed elevation tiles. Nov 26, 2021

lseelenbinder changed the title ~~Add rudimentary support for LZ4 compressed elevation tiles.~~ Add support for LZ4 compressed elevation tiles. Nov 26, 2021

kevinkreiser reviewed Nov 30, 2021

View reviewed changes

kevinkreiser previously approved these changes Dec 6, 2021

View reviewed changes

kevinkreiser reviewed Dec 6, 2021

View reviewed changes

lseelenbinder dismissed kevinkreiser’s stale review via 63cae4c December 6, 2021 17:18

kevinkreiser previously approved these changes Dec 6, 2021

View reviewed changes

lseelenbinder added 8 commits December 7, 2021 12:01

Add rudimentary support for LZ4 compressed elevation tiles.

85dc8fc

Tidy up code.

41db798

Revert unnecessary const removals. Update CHANGELOG.md and add (faili…

671ab5f

…ng) test.

Add missing free for LZ4 context.

2bbe8cf

Add attempt at compressing LZ4 elevation tiles.

2de6f0b

Add working lz4 elevation test.

0760bc1

Cleanup formatting and pin lz4 to 1.9.3.

055dae1

Revert unnecessary auto usage.

609935b

lseelenbinder dismissed kevinkreiser’s stale review via 609935b December 7, 2021 11:02

lseelenbinder force-pushed the add-lz4-elevation-support branch from 148a45b to 609935b Compare December 7, 2021 11:02

kevinkreiser approved these changes Dec 7, 2021

View reviewed changes

kevinkreiser merged commit b69f201 into valhalla:master Dec 7, 2021

lseelenbinder deleted the add-lz4-elevation-support branch December 7, 2021 14:11

This was referenced Apr 30, 2022

Add support for LZ4 recompression in valhalla_build_elevation #3606

Closed

Enhance valhalla_build_elevation with LZ4 recompression support #3607

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for LZ4 compressed elevation tiles. #3401

Add support for LZ4 compressed elevation tiles. #3401

lseelenbinder commented Nov 13, 2021 •

edited

Loading

kevinkreiser commented Nov 13, 2021

kevinkreiser Nov 13, 2021

kevinkreiser Nov 13, 2021

lseelenbinder Nov 13, 2021

kevinkreiser Nov 13, 2021

kevinkreiser Nov 13, 2021

kevinkreiser commented Nov 13, 2021

lseelenbinder commented Nov 13, 2021 •

edited

Loading

kevinkreiser commented Nov 18, 2021

lseelenbinder commented Nov 28, 2021

kevinkreiser Nov 30, 2021

lseelenbinder Nov 30, 2021

kevinkreiser Dec 1, 2021

kevinkreiser commented Dec 6, 2021 •

edited

Loading

kevinkreiser Dec 6, 2021

lseelenbinder Dec 6, 2021

lseelenbinder commented Dec 6, 2021

	result = LZ4F_decompress(decode, this->unpacked, &dest_size, data.get(), &src_size, &options);
	result = LZ4F_decompress(decode, const_cast<char *>(this->unpacked), &dest_size, data.get(), &src_size, &options);

Add support for LZ4 compressed elevation tiles. #3401

Add support for LZ4 compressed elevation tiles. #3401

Conversation

lseelenbinder commented Nov 13, 2021 • edited Loading

Issue

Tasklist

Requirements / Relations

kevinkreiser commented Nov 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Nov 13, 2021

lseelenbinder commented Nov 13, 2021 • edited Loading

kevinkreiser commented Nov 18, 2021

lseelenbinder commented Nov 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lseelenbinder commented Dec 6, 2021

lseelenbinder commented Nov 13, 2021 •

edited

Loading

lseelenbinder commented Nov 13, 2021 •

edited

Loading

kevinkreiser commented Dec 6, 2021 •

edited

Loading