Use ZSTD's streaming interface for compressing files #543
Conversation
Distro A, OPSEC #4584 Signed-off-by: P. J. Reed <[email protected]>
Signed-off-by: P. J. Reed <[email protected]>
pjreed
force-pushed
the
295/use-streaming-compression
branch
from
e4c99b2 to
2491207
Compare
|
Seems like a test failed after merging in the latest master, I'll investigate... |
|
I'm actually not sure why that test failed -- I don't think this change would have affected it -- and everything passes for me in my environment. @emersonknapp , any ideas? |
|
The output suggests that the failing test is happening with CycloneDDS - are you also using cyclone locally, or running with FastDDS? Edit: I just tried out the test locally and I happened to be using fastrtps, but when using cyclone it fails - looking in to it |
|
BTW if you update your branch - the test problem should be resolved - let me know when you feel ready for a review here |
There was a problem hiding this comment.
Thanks for your patience on this - the code looks fine. I'd like it if some of the common setup and shutdown could be deduplicated, perhaps as some utility function that sets up the ifstream/ofstream for you. But that's minor, I won't block the review on that, pursue it if you like.
I think you're fine to take this out of draft (and it will need a rebase) - and let's make sure the tests are passing, then we can go ahead and merge.
|
I would also like to see the code around the compressor/decompressor setup/shutdown look a little cleaner, but I've been staring at it a while and they're just different enough that I haven't thought of a good way to do so. Anyway, I've merged master into this and all of the tests pass for me; I was previously using FastRTPS, but switched to CycloneDDS, and everything still builds & runs fine for me. Not sure why the CI build would have a problem with it... |
The important one works - the |
rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
Outdated
Show resolved
Hide resolved
rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
Outdated
Show resolved
Hide resolved
…sion_writer.cpp Co-authored-by: Emerson Knapp <[email protected]> Signed-off-by: P. J. Reed <[email protected]>
…sion_writer.cpp Co-authored-by: Emerson Knapp <[email protected]> Signed-off-by: P. J. Reed <[email protected]>
pjreed
force-pushed
the
295/use-streaming-compression
branch
from
adb30b9 to
1d04628
Compare
|
Tests still build & run for me without any failures; is there a way to make just that one run again? |
|
Gist: https://gist.githubusercontent.com/emersonknapp/6320545b1692e39a982c128f44c559bc/raw/6eb426aa03b3283fa5051a2398d609df9a203d0c/ros2.repos
|
* Implement streaming compression/decompression Distro A, OPSEC #4584 Signed-off-by: P. J. Reed <[email protected]>
* Implement streaming compression/decompression Distro A, OPSEC #4584 Signed-off-by: P. J. Reed <[email protected]>
This uses ZSTD's streaming compression interface when compressing files, which will significantly reduce memory usage since it no longer needs to read in the entire file at once.
One significant caveat to note about this is that the previous version of the code had a check to verify that the size of the decompressed output matched the size recorded in the compression header for the file. ZSTD's streaming interface does not record the decompressed size in its frame header (it cannot, since it does not know the size ahead of time); this means that compressed bag files recorded with the streaming interface cannot be played by older versions of rosbag2 since they will be missing that bit of information in their headers, and that will throw an exception.
Fixes #295.
Distribution Statement A; OPSEC #4584