DDS formats conversion #11850

OhadMeir · 2023-05-30T07:49:10Z

Converting DDS frames from camera raw format into user friendly format.

The camera raw profiles and the converters used are LibRS only information. I collected data from the various device classes into rs-dds-internal-data class. We can consider sending the data in the discovery notifications instead, currently, I did not want to add this very LibRS specific data to realdds.

…nd motion. Some unit tests fail

…ltiple sensors, to avoid concurrent device discovery problem

src/dds/rs-dds-internal-data.h

src/dds/rs-dds-internal-data.cpp

src/dds/rs-dds-device-proxy.cpp

src/dds/rs-dds-sensor-proxy.h

src/dds/rs-dds-sensor-proxy.cpp

maloel · 2023-06-05T05:53:39Z

src/dds/rs-dds-sensor-proxy.cpp

+{
+    auto converters = dds_rs_internal_data::get_profile_converters( product_id, product_line );
+    _formats_converter.register_converters( converters );
+    _profiles = _formats_converter.get_all_possible_profiles( _raw_rs_profiles );


What is _profiles? For some reason I don't see it in the header?

_profiles is a member from the base software_device.
_raw_rs_profiles hold the librealsense profiles created from the received dds profiles. _profiles holds the profiles created afterd conversion.

maloel · 2023-06-05T05:55:52Z

src/dds/rs-dds-internal-data.cpp

+        tags.push_back( { RS2_STREAM_COLOR, -1, 1920, 1080, RS2_FORMAT_RGB8, 30, profile_tag::PROFILE_TAG_SUPERSET | profile_tag::PROFILE_TAG_DEFAULT } );
+    }
+    else
+        throw std::runtime_error( "Unsupported product id or product line" );


I don't think this is good. We need to support any DDS device. Just because we don't recognize it doesn't mean we don't want to use it...

OK, will add support by identity processing blocks. Will show raw profiles as we received them.

unit-tests/dds/test-librs-formats-converison.py

unit-tests/dds/formats-conversion-server.py

unit-tests/dds/d435i.py

src/dds/rs-dds-internal-data.cpp

src/dds/rs-dds-device-proxy.cpp

maloel · 2023-06-13T06:45:17Z

src/dds/rs-dds-internal-data.cpp

+        formats.push_back( RS2_FORMAT_UYVY );
+        break;
+    default:
+        throw std::runtime_error( "Format is not supported for mapping" );


Should this throw? Don't we want an identity somehow? E.g., what if the source is rgb8?

This function is being called internally in this file only for YUYV and UYVY formats, to shorten the code line and make it more readable. For a format like rgb8 no converter is needed so the sensor proxy will create an identity converter for it.

I still think we shouldn't throw in this code, but -- knowing that this code will be removed in the PR -- let's move on...

maloel · 2023-06-13T06:54:27Z

src/dds/rs-dds-internal-data.cpp

+                                                                                 RS2_STREAM_COLOR );
+            for( auto & it : tmp )
+                factories.push_back( std::move( it ) );
+            tmp = processing_block_factory::create_pbf_vector< uyvy_converter >( RS2_FORMAT_YUYV,


Shouldn't this be yuy2_converter?

This was copied from d400-color, without the comment about it being a workaround.

But isn't it a bug? The source is YUYV, not UYVY... maybe it's a bug in the original, too...

Looks like a bug: uyvy_converter unpacks UYVY and yuy2_converter unpacks YUYV. I.e., it looks like the converter name is the SOURCE format. If we know the source is YUYV, we should use the yuy2_converter, right?

Remi says it's intended: for D457, YUYV is reported but it's UYVY in actuality.

There may be a bug in the (original) code still: if it's UYVY and create_pbf_vector is called with a source of YUYV, then we get a combination where source==dest (YUYV) which generates an identity block rather than UYVY-to-YUYV conversion.

I'm letting this go for this PR as this code will likely be removed soon -- but this might still be an issue for rs-dds-server, since it'll send out UYVY as YUYV (meaning the DDS client will get the wrong format).

maloel · 2023-06-13T06:55:23Z

src/dds/rs-dds-internal-data.cpp

+                                                                                 target_formats( RS2_FORMAT_YUYV ),
+                                                                                 RS2_STREAM_COLOR );
+            for( auto & it : tmp )
+                factories.push_back( std::move( it ) );


Why not create directly in factories?

Since create_pbf_vector returns more than one object to insert into factories I cannot use emplace. I think that moving using push_back is better than copying using insert

I meant 3 lines above, just assign to factories instead of manually moving item by item.

maloel · 2023-06-13T09:01:22Z

src/dds/rs-dds-internal-data.cpp

+        // temporary solution, the camera should send all this data in a designated topic.
+        factories.push_back( { { { RS2_FORMAT_MOTION_XYZ32F, RS2_STREAM_ACCEL } },
+                               { { RS2_FORMAT_MOTION_XYZ32F, RS2_STREAM_ACCEL } },
+                               []() { return std::make_shared< identity_processing_block >(); } } );


Since the default is to have identity, no need to specifically mention IMU -- but this will be removed in next PR, right?

maloel · 2023-06-13T09:03:37Z

src/dds/rs-dds-internal-data.cpp

+        factories.push_back( { { { RS2_FORMAT_W10 } },
+                               { { RS2_FORMAT_Y10BPACK, RS2_STREAM_INFRARED, 1 } },
+                               []() { return std::make_shared< w10_converter >( RS2_FORMAT_Y10BPACK ); } } );
+        factories.push_back( { { { RS2_FORMAT_Y8I } },


As discussed, DDS device-server should never send out Y8I or Y12I, or any other packed format -- the unpacking needs to happen on the server side and the data split into two streams. It's harmless so we may keep it but definitely need to place a comment documenting this in in the next PR.

maloel · 2023-06-13T09:07:57Z

src/dds/rs-dds-device-proxy.cpp

@@ -209,6 +209,28 @@ dds_device_proxy::dds_device_proxy( std::shared_ptr< context > ctx, std::shared_
            }
        } );  // End foreach_stream lambda

+    for( auto & sensor_info : sensor_name_to_info )
+    {
+        sensor_info.second.proxy->initialization_done( dev->device_info().product_id, dev->device_info().product_line );


I expect both arguments to the function will go away: note that, even now, both are optional. In fact, I think we can consider removing product_line, as well...

src/dds/rs-dds-internal-data.cpp

maloel · 2023-06-13T09:38:12Z

src/proc/formats-converter.cpp

+                    // When using DDS the server may split one stream of data into multiple dds_streams, e.g. Y8I
+                    // infrared data. When grouping these dds_streams under one sensor we get multiple instances of the
+                    // same profile, but the server should only open one to streaming
+                    if( is_profile_in_list( raw_profile, { current_resolved_reqs.begin(), current_resolved_reqs.end() } ) )


Please take a look if this code is necessary: as discussed, the adapter should do the Y8I split itself (for two Y8 streams) so the client would need open profiles for both.

When changing the implementation, so that

No two converters give the same output profile (as is the case now for Y8/Y8I profiles for index 1)

No interlaced format will be sent
then this code could be removed

So you'll remove in the next PR?

Yes, in the next PR I will verify it is no longer needed and remove

Removed in PR#11946

maloel · 2023-06-13T09:43:52Z

unit-tests/dds/d405.py

-        dds.video_stream_profile( 30, dds.stream_format("mono8"), 424, 240 ),
-        dds.video_stream_profile( 15, dds.stream_format("mono8"), 424, 240 ),
-        dds.video_stream_profile( 5,  dds.stream_format("mono8"), 424, 240 )
+        dds.video_stream_profile( 25, dds.stream_format("Y12I"),  1288, 808 ),


These are per IR stream (left/right), so should technically be already de-interlaced?

unit-tests/dds/d455.py

unit-tests/dds/test-librs-device-properties.py

unit-tests/dds/test-librs-formats-converison.py

maloel · 2023-06-13T11:29:02Z

src/software-device.h

-        std::shared_ptr<stream_profile_interface> add_video_stream(rs2_video_stream video_stream, bool is_default=false);
-        std::shared_ptr<stream_profile_interface> add_motion_stream(rs2_motion_stream motion_stream, bool is_default = false);
-        std::shared_ptr<stream_profile_interface> add_pose_stream(rs2_pose_stream pose_stream, bool is_default = false);
+        virtual std::shared_ptr<stream_profile_interface> add_video_stream(rs2_video_stream video_stream, bool is_default = false);


If we're changing this, can't we pass the rs2_video_stream as const &?

maloel

See comments, but overall good

OhadMeir marked this pull request as ready for review June 1, 2023 07:17

OhadMeir requested a review from maloel June 1, 2023 07:17

OhadMeir added 14 commits June 1, 2023 16:18

trigger build

f26e59c

DDS formats conversion at host, first working version

e04fc73

Activated conversion in start. Handling multiple streams per sensor a…

d295f39

…nd motion. Some unit tests fail

CLEANer code (extracted to functions)

43395e6

Fix test-dds-librs-device-properties

8387927

Fix test-dds-librs-extrinsics

319740e

Fix test-stream-sensor-bridge

4988217

Add formats conversion unit test

8a6e6de

Trigger build

246daed

Trigger build

61b98cc

Update formats conversion unit test to create a single device with mu…

8eb143c

…ltiple sensors, to avoid concurrent device discovery problem

Trigger build

29d7fbc

Trigger build

2e0c3f0

Trigger build

2daa179

OhadMeir force-pushed the dds_formats_conversion branch from 00f8b67 to 2daa179 Compare June 1, 2023 13:28

OhadMeir changed the title ~~DDS formats conversion (+ merge with development)~~ DDS formats conversion Jun 1, 2023