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.

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

fix: ros2 gstreamer timestamps #83

Merged
merged 2 commits into from
Jun 15, 2022

Conversation

drwnz
Copy link

@drwnz drwnz commented Jun 13, 2022

Addresses #84
When use_gst_timestamps was is set to true, timestamps converted from the gstreamer buffer timestamps would be incorrect.
This is a ROS2 specific issue, caused by no constructor for rclcpp::Time which takes the time parameter in seconds as a float type.

This PR:

  • Adds use_gst_timestamps to the readme
  • Uses nanoseconds instead of seconds to calculate the offset timestamps from the gstreamer buffer timestamps

It has been tested on Galactic.
@wep21

@wep21 wep21 self-requested a review June 13, 2022 17:55
@wep21 wep21 force-pushed the fix/ros2_gstreamer_timestamps branch from 92fd449 to 2391d25 Compare June 13, 2022 17:59
@wep21
Copy link
Collaborator

wep21 commented Jun 13, 2022

I confirmed the changes modified the timestamp in header even when use_gst_timestamps is true.
before:

stamp:
  sec: 1
  nanosec: 655143410
frame_id: camera_frame

after:

stamp:
  sec: 1655143667
  nanosec: 434399126
frame_id: camera_frame

Copy link
Collaborator

@wep21 wep21 left a comment

Choose a reason for hiding this comment

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

LGTM

@wep21
Copy link
Collaborator

wep21 commented Jun 13, 2022

@jbohren @clydemcqueen the changes look good to me. What do you think?

@wep21 wep21 requested a review from jbohren June 13, 2022 18:19
@clydemcqueen
Copy link

LGTM. I tested this on Galactic and it works as expected. Thanks @drwnz for finding & fixing and @wep21 for moving this forward!

I noticed a few unrelated bugs in the README:

  • the build badge references clydemcqueen/gscam, it should reference ros-drivers/gscam
  • the GStreamer version for Rolling should be 1.20, not 1.18
  • Humble should be added to the list of supported versions

/Clyde

@wep21
Copy link
Collaborator

wep21 commented Jun 15, 2022

@clydemcqueen Thank you for confirming it. Could you create another PR to fix README?

@wep21 wep21 merged commit f9100e4 into ros-drivers:ros2 Jun 15, 2022
@wep21 wep21 deleted the fix/ros2_gstreamer_timestamps branch June 15, 2022 17:00
@wep21
Copy link
Collaborator

wep21 commented Jun 15, 2022

@drwnz Thank you for fixing the bug.

@clydemcqueen
Copy link

I submitted a PR to fix the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants