Skip to content
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions rosbag2_cpp/include/rosbag2_cpp/compression_options.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace rosbag2_cpp
* Modes are used to specify whether to compress by individual serialized bag messages or by file.
* rosbag2_cpp defaults to NONE.
*/
enum class ROSBAG2_CPP_PUBLIC CompressionMode : uint32_t
enum class ROSBAG2_CPP_PUBLIC CompressionMode: uint32_t
{
NONE = 0,
FILE,
Expand All @@ -40,7 +40,8 @@ enum class ROSBAG2_CPP_PUBLIC CompressionMode : uint32_t
* \param compression_mode A case insensitive string that is either "FILE" or "MESSAGE".
* \return CompressionMode NONE if compression_mode is invalid. FILE or MESSAGE otherwise.
*/
ROSBAG2_CPP_PUBLIC CompressionMode compression_mode_from_string(const std::string & compression_mode);
ROSBAG2_CPP_PUBLIC CompressionMode compression_mode_from_string(
const std::string & compression_mode);

/**
* Converts a rosbag2_cpp::CompressionMode enum into a string.
Expand All @@ -50,5 +51,14 @@ ROSBAG2_CPP_PUBLIC CompressionMode compression_mode_from_string(const std::strin
*/
ROSBAG2_CPP_PUBLIC std::string compression_mode_to_string(CompressionMode compression_mode);

/**
* Compression options used in the writer which are passed down from the CLI in rosbag2_transport.
*/
struct CompressionOptions
{
std::string compression_format;
CompressionMode compression_mode;
Comment thread
piraka9011 marked this conversation as resolved.
};

} // namespace rosbag2_cpp
#endif // ROSBAG2_CPP__COMPRESSION_OPTIONS_HPP_
17 changes: 12 additions & 5 deletions rosbag2_cpp/include/rosbag2_cpp/writer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <unordered_map>
#include <vector>

#include "rosbag2_cpp/compression_options.hpp"
#include "rosbag2_cpp/converter_options.hpp"
#include "rosbag2_cpp/storage_options.hpp"
#include "rosbag2_cpp/visibility_control.hpp"
Expand Down Expand Up @@ -54,13 +55,19 @@ class ROSBAG2_CPP_PUBLIC Writer final
~Writer();

/**
* Opens a new bagfile and prepare it for writing messages. The bagfile must not exist.
* Opens a new bagfile and prepares it for writing messages.
*
* The bagfile must not exist.
* This must be called before any other function is used.
*
* \param storage_options Options to configure the storage
* \param converter_options options to define in which format incoming messages are stored
**/
void open(const StorageOptions & storage_options, const ConverterOptions & converter_options);
* \param storage_options Options to configure the storage.
* \param converter_options Options to define in which format incoming messages are stored.
* \param compression_options Options to configure message or bagfile compression.
*/
void open(
const StorageOptions & storage_options,
const ConverterOptions & converter_options,
const CompressionOptions & compression_options = {"", CompressionMode::NONE});

/**
* Create a new topic in the underlying storage. Needs to be called for every topic used within
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ namespace writer_interfaces
class ROSBAG2_CPP_PUBLIC BaseWriterInterface
{
public:
virtual ~BaseWriterInterface() {}
virtual ~BaseWriterInterface() = default;

virtual void open(
const StorageOptions & storage_options, const ConverterOptions & converter_options) = 0;
const StorageOptions & storage_options,
const ConverterOptions & converter_options,
const CompressionOptions & compression_options) = 0;

virtual void reset() = 0;

Expand Down
33 changes: 32 additions & 1 deletion rosbag2_cpp/include/rosbag2_cpp/writers/sequential_writer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@
#include <unordered_map>
#include <vector>

#include "rosbag2_cpp/compression_options.hpp"
#include "rosbag2_cpp/converter.hpp"
#include "rosbag2_cpp/serialization_format_converter_factory.hpp"
#include "rosbag2_cpp/storage_options.hpp"
#include "rosbag2_cpp/writer_interfaces/base_writer_interface.hpp"
#include "rosbag2_cpp/visibility_control.hpp"

#include "rosbag2_compression/base_compressor_interface.hpp"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

might want to consider a new line after this to group the includes on a per-package basis.

#include "rosbag2_storage/metadata_io.hpp"
#include "rosbag2_storage/storage_factory.hpp"
#include "rosbag2_storage/storage_factory_interface.hpp"
Expand Down Expand Up @@ -71,7 +73,9 @@ class ROSBAG2_CPP_PUBLIC SequentialWriter
* \param converter_options options to define in which format incoming messages are stored
**/
void open(
const StorageOptions & storage_options, const ConverterOptions & converter_options) override;
const StorageOptions & storage_options,
const ConverterOptions & converter_options,
const CompressionOptions & compression_options) override;

