Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

abstract post-processing filter factory via context #12425

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Nov 19, 2023

Today, if you want to return "recommended processing blocks" (AKA recommended filters in the API), then you need to return the actual instantiation of the blocks.
This means that everyone needs to know how to instantiate them: the rosbag reader; the DDS device; etc.
The problem is that, if we add new filters (or remove old ones), the code needs to be updated everywhere.
The problem is that there's no abstraction of a factory that can instantiate a filter by name, that would allow a piece of code to know about the factory rather than the block implementations.

This adds such a factory, with the goal of separating the DDS/ROS/whatever code from post-processing (which may not even exist!).

  • the factory interface is called pp_block_factory
    • pp == post-processing, to denote these are used for post-processing
    • kept the block designation because it returns processing_block_interface
    • there is already an internal processing_block_factory that's used for more-internal non-filter stuff, so I couldn't call it what it should be called
  • the factory is implemented by rscore_pp_block_factory which the context knows about
  • added context::create_pp_block() - the context will know what factories exist and how to use them; nobody else should
  • create_pp_block() accepts JSON settings that can be used to serialize settings; these aren't used right now
    • rosbag files do not record any PP settings
    • DDS doesn't communicate those but that should be easy to communicate
    • persistent context settings are pending PR and could be used easily, too

Tracked on [LRS-967]

@maloel maloel requested a review from Nir-Az November 19, 2023 11:30
// These filters do not accept settings (nor are settings recorded in ros_writer)
(void *)&settings;

if( rsutils::string::nocase_equal( name, "Decimation Filter" ) )
Copy link
Collaborator

@Nir-Az Nir-Az Nov 24, 2023

Choose a reason for hiding this comment

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

How is this code better than before?
Performance is worse (comparing all strings)
If you make a string mistake it compile but dont work

Magic strings are used (no header that group all)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code compared strings before, too.

Yes, before they were "hard-coded" in that they were generated from the extension names. But we don't want to use the extension names. If you prefer, I can compare block names and fall back to "automatic" extension name comparison which was case-sensitive, but I don't think that's readable or easy to understand at all.

This is the factory. It centralizes the knowledge of processing blocks, and lets us add processing blocks dynamically in different modules. I like this much better than having extension names spread all over the place, many times not in any human-preferable way ("Hdr Merge" rather than "HDR Merge").

I can change to fit your preference, but the factory idea is much better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as performance, I can change to a map if you like. But we compared all strings before, too. This happens once during device creation.

@maloel maloel merged commit 5a2749b into IntelRealSense:development Nov 29, 2023
15 of 17 checks passed
@maloel maloel deleted the pb-factory-2 branch November 29, 2023 09:46
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.

2 participants