Skip to content

DDS metadata support - #11412

Merged
Nir-Az merged 20 commits into
realsenseai:ddsfrom
OhadMeir:dds
Mar 9, 2023
Merged

DDS metadata support#11412
Nir-Az merged 20 commits into
realsenseai:ddsfrom
OhadMeir:dds

Conversation

@OhadMeir

@OhadMeir OhadMeir commented Feb 8, 2023

Copy link
Copy Markdown
Contributor

This adds metadata support to the DDS.

Metadata and data are synchronized based on frame ID, synchronization is done at LibRS level.
Metadata information is sent using flexible_msg topic, added dds_metadata_stream/server type to handle this stream type.

This is not the final PR for this subject, I want to merge it for now so work done by Eran will not branch to far away from mine. Future PRs will include:

  1. metadata supported as part of device_info, not device level option.
  2. Remove references to metadata stream names in realdds.
  3. Move DDS code from context.cpp to new files to reduce file size and separate dependencies.

Comment thread src/frame.h
Comment thread src/metadata-parser.h Outdated
Comment thread src/frame.h Outdated
Comment thread third-party/realdds/include/realdds/dds-device-server.h Outdated
Comment thread third-party/realdds/include/realdds/dds-stream-server.h Outdated
Comment thread third-party/realdds/include/realdds/dds-stream-server.h Outdated
Comment thread third-party/realdds/include/realdds/dds-stream.h Outdated
Comment thread third-party/realdds/include/realdds/dds-stream.h Outdated
Comment thread third-party/realdds/include/realdds/dds-stream.h Outdated
Comment thread third-party/realdds/src/dds-device-impl.cpp Outdated
Comment thread third-party/realdds/src/dds-device-impl.cpp Outdated
Comment thread third-party/realdds/src/dds-device-impl.cpp Outdated
Comment thread third-party/realdds/src/dds-device-impl.cpp Outdated
Comment thread third-party/realdds/src/dds-device.cpp Outdated
Comment thread third-party/realdds/src/dds-stream.cpp Outdated
Comment thread src/context.cpp
Comment thread src/context.cpp Outdated
Comment thread src/context.cpp
Comment thread src/context.cpp Outdated
Comment thread src/context.cpp Outdated
Comment thread src/context.cpp Outdated
Comment thread src/context.cpp
Comment thread src/context.cpp
// prof parameter holds the real data, rs2_software_video_frame forces us to hold a pointer to it.
// Because we use syncer, not calling on_video_frame in the lifetime of this function, we need a shared_ptr
// that the syncer will hold till using the frame.
std::shared_ptr< rs2_stream_profile > prof_holder = std::make_shared< rs2_stream_profile >();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we need a holder to hold information about the frame (whether an extra smart ptr for lifetime or otherwise), how about:

  • define a "synced frame" object that has just the relevant information needed by the syncer (i.e., frame-id)
  • contains a pointer to the actual frame (as a void*), possibly with a deleter
  • if needed, contains any additional pointers (like this holder)

So this class doesn't have to be a template. You deal with your own frame objects and only the callbacks need to know how to interpret the frame (and metadata).

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 don't see how to avoid the template using the current rs2_software_video_frame and rs2_motion_video_frame API structs. The syncer still needs to call on_video_frame/on_motion_frame with the correct type.

Changes to the API should be done carefully and in a different PR.

Comment thread src/context.cpp Outdated

//Copying from dds into LibRS space, same as copy from USB backend.
//TODO - use memory pool or some other frame allocator
rs2_frame.pixels = new uint8_t[dds_frame.size];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about a new std::vector( std::move( dds_frame.raw_data ))?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I.e.,

auto p_pixels_vector = new std::vector< uint8_t >( std::move( dds_frame.raw_data ));
rs2_frame.pixels = p_pixels_vector->data();
rs2_frame.deleter = [p_pixels_vector]( void * ) { delete p_pixels_vector; };

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.

The deleter is a plain C function void(*deleter)(void*) so I can't bind lambda parameters

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.

Added to the to-do list a comment about changing the deleter API to receive a parameter.

Comment thread third-party/realdds/doc/DDS ICD.md Outdated
Comment thread third-party/realdds/doc/DDS ICD.md Outdated
Comment thread third-party/realdds/include/realdds/dds-stream-base.h Outdated
Comment thread third-party/realdds/include/realdds/dds-stream-base.h
_running = true;

// Start handling options only after init() is done
if( !_notifications_reader )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure what this is doing here... belongs inside create_notifications_reader()?

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.

It is a check before dereferencing _notification_reader in the next line.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes I know that... but doesn't create_notifications_reader() throw if it fails?

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.

Did not throw. I moved the check to create_notification_reader function so it will throw from there.

void dds_device::impl::create_metadata_reader()
{
if( _metadata_reader )
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

// We can be called multiple times, once per stream

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.

Yes, the first metadata enabled stream will create the reader for the device. Subsequent calls will just return.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes meaning you added the comment?

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.

Added now :-)

if( ! notification.is_valid() )
continue;

if( _on_metadata_available )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any chance this will change during runtime?
Up until now was to set the callbacks before run() is called. I propose that we keep to that and only set the callback if we have a callback.

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.

The callback is set during dds_device_proxy constructor and does not change during runtime.
Still need to check validity before dereferencing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But that's what I'm saying: if the callback isn't set when we run() then no need to set up another on_data_available callback...

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.

Currently run is being called from on_device_added callback.
The callback is only set later in dds_device_proxy constructor.
@maloel Do you mean to move the call to run into dds_device_proxy constructor? And then I will only check if it is set once when I check _metadata_reader existence?

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 so then we need to pass message_timeout_ms parameter to the dds_device_proxy

Comment thread third-party/realdds/src/dds-stream-server.cpp
Comment thread third-party/realdds/include/realdds/topics/image/image-msg.h
Comment thread third-party/realdds/src/dds-stream-server.cpp
@Nir-Az
Nir-Az merged commit edb597f into realsenseai:dds Mar 9, 2023
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.

3 participants