void reset() override;

Expand Down Expand Up @@ -102,13 +106,37 @@ class ROSBAG2_CPP_PUBLIC SequentialWriter
*/
void write(std::shared_ptr<rosbag2_storage::SerializedBagMessage> message) override;

protected:
/**
* Initialize the compressor.
*
* \throws runtime_error If the compression implementation does not exist.
*/
virtual void init_compression(const CompressionOptions & compression_options);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what happens if the bag is already opened with a set of specific compression options and then this function is called with a different, potentially conflicting, set of it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This function is only called in open() and is a protected method (for unit testing) so we don't expect to call it anywhere else.
We could simply move the functionality into open() only but that might make the function too large.


/**
* Compress the most recent file and update the metadata file path.
*/
virtual void compress_last_file();

/**
* Checks if the compression by message option is specified and a compressor exists.
*
* If the above conditions are satisfied, compresses the serialized bag message.
*
* \param message The message to compress.
* \return True if compression occurred, false otherwise.
*/
virtual void compress_message(std::shared_ptr<rosbag2_storage::SerializedBagMessage> message);

private:
std::string base_folder_;
std::unique_ptr<rosbag2_storage::StorageFactoryInterface> storage_factory_;
std::shared_ptr<SerializationFormatConverterFactoryInterface> converter_factory_;
std::shared_ptr<rosbag2_storage::storage_interfaces::ReadWriteInterface> storage_;
std::unique_ptr<rosbag2_storage::MetadataIo> metadata_io_;
std::unique_ptr<Converter> converter_;
std::unique_ptr<rosbag2_compression::BaseCompressorInterface> compressor_;

// Used in bagfile splitting; specifies the best-effort maximum sub-section of a bagfile in bytes.
uint64_t max_bagfile_size_;
Expand All @@ -118,6 +146,9 @@ class ROSBAG2_CPP_PUBLIC SequentialWriter

rosbag2_storage::BagMetadata metadata_;

// Used in invoking compression
rosbag2_cpp::CompressionMode compression_mode_;

// Closes the current backed storage and opens the next bagfile.
void split_bagfile();

Expand Down
6 changes: 4 additions & 2 deletions rosbag2_cpp/src/rosbag2_cpp/writer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,11 @@ Writer::~Writer()
}

void Writer::open(
const StorageOptions & storage_options, const ConverterOptions & converter_options)
const StorageOptions & storage_options,
const ConverterOptions & converter_options,
const CompressionOptions & compression_options)
{
writer_impl_->open(storage_options, converter_options);
writer_impl_->open(storage_options, converter_options, compression_options);
}

void Writer::create_topic(const rosbag2_storage::TopicMetadata & topic_with_type)
Expand Down
68 changes: 57 additions & 11 deletions rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@
#include "rosbag2_cpp/writers/sequential_writer.hpp"

#include <algorithm>
#include <cassert>
#include <chrono>
#include <memory>
#include <stdexcept>
#include <string>
#include <utility>

#include "rcpputils/filesystem_helper.hpp"

#include "rcutils/filesystem.h"
#include "rosbag2_compression/zstd_compressor.hpp"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

new line above to group per package.


#include "rosbag2_cpp/info.hpp"
#include "rosbag2_cpp/storage_options.hpp"
Expand All @@ -47,6 +48,7 @@ std::string format_storage_uri(const std::string & base_folder, uint64_t storage
}
} // namespace

