Improvements triggered by code review

examples/hdr/readme.md

@@ -83,7 +83,7 @@ pipe.start(cfg);
 Initializing the merging filter - will be further used in order to merge frames from both HDR sequence IDs that have been configured:
 ```cpp
 // initializing the merging filter
-rs2::merge_filter merging_filter;
+rs2::depth_merge merging_filter;
 ```
 After getting the frames, by using the wait_for_frames method, the merging filter is used:
 ```cpp
@@ -98,7 +98,7 @@ app.show(merged_frameset);
 Initializing also the spliting filter, with the requested sequence ID as 2:
 ```cpp
 // initializing the spliting filter
-rs2::split_filter spliting_filter;
+rs2::depth_split spliting_filter;
 // setting the required sequence ID to be shown
 spliting_filter.set_option(RS2_OPTION_SELECT_ID, 2);
 ```

examples/hdr/rs-hdr.cpp

@@ -95,10 +95,10 @@ int main(int argc, char * argv[]) try
     pipe.start(cfg);
 
     // initializing the merging filter
-    rs2::merge_filter merging_filter;
+    rs2::depth_merge merging_filter;
 
     // initializing the spliting filter
-    rs2::split_filter spliting_filter;
+    rs2::depth_split spliting_filter;
 
     // setting the required sequence ID to be shown
     spliting_filter.set_option(RS2_OPTION_SELECT_ID, 2);

include/librealsense2/h/rs_processing.h

@@ -252,14 +252,14 @@ rs2_processing_block* rs2_create_zero_order_invalidation_block(rs2_error** error
 rs2_processing_block* rs2_create_huffman_depth_decompress_block(rs2_error** error);
 
 /**
-* Creates a merge_filter processing block. 
+* Creates a depth_merge processing block. 
 * The block merges between two depth frames with different exposure values
 * \param[out] error  if non-null, receives any error that occurs during this call, otherwise, errors are ignored
 */
 rs2_processing_block* rs2_create_merge_processing_block(rs2_error** error);
 
 /**
-* Creates a split_filter processing block.
+* Creates a depth_split processing block.
 * The block reassigns separate profiles for the frames produced with sub-presets
 * \param[out] error  if non-null, receives any error that occurs during this call, otherwise, errors are ignored
 */

include/librealsense2/h/rs_types.h

@@ -191,8 +191,8 @@ typedef enum rs2_extension
     RS2_EXTENSION_TEMPORAL_FILTER,
     RS2_EXTENSION_HOLE_FILLING_FILTER,
     RS2_EXTENSION_ZERO_ORDER_FILTER,
-    RS2_EXTENSION_MERGE_FILTER,
-    RS2_EXTENSION_SPLIT_FILTER,
+    RS2_EXTENSION_DEPTH_MERGE,
+    RS2_EXTENSION_DEPTH_SPLIT,
     RS2_EXTENSION_RECOMMENDED_FILTERS,
     RS2_EXTENSION_POSE,
     RS2_EXTENSION_POSE_SENSOR,

include/librealsense2/hpp/rs_processing.hpp

@@ -1101,19 +1101,20 @@ namespace rs2
         }
     };
 
-    class merge_filter : public filter
+    class depth_merge : public filter
     {
     public:
         /**
-        * Create merge_filter processing block
-        * the processing perform the hole filling base on different hole filling mode.
+        * Create depth_merge processing block
+        * the processing merges between depth frames with 
+        * different sub-preset sequence ids.
         */
-        merge_filter() : filter(init()) {}
+        depth_merge() : filter(init()) {}
 
-        merge_filter(filter f) :filter(f)
+        depth_merge(filter f) :filter(f)
         {
             rs2_error* e = nullptr;
-            if (!rs2_is_processing_block_extendable_to(f.get(), RS2_EXTENSION_MERGE_FILTER, &e) && !e)
+            if (!rs2_is_processing_block_extendable_to(f.get(), RS2_EXTENSION_DEPTH_MERGE, &e) && !e)
             {
                 _block.reset();
             }
@@ -1135,19 +1136,19 @@ namespace rs2
         }
     };
 
-    class split_filter : public filter
+    class depth_split : public filter
     {
     public:
         /**
-        * Create split_filter processing block
+        * Create depth_split processing block
         * the processing perform the hole filling base on different hole filling mode.
         */
-        split_filter() : filter(init()) {}
+        depth_split() : filter(init()) {}
 
-        split_filter(filter f) :filter(f)
+        depth_split(filter f) :filter(f)
         {
             rs2_error* e = nullptr;
-            if (!rs2_is_processing_block_extendable_to(f.get(), RS2_EXTENSION_SPLIT_FILTER, &e) && !e)
+            if (!rs2_is_processing_block_extendable_to(f.get(), RS2_EXTENSION_DEPTH_SPLIT, &e) && !e)
             {
                 _block.reset();
             }

src/ds5/ds5-device.cpp

@@ -35,8 +35,8 @@
 #include "proc/hole-filling-filter.h"
 #include "proc/depth-formats-converter.h"
 #include "proc/depth-decompress.h"
-#include "proc/merge-filter.h"
-#include "proc/split-filter.h"
+#include "proc/depth-merge.h"
+#include "proc/depth-split.h"
 #include "hdr-config.h"
 #include "../common/fw/firmware-version.h"
 #include "fw-update/fw-update-unsigned.h"
@@ -955,6 +955,7 @@ namespace librealsense
                 make_attribute_parser(&md_configuration::sub_preset_info,
                     md_configuration_attributes::sub_preset_info_attribute, md_prop_offset ,
                 [](const rs2_metadata_type& param) {
+                        // bit mask and offset used to get data from bitfield
                         return (param & md_configuration::SUB_PRESET_BIT_MASK_SEQUENCE_SIZE)
                             >> md_configuration::SUB_PRESET_BIT_OFFSET_SEQUENCE_SIZE;
                     }));
@@ -963,6 +964,7 @@ namespace librealsense
                 make_attribute_parser(&md_configuration::sub_preset_info,
                     md_configuration_attributes::sub_preset_info_attribute, md_prop_offset ,
                 [](const rs2_metadata_type& param) {
+                        // bit mask and offset used to get data from bitfield
                         return (param & md_configuration::SUB_PRESET_BIT_MASK_SEQUENCE_ID) 
                             >> md_configuration::SUB_PRESET_BIT_OFFSET_SEQUENCE_ID;
                     })); 
@@ -971,6 +973,7 @@ namespace librealsense
                 make_attribute_parser(&md_configuration::sub_preset_info,
                     md_configuration_attributes::sub_preset_info_attribute, md_prop_offset,
                     [](const rs2_metadata_type& param) {
+                        // bit mask and offset used to get data from bitfield
                         return (param & md_configuration::SUB_PRESET_BIT_MASK_ID)
                             >> md_configuration::SUB_PRESET_BIT_OFFSET_ID;
                     }));
@@ -1145,8 +1148,8 @@ namespace librealsense
     processing_blocks get_ds5_depth_recommended_proccesing_blocks()
     {
         auto res = get_depth_recommended_proccesing_blocks();
-        res.push_back(std::make_shared<merge_filter>());
-        res.push_back(std::make_shared<split_filter>());
+        res.push_back(std::make_shared<depth_merge>());
+        res.push_back(std::make_shared<depth_split>());
         res.push_back(std::make_shared<threshold>());
         res.push_back(std::make_shared<disparity_transform>(true));
         res.push_back(std::make_shared<spatial_filter>());

src/hdr-config.cpp

@@ -26,13 +26,17 @@ namespace librealsense
 
         // restoring current HDR configuration if such subpreset is active
         command cmd(ds::GETSUBPRESET);
-        auto res = _hwm.send(cmd);
-
         bool existing_subpreset_restored = false;
-        if (res.size() && is_current_subpreset_hdr(res))
-        {
-            existing_subpreset_restored = configure_hdr_as_in_fw(res);
+        try {
+            auto res = _hwm.send(cmd);
+            if (res.size() && is_current_subpreset_hdr(res))
+            {
+                existing_subpreset_restored = configure_hdr_as_in_fw(res);
+            }
         }
+        catch (std::exception ex) {
+            LOG_WARNING("In hdr_config::hdr_config() - hw command failed: " << ex.what());
+        }        
 
         if (!existing_subpreset_restored)
         {
@@ -51,25 +55,20 @@ namespace librealsense
 
     bool hdr_config::is_current_subpreset_hdr(const std::vector<byte>& current_subpreset) const
     {
-        int current_subpreset_id = current_subpreset[1];
-        return is_hdr_id(current_subpreset_id);
+        bool result = false;
+        if (current_subpreset.size() > 0)
+        {
+            int current_subpreset_id = current_subpreset[1];
+            result = is_hdr_id(current_subpreset_id);
+        }
+        return result;
     }
 
     bool hdr_config::is_hdr_id(int id) const
     {
         return id >= 0 && id <= 3;
     }
 
-    int int_from_4_bytes_big_endian(const std::vector<byte>& vec, int start_offset)
-    {
-        int result = int((unsigned char)(vec[start_offset]) |
-            (unsigned char)(vec[start_offset + 1]) << 8 |
-            (unsigned char)(vec[start_offset + 2]) << 16 |
-            (unsigned char)(vec[start_offset + 3]) << 24);
-
-        return result;
-    }
-
     bool hdr_config::configure_hdr_as_in_fw(const std::vector<byte>& current_subpreset)
     {
         // parsing subpreset pattern, considering:
@@ -81,34 +80,40 @@ namespace librealsense
         const int size_of_control_id = 1;
         const int size_of_control_value = 4;
 
+        int subpreset_size = size_of_subpreset_header + 2 * (size_of_subpreset_item_header +
+            2 * (size_of_control_id + size_of_control_value));
+
+        if (current_subpreset.size() != subpreset_size)
+            return false;
+
         int offset = 0;
         offset += size_of_subpreset_header;
         offset += size_of_subpreset_item_header;
 
         if (current_subpreset[offset] != CONTROL_ID_EXPOSURE)
             return false;
         offset += size_of_control_id;
-        float exposure_0 = int_from_4_bytes_big_endian(current_subpreset, offset);
+        float exposure_0 = *reinterpret_cast<const uint32_t*>(&(current_subpreset[offset]));
         offset += size_of_control_value;
 
         if (current_subpreset[offset] != CONTROL_ID_GAIN)
             return false;
         offset += size_of_control_id;
-        float gain_0 = int_from_4_bytes_big_endian(current_subpreset, offset);
+        float gain_0 = *reinterpret_cast<const uint32_t*>(&(current_subpreset[offset]));
         offset += size_of_control_value;
 
         offset += size_of_subpreset_item_header;
 
         if (current_subpreset[offset] != CONTROL_ID_EXPOSURE)
             return false;
         offset += size_of_control_id;
-        float exposure_1 = int_from_4_bytes_big_endian(current_subpreset, offset);
+        float exposure_1 = *reinterpret_cast<const uint32_t*>(&(current_subpreset[offset]));
         offset += size_of_control_value;
 
         if (current_subpreset[offset] != CONTROL_ID_GAIN)
             return false;
         offset += size_of_control_id;
-        float gain_1 = int_from_4_bytes_big_endian(current_subpreset, offset);
+        float gain_1 = *reinterpret_cast<const uint32_t*>(&(current_subpreset[offset]));
         offset += size_of_control_value;
 
         _hdr_sequence_params[0]._exposure = exposure_0;
@@ -157,15 +162,15 @@ namespace librealsense
             }
             break;
         default:
-            throw invalid_value_exception("option is not an HDR option");
+            throw invalid_value_exception(to_string() << "option: " << rs2_option_to_string(option) << " is not an HDR option");
         }
         return rv;
     }
 
     void hdr_config::set(rs2_option option, float value, option_range range)
     {
         if (value < range.min || value > range.max)
-            throw invalid_value_exception(to_string() << "hdr_config::set(...) failed! value is out of the option range.");
+            throw invalid_value_exception(to_string() << "hdr_config::set(...) failed! value: "<< value << " is out of the option range: [" << range.min << ", " << range.max << "].");
 
         switch (option)
         {
@@ -233,9 +238,8 @@ namespace librealsense
                 // saving status of options that are not compatible with hdr,
                 // so that they could be reenabled after hdr disable
                 set_options_to_be_restored_after_disable();
-
-                send_sub_preset_to_fw();
-                _is_enabled = true;
+
+                _is_enabled = send_sub_preset_to_fw();
                 _has_config_changed = false;
             }
             else
@@ -294,11 +298,19 @@ namespace librealsense
         }
     }
 
-    void hdr_config::send_sub_preset_to_fw()
+    bool hdr_config::send_sub_preset_to_fw()
     {
+        bool result = false;
         // prepare sub-preset command
         command cmd = prepare_hdr_sub_preset_command();
-        auto res = _hwm.send(cmd);
+        try {
+            auto res = _hwm.send(cmd);
+            result = true;
+        }
+        catch (std::exception ex) {
+            LOG_WARNING("In hdr_config::send_sub_preset_to_fw() - hw command failed: " << ex.what());
+        }
+        return result;
     }
 
     void hdr_config::disable()
@@ -309,31 +321,15 @@ namespace librealsense
         // TODO - make it usable not only for ds - use _sensor
         command cmd(ds::SETSUBPRESET, static_cast<int>(pattern.size()));
         cmd.data = pattern;
-        auto res = _hwm.send(cmd);
-    }
-
-    //helper method - for debug only - to be deleted
-    std::string hdrchar2hex(unsigned char n)
-    {
-        std::string res;
-
-        do
-        {
-            res += "0123456789ABCDEF"[n % 16];
-            n >>= 4;
-        } while (n);
-
-        reverse(res.begin(), res.end());
-
-        if (res.size() == 1)
-        {
-            res.insert(0, "0");
+        try {
+            auto res = _hwm.send(cmd);
         }
-
-        return res;
+        catch (std::exception ex) {
+            LOG_WARNING("In hdr_config::disable() - hw command failed: " << ex.what());
+        }
+
     }
 
-
     command hdr_config::prepare_hdr_sub_preset_command() const
     {
         std::vector<uint8_t> subpreset_header = prepare_sub_preset_header();
@@ -346,19 +342,6 @@ namespace librealsense
             pattern.insert(pattern.end(), &subpreset_frames_config[0], &subpreset_frames_config[0] + subpreset_frames_config.size());
         }
 
-        /*std::cout << "pattern for hdr sub-preset: ";
-        for (int i = 0; i < pattern.size(); ++i)
-            std::cout << hdrchar2hex(pattern[i]) << " ";
-        std::cout << std::endl;
-
-        std::ofstream outFile("subpreset_bytes.txt", std::ofstream::out);
-        outFile << "pattern for hdr sub-preset: ";
-        for (int i = 0; i < pattern.size(); ++i)
-            outFile << hdrchar2hex(pattern[i]) << " ";
-        outFile << std::endl;*/
-
-        //uint8_t sub_preset_opcode = _sensor->get_set_sub_preset_opcode();
-        // TODO - make it usable not only for ds - use _sensor
         command cmd(ds::SETSUBPRESET, static_cast<int>(pattern.size()));
         cmd.data = pattern;
         return cmd;
@@ -459,7 +442,6 @@ namespace librealsense
 
     void hdr_config::set_exposure(float value)
     {
-        /* TODO - add limitation on max exposure to be below frame interval - is range really needed for this?*/
         _hdr_sequence_params[_current_hdr_sequence_index]._exposure = value;
         _has_config_changed = true;
     }

src/hdr-config.h

@@ -22,7 +22,9 @@ namespace librealsense
 
     inline bool operator==(const hdr_params& first, const hdr_params& second)
     {
-        return (first._exposure == second._exposure) && (first._gain == second._gain);
+        return (first._sequence_id == second._sequence_id) &&
+            (first._exposure == second._exposure) && 
+            (first._gain == second._gain);
     }
 
     class hdr_config
@@ -60,7 +62,7 @@ namespace librealsense
         void restore_options_after_disable();
 
         bool validate_config() const;
-        void send_sub_preset_to_fw();
+        bool send_sub_preset_to_fw();
         void disable();
         void set_id(float value);
         void set_sequence_size(float value);