Add support for post processing filters #11566

OhadMeir · 2023-03-15T10:58:06Z

Enabling post processing filters for DDS devices.

Using filters name to create appropriate filters at the client side.

Nir-Az · 2023-03-19T12:29:25Z

src/context.cpp

+            if( filter_name.compare( "Depth Huffman Decoder" ) == 0 )
+                current_filters.add_processing_block( std::make_shared< depth_decompression_huffman >() );
+            else if( filter_name.compare( "Decimation Filter" ) == 0 )
+                //TODO - set options? might be needed according to stream type


Currently this does not support setting internal options?

Changed the comment - sensor.cpp sets format option based on sensor type, but the filter does not use it and selects the appropriate decimation algorithm based on processed frame profile format. No need to set the options

Nir-Az · 2023-03-19T12:45:40Z

src/context.cpp

    };

+    // For cases when checking if this is< color_sensor > (like realsense-viewer::subdevice_model)
+    class dds_color_sensor_proxy : public dds_sensor_proxy, public color_sensor


Check is we can add also depth_sensor
Can be on the todo list

Nir-Az · 2023-03-19T12:50:16Z

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

                else if( state_type::WAIT_FOR_DEVICE_OPTIONS == state && id == "device-options" )
                {
-                    LOG_DEBUG( "... device-options: " << j["n-options"] << " options received" );
+                    LOG_DEBUG( "... device-options: " << j["options"].size() << " options received" );


rsutils::json::has

Nir-Az · 2023-03-19T12:50:59Z

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

+                            filter_names.push_back( filter );
+                        }
+
+                        stream_it->second->set_recommended_filters( filter_names );


Consider std::move

Nir-Az · 2023-03-19T12:52:23Z

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

            }
        }
        server->init_options( options );
+        server->set_recommended_filters( filter_names );


Consider std::move

remibettan · 2023-03-19T13:00:11Z

src/context.cpp

            throw std::runtime_error( "Could not find a stream that supports option " + name );
        }

+        void add_processing_block( std::string filter_name )


This file starts to be really long. If I read it correctly, most of it is dds_sensor_proxy class - please consider to add new h and cpp files for it.

Already mentioned in a previous PR (about metadata support) that we intend to split the DDS to a different file, but because the changes we made broke the LibCI we address it first. After changes will be merged we will create a new PR to split this file.

remibettan · 2023-03-19T13:00:39Z

src/context.cpp

+            create_processing_block( filter_name );
+        }
+
+        bool processing_block_exists( processing_blocks const & blocks, std::string const & block_name )


Should this method be const?

remibettan · 2023-03-19T13:03:17Z

src/context.cpp

+        {
+            auto & current_filters = get_software_recommended_proccesing_blocks();
+
+            if( filter_name.compare( "Depth Huffman Decoder" ) == 0 )


Consider removing the "==0" and adding"!" instead. e.g:
if( !filter_name.compare( "Depth Huffman Decoder" ) )

I think that using ! is less readable, because compare returns 0 when it is true. !compare reads like it does not compare.

your style - ok

remibettan · 2023-03-19T13:04:13Z

src/context.cpp

    };

+    // For cases when checking if this is< color_sensor > (like realsense-viewer::subdevice_model)
+    class dds_color_sensor_proxy : public dds_sensor_proxy, public color_sensor


Consider exporting this class to separate h, cpp files

Will be moved in a future PR. See previous comments

remibettan · 2023-03-19T13:08:25Z

src/context.cpp

                {
                    // This is a new sensor we haven't seen yet
-                    sensor_info.proxy = std::make_shared< dds_sensor_proxy >( stream->sensor_name(), this, _dds_dev );
+                    if( stream->sensor_name().compare( "RGB Camera" ) == 0 )


Isn't it some member in the realdds::dds_stream class that can return the stream type (as RS2_STREAM_COLOR)?

No.
And we can't go over all the profiles and test if there is RGB8 one, because D405 has RGB8 but not a color_sensor.

Comparing sensor name to "RGB Camera" is also the way it was done in ROS wrapper.

remibettan

few comments - great work!

Nir-Az · 2023-03-20T07:52:08Z

@remibetta pending your approval

remibettan

LGTM

OhadMeir requested review from Nir-Az and remibettan March 15, 2023 10:58

OhadMeir marked this pull request as ready for review March 19, 2023 11:54

Nir-Az reviewed Mar 19, 2023

View reviewed changes

remibettan reviewed Mar 19, 2023

View reviewed changes

Nir-Az approved these changes Mar 20, 2023

View reviewed changes

remibettan self-requested a review March 20, 2023 08:50

remibettan approved these changes Mar 20, 2023

View reviewed changes

OhadMeir added 3 commits March 20, 2023 13:08

Add support for post processing filters

3cd9601

Identify color_sensor to support OpenVino face detection filter

63b2f10

Handle PR#11566 comments

adc94d7

OhadMeir force-pushed the dds branch from f78f2cd to adc94d7 Compare March 20, 2023 11:09

Nir-Az merged commit a4ea614 into realsenseai:dds Mar 20, 2023

maloel mentioned this pull request Mar 24, 2023

merge development -> dds #11603

Merged

maloel added a commit to maloel/librealsense that referenced this pull request Mar 24, 2023

remove Depth Huffman Decoder as it was removed in realsenseai#11566

35255b4

Add support for post processing filters #11566

Add support for post processing filters #11566

Uh oh!

Conversation

OhadMeir commented Mar 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remibettan left a comment

Choose a reason for hiding this comment

Uh oh!

Nir-Az commented Mar 20, 2023

Uh oh!

remibettan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants