Add compressing and decompressing modes #24

ivanpauno · 2021-06-02T14:52:46Z

5007b22 fixes a bug.
90a4fc9 implements the compressing and decompressing modes.

There are a lot of things that could be improved but this is an okay first prototype.

e.g.:

# decompressing_bridge.yaml
name: decompressing_bridge
from_domain: 3
to_domain: 2
mode: decompress
topics:
  chatter:
    type: example_interfaces/msg/String
    qos:
      reliability: best_effort

# compressing_bridge.yaml
name: compressing_bridge
from_domain: 0
to_domain: 3
mode: compress
topics:
  chatter:
    type: example_interfaces/msg/String
    qos:
      reliability: best_effort

In one terminal:

ros2 run domain_bridge domain_bridge decompressing_bridge.yaml &
ros2 run domain_bridge domain_bridge compressing_bridge.yaml &
ros2 topic pub /chatter example_interfaces/msg/String "{data: asd}"

In another terminal:

ROS_DOMAIN_ID=2 ros2 topic echo /chatter

ivanpauno · 2021-06-02T14:52:57Z

FYI @jacobperron

clalancette · 2021-06-02T15:06:55Z

Just as a thought, it is probably worthwhile to look at the pluggable compression that rosbag2 has. It's probably overkill for right now, but it might a) allow you to have pluggable compression, and b) share some code with rosbag2.

ivanpauno · 2021-06-02T16:16:49Z

Just as a thought, it is probably worthwhile to look at the pluggable compression that rosbag2 has. It's probably overkill for right now, but it might a) allow you to have pluggable compression, and b) share some code with rosbag2.

Yes, I actually copy-pasted part of the code from there.
I thought about using their compression plugins, but I found it easier to use zstd directly here for a first prototype.

ivanpauno · 2021-06-02T19:51:46Z

I tried this with the pc_pipe demo, and the compression rate is pretty good.

Results using a 640x480 rgb image:

A 921600B image was compressed to 187077B (~1/5).
A 4915200B pointcloud to 297651B (~1/16).

That's a really promising bandwidth reduction.

ivanpauno · 2021-06-02T19:52:53Z

I think that the bridge mode (compressing/decompressing/normal) could be specified per topic and not globally, once we merge this I'll work on improving that.

ivanpauno · 2021-06-07T21:31:52Z

@jacobperron friendly ping, I think we should at least merge the first commit 5007b22.

jacobperron

The approach sounds good to me; relying on zstd directly and later we can consider a plugin-based approach.

I think that we should make the compression mode per topic (as you suggest), since it's at the topic-level where this compression is happening.

I'd also like to see the following:

Tests for the compression feature (to prevent regression)
Green CI (Gpr and Rpr jobs).
- There's linter errors on Rolling
- It looks like there's an rclcpp API being used that's not available in Galactic. Preferable we could workaround this so we don't need to branch, but it's not a blocker.

examples/example_bridge_config.yaml

include/domain_bridge/domain_bridge_options.hpp

src/domain_bridge/compress_messages.cpp

src/domain_bridge/parse_domain_bridge_yaml_config.cpp

jacobperron · 2021-06-08T21:24:45Z

@ivanpauno Submitting the bug fix as a separate PR would be nice, then we can merge that independently.

ivanpauno · 2021-06-09T13:07:18Z

@ivanpauno Submitting the bug fix as a separate PR would be nice, then we can merge that independently.

Done, see #27.

ivanpauno · 2021-06-09T13:46:00Z

There's linter errors on Rolling

Sorry, I forgot to check that. Fixed in ee83cad, 940dca3.

ivanpauno · 2021-06-09T14:13:42Z

It looks like there's an rclcpp API being used that's not available in Galactic. Preferable we could workaround this so we don't need to branch, but it's not a blocker.

Fixed in ffef015, 53b0628.

ivanpauno · 2021-06-10T17:26:55Z

Tests for the compression feature (to prevent regression)

I have refactored some of the existing tests in 7ce726e, to deduplicate code.
New tests were added in 6a438f3.

ivanpauno · 2021-06-10T17:27:49Z

I think that we should make the compression mode per topic (as you suggest), since it's at the topic-level where this compression is happening.

I think that we can do that in a follow-up PR, IIUC the current plan was to compress of the traffic.

ivanpauno · 2021-06-10T17:28:46Z

The Gpr branch is failing because main isn't compatible with galactic anymore (see #27).

jacobperron · 2021-06-15T18:55:18Z

The Gpr branch is failing because main isn't compatible with galactic anymore (see #27).

I misunderstood the nature of #27. Is it possible we can keep the main branch compatible with Galactic if we factor out the generic pub/sub code (#28)? @rebecca-butler is taking a look at doing this right now.

ivanpauno · 2021-06-16T16:00:08Z

I misunderstood the nature of #27. Is it possible we can keep the main branch compatible with Galactic if we factor out the generic pub/sub code (#28)?

