[Compression - 6] Add Zstd file decompression implementation #230

piraka9011 · 2019-12-10T23:12:11Z

Changes

Add zstd_decompressor header and source
Add a new unit test for decompression
- Compress a file then decompresses it using the same format. Make sure original size is the same.

Dependencies

PR [Compression - 5] Add Zstd file compression #220.
Rebased after [Compression - 5] Add Zstd file compression #220 is merged.

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp

zmichaels11 · 2019-12-16T22:11:00Z

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/8caa09c53ea15fc9b854eafe8eccc589/raw/f8650ffdf23ddc9842120ed4479a0c8462cf9749/ros2.repos
BUILD args: --packages-up-to rosbag2_compression
TEST args: --packages-select rosbag2_compression
Job: ci_launcher

piraka9011 · 2019-12-16T23:06:40Z

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 · 2019-12-19T17:29:06Z

@Karsten1987 this PR is ready for review

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

rosbag2_compression/include/rosbag2_compression/zstd_decompressor.hpp

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

…fy it fails with bad input. Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

* Add comments on why uniform initialization cannot be used Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

rosbag2_compression/include/rosbag2_compression/zstd_decompressor.hpp

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 · 2020-01-03T00:01:11Z

@Karsten1987 friendly ping requesting review

Karsten1987

With compression and decompression being available with this, I would like to see a test which checks the validity of the compression. That is, a file being compressed and decompressed is byte equivalent with the initial file content.

Karsten1987 · 2020-01-03T17:23:09Z

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

+  // Read in buffer, handling accordingly
+  const auto file_pointer = open_file(uri.c_str(), "rb");
+  if (file_pointer == nullptr) {
+    std::stringstream errmsg;
+    errmsg << "Failed to open file: \"" << uri << "\" for binary reading!";
+
+    throw std::runtime_error{errmsg.str()};
+  }


Could this check be done before getting the size of the uri?

Karsten1987 · 2020-01-03T17:23:50Z

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

