Open streams working for all stream types + some bug fixes #11045

OhadMeir · 2022-10-30T19:35:23Z

Updated the PR, merge with Eran's PRs, addressed issues raised in comments and added functionality

rs-dds-server handles "open-streams" control messages
Created dds-subscriber
Added type to profile (needed by various RS tools and examples)
Added type-object support to dds-topic
Fixed some bugs

maloel · 2022-10-31T05:59:52Z

third-party/realdds/include/realdds/dds-stream-profile.h

@@ -83,18 +83,20 @@ class dds_stream_profile
 {
    dds_stream_uid _uid;
    dds_stream_format _format;
-    int16_t _frequency;  // "Frames" per second
+    int16_t _frequency; // "Frames" per second
+    int8_t _inner_type; // User defined type


I don't quite understand the need here.
If really necessary, the "type" is the "stream type" and not the profile type. All profiles of the same stream will have the same type, no? I.e., it should be communicated with the stream and not the profile.

RS applications like rs-enumerate-devices and rs-sensor-control use rs2_profile type field to print a "stream name" for each profile, and we need to pass this information to them.

I agree that this is a duplication that can be avoided by sending a stream type. Currently in notificaiton-msg it is sent in the profile struct, and I didn't want to change that so we won't have a difficult merge.

I will take the information to the stream from the first profile in the message assuming all profiles have the same type (true for libRS, not sure that true in the general case)

So approve my PR and sending a type as part of the stream-header will be simple :)

By the way -- I don't think the type should be an argument to the stream, but a subtype of the stream: i.e., a dds_gyro_stream_server derives from dds_motion_stream_server, etc...

Then we will have to add types to realdds if LibRS adds a type. realdds doesn't really care about the type, it just need to relay it to the user

But the type does change things, and can change functionality.
Right now it's divided into "video" and "motion" but if we need more, then more types are needed.
Doing this thru polymorphism is the right way to do it -- if it's important. I'm still unsure that it is (what depends on the stream type?) -- let's talk about it.

third-party/realdds/src/dds-topic.cpp

third-party/realdds/src/dds-subscriber.cpp

maloel · 2022-10-31T06:13:34Z

third-party/realdds/src/dds-device-server.cpp

@@ -176,6 +179,11 @@ void dds_device_server::init( std::vector< std::shared_ptr< dds_stream_server >
        else
            DDS_THROW( runtime_error, "unexpected stream '" + stream->name() + "' type" );
    }
+
+    // Create a control server and set callback
+    _control_server = std::make_shared< dds_control_server >( _subscriber, _topic_root + "/control" );


Seems weird: why is the device server creating a control server? It's a control client -- the server is on the client side... i.e., the "server" is the "publisher", not the subscriber...

The "Server" term refers to the camera side, as opposed to the "client" or "host".
"Server" does imply publisher. Many server only handles incoming requests.

No, let's stick to one terminology: a server "serves" data (i.e., sends), a client receives. Otherwise it's confusing.

I think that your terminology is confusing. A server just have services that handle client requests.

And what is the difference than between "broadcaster" and "server"? They both send data.

I didn't make the terminology. A server serves data, whether it's web pages or anything else.
The "broadcaster" needed distinguishing in its name somehow, and we decided on broadcaster which you're right, also serves data. But it's understood from the name.
But here you have something that consumes data and serves nothing. Why call it a server?

I don't agree with you here.
All the -server classes are on the "server" side (as in rs-dds-server tool) to distinguish from the "host" side.
dds-device-server vs dds-device, dds-stream-server vs dds-stream.
Keeping this notation helps to orient in the code and distinguish the class purpose.

@Nir-Az - what do you think?

This argument might be redundant, as mentioned in another conversation this class might later be refactored out...

I agree that server should not be confused and places at the client side.
server is the node that sends frames, it a camera server.
It will add more confusion to call the client server.

We are creating here a client for controlling the camera server and getting frames..
It does help distinguish between sides IMO.

maloel · 2022-10-31T06:21:36Z

third-party/realdds/include/realdds/dds-control-server.h

+
+// A control is a command sent from a client (camera user) and the server (this class) receives and interprets.
+//
+class dds_control_server


From my understanding of the usage, this is not really a server but a client -- a listener for control messages.

If so -- then how is this different than a topic-reader? Do you envision more logic being placed here? I don't see much logic belonging in realdds other than high-level commands (stream start/stop, control get/set) that would still need customization by the end implementation.

I think that we will need more logic to handle stuff like query-reply pairs and msg-id matching.
If it will all be in the application than you are right and we can refactor this out

I think it'll be where the control messages are handled.
Most controls will be handled in the app and only a few will be done entirely in realdds and even then it'll likely be in the device-server. If there's a lot of code we can certainly push to another class but at this time I don't see much justification.

third-party/realdds/src/dds-control-server.cpp

…into dds

