-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
remove image-msg::frame-id; use timestamp #11751
Conversation
@@ -49,10 +52,9 @@ class dds_stream_server : public dds_stream_base | |||
void close(); | |||
|
|||
bool is_streaming() const override { return _streaming; } | |||
void start_streaming( const image_header & header ); | |||
void start_streaming( const image_header & ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the variable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because saying image_header header
is redundant and unhelpful. If the type conveys the meaning, then everything else is extra. But that's in hindsight -- maybe I changed something and then undid it...?
raw_image.encoding() = _image_header.format.to_string(); | ||
raw_image.height() = _image_header.height; | ||
raw_image.width() = _image_header.width; | ||
raw_image.step() = uint32_t( image.raw_data.size() / _image_header.height ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check _image_header.height != 0
(can be in start_streaming
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to address this when I do the IMU work, since an image_header is the wrong thing to use when the data isn't an image.
raw_image.height() = _image_header.height; | ||
raw_image.width() = _image_header.width; | ||
raw_image.step() = uint32_t( image.raw_data.size() / _image_header.height ); | ||
raw_image.is_bigendian() = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is true for Jetson?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's reflective of our images (whoever created the data) rather than the system. For now it should do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not entirely sure what it's indicative of. It might refer to the endian-ness of the message header, since the image format is pretty much set in stone...
I think we'll address this issue when we get there.
if( ! is_streaming() ) | ||
DDS_THROW( runtime_error, "stream '" + name() + "' cannot publish before start_streaming()" ); | ||
|
||
if( image.height != _image_header.height ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need this? motion does not have height and width.
Same question for step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a copy of publish_image
until I get to the IMU.
|
||
DDS_API_CALL( _writer->get()->write( &raw_image ) ); | ||
} | ||
|
||
} // namespace realdds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check new line at EOF
server->publish( static_cast< const uint8_t * >( f.get_data() ), f.get_data_size(), f.get_frame_number() ); | ||
realdds::topics::image_msg image; | ||
auto data = static_cast< const uint8_t * >( f.get_data() ); | ||
image.raw_data.assign( data, data + f.get_data_size() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the data?
Maybe construct a new vector on this range and move to raw_data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructing a new vector (meaning a copy) and moving it is the same as assigning the data. We assigned the data inside publish
before, too, so this is not a change.
if( res.rem < 0 ) | ||
{ | ||
--res.quot; | ||
res.rem += 1000000000ull; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this number a lot. Good opportunity to use constexpr uint64_t nanos_in_second = 1'000'000'000ull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be a good idea, but I don't want to introduce this in a header at this point...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Follow-up to #11665, this introduces timestamp-based synchronization of metadata to images:
frame-id
in the MD toframe-number
to be consistent, since that's what it's called in librsframe_id
field in imageframe_id
in the ROS2 image format is a string denoting the device & sensor - this needs more workAlong the way:
dds_time
with negatives, and added adds-time
unit-test that clarifies a lot of its (mis)behaviorimage
formatimage_msg
,flexible_msg
, located under topics/publish_image
frompublish_motion
As before, I suggest looking at the commits one by one; they're fairly self-contained.
Next is adding the ros2imu format and getting that to work.