+std::vector<uint8_t> get_input_buffer(const std::string & uri)
+{
+  // Get the file size
+  const auto compressed_buffer_length = rosbag2_storage::FilesystemHelper::get_file_size(uri);


I think we should react here in case the returned filesize is 0 (aka the file is non-existing)

Karsten1987 · 2020-01-03T17:24:47Z

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

+  // the initializer list constructor instead.
+  std::vector<uint8_t> compressed_buffer(compressed_buffer_length);
+
+  const auto nRead = fread(


nit: generally we don't have camelcase variable names in ROS2.

Karsten1987 · 2020-01-03T17:27:40Z

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

+    ROSBAG2_COMPRESSION_LOG_ERROR_STREAM("Bytes read !(" <<
+      nRead << ") != compressed_buffer_length (" << compressed_buffer_length <<
+      ")!");
+    // An error indicator is set by fread, so the following check will throw.


This comment actually refers to line 90, right?

Yeah, the error log message on L84 exists to provide further information on an error from fread.

The comment on L87 is indicating that the code should throw at this state and that the check on L90 does the actual throw since fread may have set the error indicator for other reasons besides nRead != compressed_buffer_length.

Karsten1987 · 2020-01-03T17:29:29Z

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

+  }
+
+  const auto nWrite = fwrite(
+    output_buffer.data(), sizeof(uint8_t),


what if output buffer is empty?

Karsten1987 · 2020-01-03T17:32:12Z

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp

+/**
+ * Checks if the frame content is valid.
+ */
+void check_frame_content(const size_t frame_content)


given that we have throw_on_zstd_error I think this function could be renamed accordingly as it really does only throwing on error.

zmichaels11 · 2020-01-03T17:44:19Z

With compression and decompression being available with this, I would like to see a test which checks the validity of the compression. That is, a file being compressed and decompressed is byte equivalent with the initial file content.

There's a unit test for this:

rosbag2/rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp

Line 130 in 9e18969

TEST_F(CompressionHelperFixture, zstd_decompress_file_contents)

zmichaels11 · 2020-01-06T16:57:15Z

@Karsten1987 I'm opening a PR to address your comments: #245

zmichaels11 force-pushed the compression/zstd-decompressor branch 3 times, most recently from 95f6c4e to 04698fb Compare December 13, 2019 17:18

piraka9011 commented Dec 13, 2019

View reviewed changes

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Show resolved Hide resolved

piraka9011 commented Dec 13, 2019

View reviewed changes

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Outdated Show resolved Hide resolved

Karsten1987 mentioned this pull request Dec 13, 2019

github actions fail #235

Closed

zmichaels11 requested a review from Karsten1987 December 16, 2019 16:13

zmichaels11 force-pushed the compression/zstd-decompressor branch from 61ec2f2 to db10182 Compare December 16, 2019 19:24

Anas Abou Allaban and others added 4 commits December 18, 2019 07:58

Add Zstd decompressor with tests

fe898ec

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

Test better for decompression

4313c32

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Follow conventions used in compression for decompression

e4b6af0

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Apply suggestions

e989b8b

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 force-pushed the compression/zstd-decompressor branch from db10182 to e989b8b Compare December 18, 2019 15:59

zmichaels11 mentioned this pull request Dec 19, 2019

[DO NOT MERGE] Compression Metadata changes aws-ros-dev/rosbag2#10

Closed

zmichaels11 requested review from dabonnie, emersonknapp, mm318 and prajakta-gokhale December 23, 2019 19:15

mm318 suggested changes Dec 30, 2019

View reviewed changes

dabonnie reviewed Dec 30, 2019

View reviewed changes

rosbag2_compression/include/rosbag2_compression/zstd_decompressor.hpp Show resolved Hide resolved

dabonnie reviewed Dec 30, 2019

View reviewed changes

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Outdated Show resolved Hide resolved

mm318 reviewed Dec 30, 2019

View reviewed changes

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Show resolved Hide resolved

dabonnie reviewed Dec 30, 2019

View reviewed changes

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Show resolved Hide resolved

dabonnie reviewed Dec 30, 2019

View reviewed changes

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Show resolved Hide resolved

dabonnie reviewed Dec 30, 2019

View reviewed changes

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Show resolved Hide resolved

zmichaels11 added 2 commits December 30, 2019 21:07

Improve documentation on ZstdDecompressor

f66fc94

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Use Vector resize instead of reserve

702ed23

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 added 2 commits December 30, 2019 22:10

Add unit test to verify decompression works and add unit test to veri…

34dada4

…fy it fails with bad input. Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Add unit test to check that ZstdDecompressor fails if file uri is bad.

1b601b4

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 requested review from dabonnie and mm318 December 31, 2019 06:23

mm318 reviewed Dec 31, 2019

View reviewed changes

rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Outdated Show resolved Hide resolved

zmichaels11 added 2 commits December 31, 2019 09:41

Use sizeof(decltype(vector)::value_type) when possible

5eb5718

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

Use vector constructor with initial size instead of calling resize

fdbdbfd

* Add comments on why uniform initialization cannot be used Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 requested a review from mm318 December 31, 2019 18:08

mm318 reviewed Dec 31, 2019

View reviewed changes

mm318 approved these changes Dec 31, 2019

View reviewed changes

Use sizeof(decltype(vec)) and pass vector

250bab3

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 force-pushed the compression/zstd-decompressor branch from a5eee19 to 250bab3 Compare December 31, 2019 20:55

dabonnie reviewed Dec 31, 2019

View reviewed changes

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Show resolved Hide resolved

dabonnie approved these changes Dec 31, 2019

View reviewed changes

Include example of debug log in comments

8859388

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 approved these changes Dec 31, 2019

View reviewed changes

piraka9011 commented Jan 2, 2020

View reviewed changes

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Outdated Show resolved Hide resolved

dabonnie reviewed Jan 2, 2020

View reviewed changes

rosbag2_compression/src/rosbag2_compression/zstd_decompressor.cpp Outdated Show resolved Hide resolved

Fix log message for decompression statistics

050d8a4

Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

zmichaels11 force-pushed the compression/zstd-decompressor branch from 4f46b75 to 050d8a4 Compare January 2, 2020 22:26

zmichaels11 mentioned this pull request Jan 2, 2020

[Compression] Use vector resize instead of reserve #243

Merged

zmichaels11 merged commit 9e18969 into ros2:master Jan 3, 2020

Karsten1987 reviewed Jan 3, 2020

View reviewed changes

zmichaels11 deleted the compression/zstd-decompressor branch January 3, 2020 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Compression - 6] Add Zstd file decompression implementation #230

[Compression - 6] Add Zstd file decompression implementation #230

piraka9011 commented Dec 10, 2019 •

edited by zmichaels11

Loading

zmichaels11 commented Dec 16, 2019

piraka9011 commented Dec 16, 2019 •

edited

Loading

zmichaels11 commented Dec 19, 2019

zmichaels11 commented Jan 3, 2020

Karsten1987 left a comment

Karsten1987 Jan 3, 2020

Karsten1987 Jan 3, 2020

Karsten1987 Jan 3, 2020

Karsten1987 Jan 3, 2020

zmichaels11 Jan 3, 2020

Karsten1987 Jan 3, 2020

Karsten1987 Jan 3, 2020

zmichaels11 commented Jan 3, 2020

zmichaels11 commented Jan 6, 2020

[Compression - 6] Add Zstd file decompression implementation #230

[Compression - 6] Add Zstd file decompression implementation #230

Conversation

piraka9011 commented Dec 10, 2019 • edited by zmichaels11 Loading

Changes

Dependencies

zmichaels11 commented Dec 16, 2019

piraka9011 commented Dec 16, 2019 • edited Loading

zmichaels11 commented Dec 19, 2019

zmichaels11 commented Jan 3, 2020

Karsten1987 left a comment

Choose a reason for hiding this comment

Karsten1987 Jan 3, 2020

Choose a reason for hiding this comment

Karsten1987 Jan 3, 2020

Choose a reason for hiding this comment

Karsten1987 Jan 3, 2020

Choose a reason for hiding this comment

Karsten1987 Jan 3, 2020

Choose a reason for hiding this comment

zmichaels11 Jan 3, 2020

Choose a reason for hiding this comment

Karsten1987 Jan 3, 2020

Choose a reason for hiding this comment

Karsten1987 Jan 3, 2020

Choose a reason for hiding this comment

zmichaels11 commented Jan 3, 2020

zmichaels11 commented Jan 6, 2020

piraka9011 commented Dec 10, 2019 •

edited by zmichaels11

Loading

piraka9011 commented Dec 16, 2019 •

edited

Loading