I think so.
There will probably be some conflicts with this PR when we introduce the rclcpp generic pub/sub, as the generic pub/sub API changed a bit.

@jacobperron should we merge this as-is to avoid conflicts or do you prefer to first reintroduce compatibility with the galactic branch?

ivanpauno · 2021-06-16T17:27:14Z

#29 reintroduces compatibility with galactic, I think we can merge that one first and then go ahead with this one.

jacobperron · 2021-06-17T15:12:40Z

Now that #30 is merged. I'm hopeful that rebasing on main will resolve Galactic compatibility issues.

* Add domain_bridge/msg/CompressedMsg interface. * Add compress and decompress mode to the bridge. * Refactor existing tests to better reuse code. * Add tests for compress and decompress modes. Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno · 2021-06-17T22:07:15Z

Now that #30 is merged. I'm hopeful that rebasing on main will resolve Galactic compatibility issues.

Rebased.
The rebase wasn't as easy as I would've wanted, because either:

The API create a GenericPublisher isn't flexible enough.
PublisherBase doesn't have a method to publish serialized messages.

Either thing would have made this patch easier.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

jacobperron

LGTM. I've left a few minor comments.

The rebase wasn't as easy as I would've wanted, because either:

The API create a GenericPublisher isn't flexible enough.
PublisherBase doesn't have a method to publish serialized messages.

That's too bad. I suppose we should support one of those options (or both) in rclcpp. If you agree, can you create an rclcpp ticket and reference it the code here as a comment? Thanks 🙇

test/domain_bridge/test_domain_bridge_end_to_end.cpp

jacobperron · 2021-06-18T20:39:22Z

test/domain_bridge/test_domain_bridge_end_to_end.cpp

+{
+public:
+  explicit ScopedAsyncSpinner(std::shared_ptr<rclcpp::Context> context)
+  : executor_{get_executor_options_with_context(std::move(context))},


Are the std:move calls necessary? It seems like the code would be more straightforward without them.

They aren't necessary, but it's cheaper to move a shared pointer passed by value than to copy it again.
(i.e. this is avoiding to increment the reference count and then decrement it again in the destructor of the context argument)

IMO this is the standard way of getting part ownership of a shared_ptr.

test/domain_bridge/test_domain_bridge_end_to_end.cpp

src/domain_bridge/domain_bridge.cpp

Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Jacob Perron <[email protected]>

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

jacobperron

LGTM

jacobperron reviewed Jun 8, 2021

View reviewed changes

ivanpauno mentioned this pull request Jun 10, 2021

Add template method to help bridging services #26

Merged

ivanpauno requested a review from jacobperron June 10, 2021 19:01

ivanpauno force-pushed the ivanpauno/add-compressing-and-decompressing-modes branch from 6a438f3 to 4122e0b Compare June 17, 2021 22:05

Implicit constructor was unnecessary

3134ba6

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

jacobperron approved these changes Jun 18, 2021

View reviewed changes

ivanpauno and others added 2 commits June 22, 2021 10:46

fix grammar mistakes

aa545cd

Signed-off-by: Ivan Santiago Paunovic <[email protected]> Co-authored-by: Jacob Perron <[email protected]>

Use steady_clock

be689d2

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno requested a review from jacobperron June 22, 2021 21:03

jacobperron approved these changes Jun 22, 2021

View reviewed changes

ivanpauno merged commit 1030a4a into ros2:main Jun 23, 2021

ivanpauno deleted the ivanpauno/add-compressing-and-decompressing-modes branch June 23, 2021 14:14

aprotyas mentioned this pull request Oct 28, 2021

Add Foxy support #60

Open

7 tasks

Add compressing and decompressing modes #24

Add compressing and decompressing modes #24

Uh oh!

Conversation

ivanpauno commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanpauno commented Jun 2, 2021

Uh oh!

clalancette commented Jun 2, 2021

Uh oh!

ivanpauno commented Jun 2, 2021

Uh oh!

ivanpauno commented Jun 2, 2021

Uh oh!

ivanpauno commented Jun 2, 2021

Uh oh!

ivanpauno commented Jun 7, 2021

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobperron commented Jun 8, 2021

Uh oh!

ivanpauno commented Jun 9, 2021

Uh oh!

ivanpauno commented Jun 9, 2021

Uh oh!

ivanpauno commented Jun 9, 2021

Uh oh!

ivanpauno commented Jun 10, 2021

Uh oh!

ivanpauno commented Jun 10, 2021

Uh oh!

ivanpauno commented Jun 10, 2021

Uh oh!

jacobperron commented Jun 15, 2021

Uh oh!

ivanpauno commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanpauno commented Jun 16, 2021

Uh oh!

jacobperron commented Jun 17, 2021

Uh oh!

ivanpauno commented Jun 17, 2021

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacobperron Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

ivanpauno Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ivanpauno commented Jun 2, 2021 •

edited

Loading

ivanpauno commented Jun 16, 2021 •

edited

Loading