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.

Sign up for GitHub

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

Jump to bottom

[Compression - 5] Add Zstd file compression #220

Merged
merged 25 commits into from
Dec 12, 2019
Merged

[Compression - 5] Add Zstd file compression #220

merged 25 commits into from
Dec 12, 2019

Conversation

piraka9011
Copy link
Contributor

Implement file compression for Zstd with tests.

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

Anas Abou Allaban added 3 commits December 2, 2019 19:29
Implement zstd file compression
9c22acb 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Add test for zstd file compression
c2d52ec 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Remove extra package
50c397e 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Add zstd_vendor for compression
ff593f2 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
zmichaels11
zmichaels11 suggested changes Dec 5, 2019
View reviewed changes
Copy link
Contributor

@zmichaels11 zmichaels11 left a comment

Choose a reason for hiding this comment

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

See comments

rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
zmichaels11
zmichaels11 suggested changes Dec 5, 2019
View reviewed changes
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
Anas Abou Allaban added 3 commits December 9, 2019 09:16
Update and simplify zstd_vendor
1df3366 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

Add zstd_vendor, use scopes for files

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Use u_ptr, scope for files, and findlib for zstd
b28ba93 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

Use u_ptr, scope for files, and findlib for zstd

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Use raw pointers since Zstd is C library
42378f8 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>

Wrap file output statement

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

Constness

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

piraka9011 commented Dec 9, 2019 •
edited
Loading

@zmichaels11 @emersonknapp I was unable to go with your suggestions of using a std::string or unique_ptr for holding the compression buffers. I spent yesterday trying to figure out why and it seems that since Zstd is a C library, it’s unable to free the smart pointer even if I pass a custom destructor to the unique_ptr constructor.

@piraka9011
Copy link
Contributor Author

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/piraka9011/b6a4a79d7614c122a824d81d813a654f/raw/df4a4c3c0666d16fb4f902c9a8ce5b026111ad9f/ros2.repos
BUILD args: --packages-up-to rosbag2_compression zstd_vendor
TEST args: --packages-select rosbag2_compression zstd_vendor
Job: ci_launcher

@zmichaels11
Copy link
Contributor

zmichaels11 commented Dec 9, 2019 •
edited
Loading

@piraka9011 to make a buffer using unique_ptr, you want:

auto buffer = std::make_unique<char[]>(buffer_size);

@piraka9011
Copy link
Contributor Author

piraka9011 commented Dec 9, 2019 •
edited
Loading

@piraka9011 to make a buffer using unique_ptr, you want:

auto buffer = std::make_unique<char[]>(buffer_size);

Tried that :) In fact, to have support for < C++14, you need:

std::make_unique<char[]>(new char[buffer_size], (Custom dtor..) )

See this SO answer and comment on why.

@zmichaels11
Copy link
Contributor

zmichaels11 commented Dec 9, 2019 •
edited
Loading

Rosbag2 does target C++14, no?

@zmichaels11
Copy link
Contributor

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@piraka9011 piraka9011 added this to the F milestone Dec 9, 2019
zmichaels11
zmichaels11 approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@zmichaels11 zmichaels11 left a comment

Choose a reason for hiding this comment

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

Just a bunch of nits

rosbag2_compression/package.xml Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/test/rosbag2_compression/test_zstd_compressor.cpp Show resolved Hide resolved
More const and minor nits
99a912d 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@thomas-moulard
Copy link
Contributor

fopen() + std::vector<uint8_t> is what you want IMHO

Use vector<uint8_t> to read in file buffer and compress
2ff042e 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

@Karsten1987 @thomas-moulard check it out 😃

Vectors and uint8_t. File handling should work with Windows too, unless I'm missing anything.

Karsten1987
Karsten1987 reviewed Dec 11, 2019
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

this looks already way better to me

rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
rosbag2_compression/src/rosbag2_compression/zstd_compressor.cpp Outdated Show resolved Hide resolved
@Karsten1987
Copy link
Collaborator

please make sure to run CI on this.

@piraka9011
Copy link
Contributor Author

piraka9011 commented Dec 11, 2019 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Uncrustify and alphasort
21603a0 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Fix null pointer operation
c72f0c8 
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

piraka9011 commented Dec 11, 2019 •
edited
Loading

New windows build for cppcheck

  • Windows Build Status

@piraka9011
Copy link
Contributor Author

piraka9011 commented Dec 12, 2019 •
edited by zmichaels11
Loading

Windows is still unstable since fopen apparently isn't secure on windows.

Proposed solution:

FILE * get_file_pointer(const std::string & uri, const std::string & read_mode)
{
  FILE * fp = nullptr;
#ifdef _WIN32
  fopen_s(&fp, uri.c_str(), read_mode.c_str());
#else
  fp = fopen(uri.c_str(), read_mode.c_str());
#endif
  return fp;
}

or simply

#ifdef _MSC_VER
#define _CRT_SECURE_NO_WARNINGS 1
#endif

@Karsten1987
Copy link
Collaborator

Your first proposed solution makes sense to me. We apply a similar pattern already in various things in rcutils.

@zmichaels11
Use Windows-specific fopen_s
79296d6 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Use actual buffer sizes instead of std::vector::capacity
53ab32e 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@zmichaels11
Copy link
Contributor

zmichaels11 commented Dec 12, 2019 •
edited
Loading

@Karsten1987 I've applied the suggestions in:

@ros2/aws-oncall Run this CI: #220 (comment)

@piraka9011
Copy link
Contributor Author

piraka9011 commented Dec 12, 2019 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@piraka9011 piraka9011 mentioned this pull request Dec 12, 2019
Closed
@zmichaels11
Alphasort headers
3b54aec 
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
@piraka9011 piraka9011 merged commit de2c3ab into ros2:master Dec 12, 2019
@zmichaels11 zmichaels11 mentioned this pull request Dec 16, 2019
Merged
@zmichaels11 zmichaels11 deleted the compression/zstd-header branch December 18, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp left review comments

@dirk-thomas dirk-thomas dirk-thomas left review comments

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@thomas-moulard thomas-moulard Awaiting requested review from thomas-moulard

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
ROS 2 Tooling Working Milestone #2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@piraka9011 @zmichaels11 @thomas-moulard @Karsten1987 @emersonknapp @dirk-thomas