// TODO(piraka9011) Initialize defaults in header file instead.
Comment thread
piraka9011 marked this conversation as resolved.
SequentialWriter::SequentialWriter(
std::unique_ptr<rosbag2_storage::StorageFactoryInterface> storage_factory,
std::shared_ptr<SerializationFormatConverterFactoryInterface> converter_factory,
Expand All @@ -56,16 +58,34 @@ SequentialWriter::SequentialWriter(
storage_(nullptr),
metadata_io_(std::move(metadata_io)),
converter_(nullptr),
compressor_{nullptr},
max_bagfile_size_(rosbag2_storage::storage_interfaces::MAX_BAGFILE_SIZE_NO_SPLIT),
topics_names_to_info_(),
metadata_()
{}
metadata_(),
compression_mode_{CompressionMode::NONE} {}


SequentialWriter::~SequentialWriter()
{
reset();
}

void SequentialWriter::init_compression(const CompressionOptions & compression_options)
Comment thread
piraka9011 marked this conversation as resolved.
{
if (compression_options.compression_mode != rosbag2_cpp::CompressionMode::NONE) {
if (compression_options.compression_format == "zstd") {
compressor_ = std::make_unique<rosbag2_compression::ZstdCompressor>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am having a problem with this line as it's imposing a hard dependency on Zstd and all it's dependencies.
rosbag2_cpp should potentially be compiled without any compression algorithm

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have this issue on our backlog next to make compression a plugin instead so that its not hardcoded.

Do you have any other suggestions in the meantime on how to avoid the hard dependency?
Maybe something along the lines of:

auto zstd_compressor = std::make_unique<rosbag2_compression::ZstdCompressor>();
compressor_ = std::make_unique<rosbag2_compression::BaseCompressorInterface>(std::move(zstd_compressor));

If not, I suggest we work with the current implementation and then after introducing the plugin changes to rosbag2_compression use a CompressorFactory to make the writer more robust for different compression implementations.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure I get your proposition right. We can either use a factory for loading it as a plugin dynamically or as discussed in a previous PR, inherit from the SequentialWriter or the BaseReaderInterface and make it live in the compression package. It can still be passed in by the caller side when instantiating the final Reader class. Example here: https://github.com/ros2/rosbag2/blob/master/rosbag2_transport/src/rosbag2_transport/player.cpp#L44-L46

The moment we include a compression header in the rosbag2_cpp package we introduce the dependency. That being said, the current PR also lacks the dependency of rosbag2_compression in its package.xml.

@piraka9011 piraka9011 Jan 13, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, you prefer to isolate compression from rosbag2_cpp.
So instead there would be a CompressionWriter in rosbag2_compression that would implement compression by message or file which can be used in the other rosbag2 packages.
However, that means losing the ability to split and compress simultaneously since we're overriding write() of the SequentialWriter.
Moreover, I don't see the issue of introducing compression as a dependency since we'd like to have that feature available for both the API and the CLI. We'd have to introduce it as a dependency at some point.

One option I see is compressing at the transport layer instead, though I'm not sure we want to do that.

@Karsten1987 Karsten1987 Jan 13, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Introducing the compression functionality is not the problem. So things like compression_options and general compression related structs might very well be living in this package. However, hard coding ztd is giving people no change to not compile zstd and its vendor package etc. This might not suit very well to every use case.
Just the sqlite3 it is possible to compile rosbag2 without any notion of sqlite3.

@piraka9011 piraka9011 Jan 13, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

However, hard coding ztd is giving people no change to not compile zstd and its vendor package etc.

I agree, which is why we want to work on making compression a plugin and work like storage and storage_factory does as opposed to the current implementation which is only Zstd.

However, since this is the first iteration, we want to make sure the whole compression pipeline works with a single compression format before extending it to other formats though a plugin.


P.S. Accidentally edited your comment, my bad.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

P.S. Accidentally edited your comment, my bad.

That's one way to steer the direction of aa conversation ;-)

However, since this is the first iteration, we want to make sure the whole compression pipeline works with a single compression format before extending it to other formats though a plugin.

That's fine. I think having a compression writer/reader implementation within rosbag2_compression is then the right way to go. It gives you the freedom to test your compression pipeline without introducing a dependency to zstd in rosbag2_cpp.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think having a compression writer/reader implementation within rosbag2_compression is then the right way to go.

What about splitting then? Basically implementing a SequentialCompressionWriter?
If that's the case we can consider going down that route instead for now.

} else {
std::stringstream err;
err << "Unsupported compression format " << compression_options.compression_format;
throw std::invalid_argument{err.str()};
}
}
metadata_.compression_format = compression_options.compression_format;
metadata_.compression_mode =
rosbag2_cpp::compression_mode_to_string(compression_options.compression_mode);
}

void SequentialWriter::init_metadata()
{
metadata_ = rosbag2_storage::BagMetadata{};
Expand All @@ -77,10 +97,12 @@ void SequentialWriter::init_metadata()

void SequentialWriter::open(
const StorageOptions & storage_options,
const ConverterOptions & converter_options)
const ConverterOptions & converter_options,
const CompressionOptions & compression_options)
{
max_bagfile_size_ = storage_options.max_bagfile_size;
base_folder_ = storage_options.uri;
compression_mode_ = compression_options.compression_mode;

if (converter_options.output_serialization_format !=
converter_options.input_serialization_format)
Expand All @@ -89,25 +111,28 @@ void SequentialWriter::open(
}

const auto storage_uri = format_storage_uri(base_folder_, 0);

storage_ = storage_factory_->open_read_write(storage_uri, storage_options.storage_id);
if (!storage_) {
throw std::runtime_error("No storage could be initialized. Abort");
}

if (max_bagfile_size_ != 0 &&
max_bagfile_size_ < storage_->get_minimum_split_file_size())
if (storage_options.max_bagfile_size != 0 &&
storage_options.max_bagfile_size < storage_->get_minimum_split_file_size())
{
throw std::runtime_error(
"Invalid bag splitting size given. Please provide a different value.");
throw std::invalid_argument{
"Invalid bag splitting size given. Please provide a different value."};
}

init_metadata();
init_compression(compression_options);
Comment thread
mm318 marked this conversation as resolved.
}

void SequentialWriter::reset()
{
if (!base_folder_.empty()) {
if (compressor_ && compression_mode_ == CompressionMode::FILE) {
Comment thread
piraka9011 marked this conversation as resolved.
Outdated
compress_last_file();
}
finalize_metadata();
metadata_io_->write_metadata(base_folder_, metadata_);
}
Expand Down Expand Up @@ -163,17 +188,26 @@ void SequentialWriter::remove_topic(const rosbag2_storage::TopicMetadata & topic
}
}

void SequentialWriter::compress_last_file()
{
assert(compressor_ != nullptr);
metadata_.relative_file_paths.back() =
compressor_->compress_uri(metadata_.relative_file_paths.back());
}

void SequentialWriter::split_bagfile()
{
if (compression_mode_ == CompressionMode::FILE) {
compress_last_file();
}

const auto storage_uri = format_storage_uri(
base_folder_,
metadata_.relative_file_paths.size());
storage_ = storage_factory_->open_read_write(storage_uri, metadata_.storage_identifier);

if (!storage_) {
std::stringstream errmsg;
errmsg << "Failed to rollover bagfile to new file: \"" << storage_uri << "\"!";

throw std::runtime_error(errmsg.str());
}

Expand All @@ -185,6 +219,14 @@ void SequentialWriter::split_bagfile()
}
}

void SequentialWriter::compress_message(
std::shared_ptr<rosbag2_storage::SerializedBagMessage> message)
{
assert(compressor_ != nullptr);
auto converted_message = converter_ ? converter_->convert(message) : message;
compressor_->compress_serialized_bag_message(converted_message.get());
}

void SequentialWriter::write(std::shared_ptr<rosbag2_storage::SerializedBagMessage> message)
{
if (!storage_) {
Expand All @@ -205,6 +247,10 @@ void SequentialWriter::write(std::shared_ptr<rosbag2_storage::SerializedBagMessa
const auto duration = message_timestamp - metadata_.starting_time;
metadata_.duration = std::max(metadata_.duration, duration);

if (compression_mode_ == CompressionMode::MESSAGE) {
compress_message(message);
}

storage_->write(converter_ ? converter_->convert(message) : message);
}

Expand Down
1 change: 1 addition & 0 deletions rosbag2_cpp/test/rosbag2_cpp/test_multifile_reader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ class ReaderCompressionTest : public Test
rosbag2_storage::TopicMetadata{"test", "test_msgs/BasicTypes", serialization_format_};
auto message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
message->topic_name = topic_metadata_.name;

ON_CALL(*storage_, read_next()).WillByDefault(Return(message));
EXPECT_CALL(*storage_factory_, open_read_only(_, _)).WillRepeatedly(Return(storage_));
EXPECT_CALL(*metadata_io_, metadata_file_exists(_)).WillRepeatedly(Return(true));
Expand Down
Loading