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.
Why did you remove the variable name?
There was a problem hiding this comment.
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.
Need to check _image_header.height != 0 (can be in start_streaming)
There was a problem hiding this comment.
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.
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.
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.
Need this? motion does not have height and width.
Same question for step
There was a problem hiding this comment.
This is a copy of publish_image until I get to the IMU.
|
|
||
| DDS_API_CALL( _writer->get()->write( &raw_image ) ); | ||
| } | ||
|
|
||
| } // namespace realdds |
| 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.
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.
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.
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.
That might be a good idea, but I don't want to introduce this in a header at this point...
Follow-up to #11665, this introduces timestamp-based synchronization of metadata to images:
frame-idin the MD toframe-numberto be consistent, since that's what it's called in librsframe_idfield in imageframe_idin the ROS2 image format is a string denoting the device & sensor - this needs more workAlong the way:
dds_timewith negatives, and added adds-timeunit-test that clarifies a lot of its (mis)behaviorimageformatimage_msg,flexible_msg, located under topics/publish_imagefrompublish_motionAs 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.