…into dds # Conflicts: # third-party/realdds/src/dds-stream-server.cpp

third-party/realdds/include/realdds/dds-device-server.h

third-party/realdds/include/realdds/dds-stream-server.h

maloel · 2022-11-13T06:25:48Z

third-party/realdds/src/dds-device-server.cpp

@@ -41,17 +44,34 @@ dds_device_server::~dds_device_server()
 }


-void dds_device_server::start_streaming( const std::string & stream_name,
-                                         const image_header & header )
+void dds_device_server::start_streaming( const std::vector< std::pair < std::string, image_header > > & streams_to_open )


As I said in our discussion:
I think this should not be a command (to the server or the stream) but rather the direct result of someone subscribing to the stream. The application needs to be notified when this happens (because it does the streaming).
Not for this PR.

Let's speak about it later on.
I think it's better to stick to USB style use case.
Open --> start stream
Start --> connect a callback

I think it's better to stick to USB style use case.

Not sure I agree.
There's no meaning to providing a callback in DDS, therefore start is meaningless. And open is just too generic.
As I said elsewhere:
I don't think there should be an open or start_streaming at all: this is DDS. When someone subscribes to a stream, the server should automatically start given the set profile. Same with stopping.

third-party/realdds/src/dds-device-server.cpp

maloel · 2022-11-13T06:35:04Z

third-party/realdds/src/dds-device-server.cpp

+        if ( !data.is_valid() )
+            continue;
+
+        _control_dispatcher.invoke( [&]( dispatcher::cancellable_timer ) { handle_control_message( data ); } );


Need to be really careful here:
data is local and will likely go out of scope by the time the dispatcher handles things. A crash is very likely eventually. The & will pass it to the lambda by ref which is no good -- we need to move it to the lambda.

invoke is moving when enqueuing

It moves the lambda, with all its contexts. But if the lambda contains a reference, the moved-to lambda will still contain the same reference to the original object, which sits on the stack.

Apparently, moving captures is not supported by C++11
Will copy the data, it is bound at 4K.

maloel · 2022-11-13T06:38:22Z

third-party/realdds/src/dds-device-server.cpp

