basic dds metadata tests (just the beginning) #11665

maloel · 2023-04-10T11:22:47Z

moved the syncer out of context.cpp and into realdds
- this presented a problem, as there is no frame_holder outside of librealsense
- I considered making it sync a device::image like it was before but that would have brought back the profile storage thing and wasn't as translatable to a different framework like for testing; decided against it
- finally settled on a simple unique_ptr approach with a deleter for on-release that's now passed to the syncer
fixed a couple of syncer bugs, plus a bug in context which did not pick up frame metadata correctly
added some more missing functionality in pyrealdds, for testing
exposed syncer in pyrealdds (though no tests in this review are yet using it)
added initial (just the beginning) of no-metadata and with-metadata tests
- these are the first tests that actually transfer images over dds, I believe

By the way:

move dds-trinsics out of dds-defines
dds_time is now more compact with a better pyrealdds repr
added timestamp to topics::device::image
improved shorten_json_string
json uses strings for keys, so passing in const char * is a "waste" in that strlen and an alloc is then used to build a string
- instead, if we already have a string, better to pass it in -- so rsutils::json:: functions have been updated to accept strings
- dds metadata was using rs2_frame_metadata_to_string which returns a const char * -- so, instead, we now directly call librealsense::get_string( metadata ) which was changed to return a string reference and use array indexing -- should now be a lot faster
changed dds.wait_for_devices to be context notification based
improve test.remote; introduce test.closure for easier test writing; revised test-librs-connections
fixed some warnings and several flushing issues evident in Linux
changed the GHA linux DDS tests to run in RSUSB and not run any other-than-dds tests

Many of these are better looked at by commit rather than going over all the changes together.

Still to do:

more direct testing of the syncer (add metadata-syncer unit-tests #11695)
the syncer has a problem in that the callbacks should not be issued while the syncer holds the mutex (add metadata-syncer unit-tests #11695)
timestamp-based sync and related changes
frame timestamp/domain without metadata
figure out the multiple-participant issue in Linux for test-metadata

src/to-string.cpp

OhadMeir · 2023-04-11T08:40:32Z

src/to-string.cpp

-    }
+        std::vector< std::string > arr( RS2_FRAME_METADATA_COUNT );
+#define CASE( X ) STRARR( arr, FRAME_METADATA, X );
+        CASE( FRAME_COUNTER )


Its a matter of style but I prefer not to define and undefine CASE, and probably doing so for all other types as well.
You can change CASE( FRAME_COUNTER ) to STRARR( arr, FRAME_METADATA, FRAME_COUNTER ) and you can do it easily for all the cases here at once in VS using vertical editing.

Replied on call :)

OhadMeir · 2023-04-11T10:23:11Z

third-party/realdds/include/realdds/dds-metadata-syncer.h

+    void enqueue_frame( key_type, frame_holder && );
+    void enqueue_metadata( key_type, metadata_type && );
+
+    dds_metadata_syncer & on_frame_release( on_frame_release_callback cb )


I don't think that on_xxx functions should return a value.
They are intended to set an aspect of the syncer behavior, nothing about them or their name implies returning something, let alone this syncer object.

Using this to chain function calls is not clear syntax, especially when passing lambda as input parameters. It is more readable to call syncer_object.on_xxx(...) multiple times expressing the object name.

I guess it's a matter of preference: I see no difference between these and operator=, for example. And the practice is well-in-use if you're used to pybind, for another example, or simply want to construct an object in multiple steps while still keeping it unnamed.
In this case:

_stream_name_to_syncer[dds_stream->name()] .on_frame_release( frame_releaser ) .on_frame_ready( [this]( syncer_type::frame_holder && fh, json && md ) { ... } );

Is now:

auto & syncer = _stream_name_to_syncer[dds_stream->name()]; syncer.on_frame_release( frame_releaser ); syncer.on_frame_ready( [this]( syncer_type::frame_holder && fh, json && md ) { ... } );

OhadMeir · 2023-04-16T07:58:35Z

src/context.cpp

@@ -187,19 +188,21 @@ namespace librealsense
        assert( _device_watcher->is_stopped() );

 #ifdef BUILD_WITH_DDS
-        if( rsutils::json::get< bool >( settings, "dds-discovery", true ) )
+        if( rsutils::json::get< bool >( settings, std::string( "dds-discovery", 13 ), true ) )


Do you have to explicitly use the string size? It is inconvenient and error prone

Looks like it should work without the explicit size (using constructor number 5, not 4)

It's optional, but I dislike using strlen for constants

What if you miscount?
Will the json find the right string while searching for dds-discover or dds-discovery (extra char can be of any value) or would it throw?

It's a constant. It's like getting a constant value wrong.
This is why we have a CR and testing :)

OhadMeir · 2023-04-16T08:10:43Z

src/context.cpp


            // Expected metadata for all depth images
-            if( rsutils::json::has( md_header, "depth-units" ) )
-                f->additional_data.depth_units = rsutils::json::get< float >( md_header, "depth-units" );
+            rsutils::json::get_ex( md_header, std::string( "depth-units", 11 ), &f->additional_data.depth_units );


Will throw for color frames

get_ex returns a bool on failure, and checks if it's there first. It can still throw, but not if it's not there.

OhadMeir · 2023-04-16T09:51:26Z

third-party/realdds/src/dds-metadata-syncer.cpp

+void dds_metadata_syncer::enqueue_frame( key_type id, frame_holder && frame )
+{
+    std::lock_guard< std::mutex > lock( _queues_lock );
+    while( _frame_queue.size() >= max_frame_queue_size )


I think that with your changes max_frame_queue_size should be 1.
If no metadata is received, we want to delay the frame for maximum of one other frame. Here it can be delayed for 2 frames.

What changes are you referring to, that we can change the limit? I'm not objecting (though I'm hesitant to change this PR - I prefer waiting until after I have the unit-tests in), just curious.

Changes like checking the queue size here and not in search_for_match. We can leave for now, until the unit-tests will be completed. Just need to make sure not to delay the frame for too long

OhadMeir · 2023-04-16T09:57:57Z

third-party/realdds/src/dds-metadata-syncer.cpp

+    key_frame & synced = _frame_queue.front();
+    auto & fh = synced.second;
+
+    metadata_type md = std::move( _metadata_queue.front().second );


Why do we need md and moving twice?
Can pop the queue after _on_frame_ready

You'll see in my next PR that we need to push/pop before any callbacks, since stuff can happen during the callbacks.

OhadMeir · 2023-04-16T11:49:16Z

unit-tests/dds/test-librs-connections.py

+        context = rs.context( '{"dds-domain":123,"dds-participant-name":"librs"}' )
+        # The DDS devices take time to be recognized and we just created the context; we
+        # should not see them yet!
+        test.check( len( context.query_devices( only_sw_devices )) != 2 )


I don't think that we need this check. You don't know the timing and discovery might have happened, especially if working in shared memory.

I thought you'd want it based on the original comments.
I agree, but the chances that it'll fail are slim.
Anyway, I commented out the line so it's still explained and there...

OhadMeir

Should add some more metadata tests - image after metadata, etc... but will be in future PRs

maloel added 21 commits April 9, 2023 13:53

working using unique_ptr

7e8eab3

add syncer reset on stream close

863edc6

added pyrealdds.video_stream.on_data_available

64d1658

fix syncer logic

593ecfa

debug msg update when publishing metadata

4176c11

move dds-trinsics out of dds-defines.h

eb92015

dds_time more compact; better pyrealdds repr

1e7a393

add timestamp to topics::device::image

532d6cd

revisions in syncer

1fede4e

syncer exposed in pyrealdds

8cbd9e7

improve shorten_json_string and the way it breaks

3d29028

remove annoying debug message in d400-private.cpp

0c86aa5

fix bug in add_frame_metadata

16044bd

add basic metadata tests (just the beginning)

6786f75

rsutils::json:: string args (since json requires it)

38d1cfc

optimize librealsense::get_string for metadata

c133616

remove unnecessary debug message in device-watcher

b24670f

split participants to client/server

73fbc14

improve test.remote; default timeout=5

ce2f25d

split test-metadata into metadata-server

5fc49a5

RSUSB for U20 dds testing

355aa5d

maloel requested a review from OhadMeir April 10, 2023 11:22

Nir-Az reviewed Apr 10, 2023

View reviewed changes

src/to-string.cpp Outdated Show resolved Hide resolved

maloel added 7 commits April 11, 2023 07:58

wait_for_device notification-based rather than sleep

2c3a7b7

log.f now in red, too

1fc6b3b

add test.closure; failed test cases now logged at end

fea0a6c

fix warnings

4e5c065

revise test-librs-connections

374f616

fix flushing issues (linux) in rspy.log

1d73c1c

disable test-metadata in Linux; indent remote

fe36ec5

maloel force-pushed the dds branch from ed15605 to fe36ec5 Compare April 11, 2023 04:59

maloel added 5 commits April 11, 2023 08:23

CR comments

a987ed0

switch test-metadata to use Timer

397d4f7

fix rspy.timer Stopwatch to be per instance!

c0fd66d

CR comments in to-string.cpp

00c9aa5

fix test.closure exception swallowing; on_fail constants

e440de3

OhadMeir reviewed Apr 11, 2023

View reviewed changes

per CR, remove 'return *this' in syncer on_xxx()

64dc4a1

OhadMeir reviewed Apr 16, 2023

View reviewed changes

OhadMeir approved these changes Apr 17, 2023

View reviewed changes

remove check in test-librs-connections, per CR

0a076ad

maloel merged commit 4359306 into IntelRealSense:dds Apr 17, 2023

maloel deleted the dds branch April 17, 2023 09:49

maloel mentioned this pull request Apr 30, 2023

remove image-msg::frame-id; use timestamp #11751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic dds metadata tests (just the beginning) #11665

basic dds metadata tests (just the beginning) #11665

maloel commented Apr 10, 2023 •

edited

Loading

OhadMeir Apr 11, 2023

maloel Apr 11, 2023

OhadMeir Apr 11, 2023

maloel Apr 13, 2023

OhadMeir Apr 16, 2023

maloel Apr 16, 2023

OhadMeir Apr 16, 2023 •

edited

Loading

maloel Apr 16, 2023

OhadMeir Apr 16, 2023

maloel Apr 16, 2023

OhadMeir Apr 16, 2023

maloel Apr 16, 2023

OhadMeir Apr 16, 2023

OhadMeir Apr 16, 2023

maloel Apr 16, 2023

OhadMeir Apr 16, 2023

maloel Apr 17, 2023

OhadMeir left a comment

basic dds metadata tests (just the beginning) #11665

basic dds metadata tests (just the beginning) #11665

Conversation

maloel commented Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhadMeir Apr 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhadMeir left a comment

Choose a reason for hiding this comment

maloel commented Apr 10, 2023 •

edited

Loading

OhadMeir Apr 16, 2023 •

edited

Loading