Enable metadata with a single click on Windows by dorodnic · Pull Request #5678 · realsenseai/librealsense

dorodnic · 2020-01-19T15:52:54Z

UVC frame metadata on Windows is causing a lot of questions and confusion, adding a single click solution that would be applicable from within the Viewer.
Alternative solution is to download and install Intel-RealSense-D400-Series-Universal-Windows-Platform-UWP-Driver-for-Windows but from our experience a lot of users don't do this.
Related to #5669

maloel · 2020-01-20T13:40:11Z

+                  "software synchronization between different\n"
+                  "camera streams.\n"
+                  "It must be explicitly enabled on Windows OS\n";
+        last_progress_time = system_clock::now();


Is draw_progress_bar() called at all? Where is this last_progress_time being used? If not used, is the inheritance from process_notification_model needed? Maybe inherit directly from notification_model?

maloel · 2020-01-20T13:41:08Z

+        enable_dismiss = true;
+        update_state = 0;
+        pinned = true;
+        message = "Frame Metadata is device feature allowing\n"


a device feature

maloel · 2020-01-20T14:01:28Z

+
+    bool operator==(const device_id& a, const device_id& b)
+    {
+        return (to_lower(a.pid) == to_lower(b.pid) &&


strcasecmp() would compare just as well without all the extra allocations/transformations

maloel · 2020-01-20T14:13:57Z

+    public:
+        static bool parse_device_id(std::string id, device_id* res)
+        {
+            static const std::regex regex("[\\s\\S]*pid_([0-9a-fA-F]+)&mi_([0-9]+)#[0-9]&([0-9a-fA-F]+)&[\\s\\S]*\\{([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})\\}[\\s\\S]*");


Isn't "[\s\S]" the same as "."? And is not needed at the beginning/end of the regex if we use regex_search() instead of regex_match()?
If we're using a-fA-F, then no need to use to_lower(), and then can pass the string in with 'const &'?

Isn't "[\s\S]" the same as "."?

Apparently no, I had problems matching special symbols with .*

And is not needed at the beginning/end of the regex if we use regex_search() instead of regex_match()

Make sense, I'll give it a try

If we're using a-fA-F, then no need to use to_lower()

Ultimately, to_lower is only used for comparisons. If we switch all comparisons to strcasecmp as you suggested it may be redundant

Isn't "[\s\S]" the same as "."?

Apparently no, I had problems matching special symbols with .*

According to this, ". doesn't catch line terminators (like new line) by default" but I don't think that would cover your difficulties. I wonder what kind of special characters...

But OK, keep it at [/s/S] but please comment so the next wise-guy like me won't try to "optimize" :)

maloel · 2020-01-20T14:16:22Z

+            return false;
+        }
+
+        static void do_foreach_device(std::vector<device_id> devices, 


Pass devices by 'const &'

maloel · 2020-01-21T09:09:19Z

+                            NULL,
+                            NULL,
+                            &ftLastWriteTime);
+                        if (retCode == ERROR_SUCCESS)


In case of errors, do we usually LOG_DEBUG() it?
What about the successes? Do we want to debug-log any of this so we can see what happened?

maloel · 2020-01-21T09:22:34Z

+                                    if (rdid == did)
+                                    {
+                                        std::wstringstream ss;
+                                        ss << ws << "\\" << ke << "\\#GLOBAL\\Device Parameters";


I think you can build the whole key path here instead of building ws ahead of time, so it'll be:

ss << "SYSTEM\\CurrentControlSet\\Control\\DeviceClasses\\{" << guid << "}" << "\\" << ke << "\\#GLOBAL\\Device Parameters";

That way it's clearer (and ws doesn't get re-defined inside this block).

maloel · 2020-01-21T09:23:03Z

+                                    {
+                                        std::wstringstream ss;
+                                        ss << ws << "\\" << ke << "\\#GLOBAL\\Device Parameters";
+                                        auto s = ss.str();


Isn't this a wstring? Why do we need ws on the next line?

maloel · 2020-01-21T09:45:22Z

+        // Heuristic that determines how many media-pins UVC device is expected to have
+        static int number_of_mediapins(std::string pid, std::string mi)
+        {
+            if (mi == "00")


This whole function is very confusing unless you know things by heart.

Isn't there a way to programmatically get all these magic numbers (which could conceivably change in the future)?
If not programmatically, isn't there a header somewhere with constants that will make this both more understandable and easier to maintain?
The way I understand it, "media pins" is the same as the streams that a device sensor exposes?

maloel · 2020-01-21T09:57:10Z

+                    std::wstring wcmd(cmd_line.begin(), cmd_line.end());
+                    sei.lpParameters = wcmd.c_str();
+
+                    ShellExecuteEx(&sei); // not checking return code - what you are going to do? retry? why?


At the very least, I think this should be logged somehow?

maloel · 2020-01-21T10:27:33Z

+        }
+
+        static void do_foreach_device(std::vector<device_id> devices, 
+            std::function<void(device_id&, std::wstring)> action)


It's not clear what the wstring is representing... it's a device path, right? Can we name the argument? There might be an issue with the compiler (it's ambiguous in the standard, so depends on the compiler)...

Maybe name the function 'foreach_device_path'?

The device_id should be passed as 'const &'.

maloel · 2020-01-21T10:29:43Z

+                            DWORD MetadataBufferSizeInKB = 5;
+                            RegSetValueEx(key, metadatakey.c_str(), 0, REG_DWORD,
+                                (const BYTE*)&MetadataBufferSizeInKB, sizeof(DWORD));
+                            // What to do if failed???


Log it, at the very least.

maloel · 2020-01-21T10:31:41Z

+        // Enable metadata for all connected devices
+        virtual void enable_metadata() { }
+
+        static std::string get_command_line_param() { return "--enable_metadata"; }


// This is the command-line parameter that gets passed to another process (running as admin) in order to enable metadata -- see WinMain

maloel · 2020-01-21T10:33:09Z

+        virtual bool is_enabled(std::string id) const { return true; }
+
+        // Enable metadata for all connected devices
+        virtual void enable_metadata() { }


Technically, it could fail, so I'd suggest to return a bool, even if the implementation always "succeeds"

maloel · 2020-01-21T10:33:50Z

+        if (s == rs2::metadata_helper::get_command_line_param())
+        {
+            rs2::metadata_helper::instance().enable_metadata();
+            exit(0);


Exit 1 on failure (if you change the return value)

maloel · 2020-01-21T10:40:39Z

+    public:
+        static metadata_helper& instance();
+
+        // Check if metadata is enabled using Physical Port ID


I think it'd help readability if we gave an example of a port ID string here, and explained is can be retrieved with device::get_info(RS2_CAMERA_INFO_PHYSICAL_PORT)

maloel · 2020-01-21T10:47:00Z

+            std::string product = dev.get_info(RS2_CAMERA_INFO_PRODUCT_LINE);
+            std::string id = dev.get_info(RS2_CAMERA_INFO_PHYSICAL_PORT);
+            bool has_metadata = rs2::metadata_helper::instance().is_enabled(id) ||
+                !(product == "D400" || product == "SR300" || product == "L500");


Can we abstract these product names into the metadata_helper class, such that we can simply ask:

auto & metadata = rs2::metadata_helper::instance(); bool has_metadata = metadata.is_enabled( dev );

That way, the product names will be local to the helper class and this information is contained centrally.

…h to not forget to merge

maloel · 2020-01-23T06:02:04Z

                            (LPBYTE)(&MetadataBufferSizeInKB),
-                            &len);
+                            &len) != ERROR_SUCCESS)
+                            rs2::log(RS2_LOG_SEVERITY_DEBUG, "Unable to read metadata registry key!");


Might want to include the key path in the message, otherwise it lacks context and will be repetitive if it happens multiple times

maloel · 2020-01-23T06:05:07Z

+        }
+
+        static void foreach_device_path(const std::vector<device_id>& devices,
+            std::function<void(device_id&,  /* matched device */


Any special reason device_id is not const?

maloel · 2020-01-23T06:11:02Z

+        {
+            try
+            {
+                DebugBreak();


Might want to remove :)

maloel · 2020-01-23T06:13:21Z

+
+        if (ImGui::IsItemHovered())
+        {
+            ImGui::SetTooltip("%s", "You will be prompted to enabled metadata on the device");


"to enable" (there's an extra d in there)

May I suggest:

"Enables metadata on connected devices (you may be prompted for administrator privileges)"

maloel · 2020-01-23T06:21:34Z

+        // (can be retrieved with device::get_info(RS2_CAMERA_INFO_PHYSICAL_PORT))
        virtual bool is_enabled(std::string id) const { return true; }

        // Enable metadata for all connected devices


Please add that it will throw a runtime_error on failure

maloel

Looks good, see comments above

dorodnic added the Windows label Jan 19, 2020

Option to enable metadata with single click on Windows

a3307ad

dorodnic force-pushed the install_metadata branch from 576c072 to a3307ad Compare January 20, 2020 09:00

maloel self-requested a review January 20, 2020 09:31

maloel reviewed Jan 20, 2020

View reviewed changes

dorodnic mentioned this pull request Jan 20, 2020

Wrong Color frame relative to Depth #5675

Closed

maloel reviewed Jan 21, 2020

View reviewed changes

Comment thread common/metadata-helper.cpp Resolved

maloel reviewed Jan 21, 2020

View reviewed changes

Comment thread common/windows-app-bootstrap.cpp Resolved

maloel reviewed Jan 21, 2020

View reviewed changes

dorodnic added 2 commits January 22, 2020 06:46

Code-review fixes

26e9b75

Re-enable back light compensation for the D465 - adding to this branc…

2264818

…h to not forget to merge

maloel reviewed Jan 23, 2020

View reviewed changes

Comment thread common/windows-app-bootstrap.cpp Outdated

{

try

{

DebugBreak();

maloel Jan 23, 2020

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Might want to remove :)

maloel reviewed Jan 23, 2020

View reviewed changes

More code review fixes

7ab4121

maloel approved these changes Jan 23, 2020

View reviewed changes

dorodnic merged commit 6006739 into realsenseai:development Jan 23, 2020

Conversation

dorodnic commented Jan 19, 2020

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!

maloel Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maloel Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

maloel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maloel Jan 21, 2020 •

edited

Loading

maloel Jan 21, 2020 •

edited

Loading