+
+void dds_device_server::handle_control_message( topics::flexible_msg control_message )
+{
+    auto const & j = control_message.json_data();


json_data returns a json object, but not a reference. This should be putting up a warning?

gcc issued a warning when I used a non-const reference

It shouldn't be a reference.

Won't it be more efficient?

If int foo() { return 5; },
Can you do int & x = foo();?
You cannot assign a reference to a temporary variable. If the compiler does not generate a warning, the performance will still be the same.

third-party/realdds/src/dds-device-server.cpp

third-party/realdds/src/dds-notification-server.cpp

maloel · 2022-11-13T06:41:38Z

third-party/realdds/src/dds-notification-server.cpp

+    // Does not allocate for every sample but still gives flexibility. See:
+    //     https://github.com/eProsima/Fast-DDS/discussions/2707
+    // (default is PREALLOCATED_MEMORY_MODE)
+    wqos.endpoint().history_memory_policy = eprosima::fastrtps::rtps::PREALLOCATED_WITH_REALLOC_MEMORY_MODE;


This is the default

tools/dds/dds-server/rs-dds-server.cpp

maloel · 2022-11-13T06:47:45Z

tools/dds/dds-server/lrs-device-watcher.h

@@ -14,8 +14,8 @@ class lrs_device_watcher : public std::enable_shared_from_this<lrs_device_watche
 public:
    lrs_device_watcher( rs2::context &_ctx );
    ~lrs_device_watcher();
-    void run( std::function< void( rs2::device ) > add_device_cb,
-              std::function< void( rs2::device ) > remove_device_cb );
+    void run( std::function< void( rs2::device && ) > add_device_cb,


Any special reason? rs2::device has a shared_ptr inside so it just does a reference count

Did not realize it was just hiding a shared_ptr and thought it would be more efficient. Will roll back the changes

tools/dds/dds-server/lrs-device-controller.h

tools/dds/dds-server/lrs-device-controller.cpp

maloel · 2022-11-13T06:55:15Z

tools/dds/dds-server/lrs-device-controller.cpp

-// and stopping/closing on destruction
-class lrs_device_controller::lrs_sensor_streamer
+
+std::shared_ptr< dds_stream_profile > create_dds_stream_profile( rs2_stream type, nlohmann::json const & j )


I'm not clear why we're creating new profiles rather than finding an existing profile that's part of the server streams?

Can be an improvement on the tool. Not for this PR

maloel · 2022-11-13T06:57:52Z

tools/dds/dds-server/lrs-device-controller.cpp

+    throw std::runtime_error( "Unsupported stream type" );
+}
+
+rs2_stream stream_name_to_type( std::string const & type_string )


Shouldn't it be stream_type_to_rs2? I.e., why is it dependent on the stream name and not the stream type?

Not for this PR. Will change when profiles are stored in stream_server

Not sure what you mean... the profiles are there.

maloel · 2022-11-13T07:03:08Z

tools/dds/dds-server/lrs-device-controller.cpp

+    //corresponding to the requested, we have found our sensor.
+    for ( sensor_index = 0; sensor_index < _sensors.size(); ++sensor_index )
+    {
+        for ( auto & profile : _sensors[sensor_index].get_stream_profiles() )


Why not do a sensor.get_info( RS2_CAMERA_INFO_NAME ) to get its name?
We could do this once and store a map of sensor-to-index, even in the ctor...

I forget how confusing it is...
You're looking for a stream name, not a sensor name.
Still, I think we need to be extra careful here: stopping a stream is not the same as stopping a sensor. I.e., you can close the IR streams but leave Depth running. I'd even make that a unit-test.

You are right that stopping a stream should not, theoretically, stop other streams, but this is not the current use case.
This tool is giving LibRS users the behavior that they expect from a USB camera, and the LibRS API does not enable to stop just a stream, but a whole sensor.
In the future we may decide to change this behavior, but not for this PR.
(Plus it would really change if we'll open/close streams based on readers availability)

I'm not saying you should take care of this "use-case" in this specific tool. But the API and the triggers for everything need to be understood and acted on now, not later.

The way we open/close streams is in realdds and the camera-host communication, not the tool. Here we need to envision all use-cases and make sure the API is suitable for multiple readers if needed.

Don't be limited by the use-case.

third-party/realdds/src/dds-notification-server.cpp

maloel · 2022-11-14T12:46:40Z

tools/dds/dds-server/rs-dds-server.cpp

@@ -2,13 +2,17 @@
 // Copyright(c) 2022 Intel Corporation. All Rights Reserved.

 #include <librealsense2/utilities/easylogging/easyloggingpp.h>
+#include <librealsense2/utilities/json.h>


Not sure you need these here any more (same for flexible-msg, using json)

maloel

Just those 2 small comments... otherwise good, as discussed.

OhadMeir added 8 commits October 30, 2022 17:01

Create dds_subscriber and dds_control_server

f64defa

Add UYVY support to dds_stream_profile

e23f732

Fix control topic type

634aa7b

Add dynamic type support for dds-topic

471e6f2

Fix rs-dds-sniffer reader history mode

63c9544

Fix dds-device-server motion profile type

cf0e141

Add type information to dds_stream_profile

cab9905

Fix bug with group name at dds_device_server

a655f4d

OhadMeir requested a review from maloel October 30, 2022 19:35

maloel reviewed Oct 31, 2022

View reviewed changes

third-party/realdds/src/dds-topic.cpp Show resolved Hide resolved

maloel reviewed Oct 31, 2022

View reviewed changes

third-party/realdds/src/dds-subscriber.cpp Outdated Show resolved Hide resolved

maloel reviewed Oct 31, 2022

View reviewed changes

third-party/realdds/src/dds-control-server.cpp Outdated Show resolved Hide resolved

maloel reviewed Oct 31, 2022

View reviewed changes

third-party/realdds/src/dds-control-server.cpp Outdated Show resolved Hide resolved

OhadMeir force-pushed the dds branch from 2e8b4f3 to e7f0b59 Compare October 31, 2022 09:51

Type in stream not profile and other PR#11045 comments

7d6185e

OhadMeir force-pushed the dds branch from e7f0b59 to 7d6185e Compare October 31, 2022 12:04

OhadMeir mentioned this pull request Oct 31, 2022

move notification out of device; more generic; uses json #11042

Merged

Roll back stream type changes (will get from Eran's PR)

6493c65

OhadMeir force-pushed the dds branch from a9bf07e to 6493c65 Compare November 6, 2022 10:27

OhadMeir added 8 commits November 6, 2022 16:16

Merge branch 'dds' of https://github.com/IntelRealSense/librealsense …

ac6f8bb

…into dds

rs-dds-server open streams logic (not tested)

cf39725

Use dds-topic-reader not dds-control-server

f7dbb75

rs-dds-server streams upon request (not automatically)

189ea50

Merge branch 'dds' of https://github.com/IntelRealSense/librealsense …

9890c59

…into dds # Conflicts: # third-party/realdds/src/dds-stream-server.cpp

support open all stream types

afe730e

rs-dds-server open all kinds of streams

4f9f8a5

Fixed bug in rs-dds-server - second participant opened

4480f74

OhadMeir requested a review from Nir-Az November 10, 2022 09:15

Fix for linux compilation

cc05f8c

OhadMeir force-pushed the dds branch from 9959dc3 to cc05f8c Compare November 10, 2022 10:11

OhadMeir added 2 commits November 10, 2022 12:56

Implement close-streams

a2f5fe2

lrs-device-controller holds device streams

f57ffa1