-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Adding depth units value to frame metadata #9154
Conversation
src/sensor.cpp
Outdated
void uvc_sensor::set_depth_units(float value) | ||
{ | ||
_depth_units = value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applies to Depth sensor only. Don't put in the base class
Add comment - TODO for refactoring
src/proc/colorizer.cpp
Outdated
} | ||
} | ||
depth_units = ((depth_frame*)f.get())->get_units(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert the playback sensor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ev-mp I still think this part is redundant because depth_units is defined the same way regardless to playback condition, but I will add it for you to see
src/proc/colorizer.cpp
Outdated
@@ -289,7 +271,7 @@ namespace librealsense | |||
} | |||
}; | |||
|
|||
auto make_value_cropped_frame = [this](const rs2::video_frame& depth, rs2::video_frame rgb) | |||
auto make_value_cropped_frame = [this, depth_units](const rs2::video_frame& depth, rs2::video_frame rgb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieve the depth units from the depth frame
src/metadata.h
Outdated
@@ -375,6 +375,7 @@ namespace librealsense | |||
struct md_depth_control | |||
{ | |||
md_header header; | |||
uint32_t depth_units; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The metadata is part of FW API - remove it
src/proc/colorizer.cpp
Outdated
throw std::runtime_error( "failed to query depth units from sensor" ); | ||
} | ||
} | ||
depth_units = ((depth_frame*)f.get())->get_units(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be checked externally
src/proc/pointcloud.cpp
Outdated
@@ -253,8 +245,8 @@ namespace librealsense | |||
{ | |||
auto res = allocate_points(source, depth); | |||
auto pframe = (librealsense::points*)(res.get()); | |||
|
|||
const float3* points = depth_to_points(res, *_depth_intrinsics, depth, *_depth_units); | |||
auto depth_units = ((depth_frame*)depth.get())->get_units(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if the member should be updated instead of local
src/sensor.h
Outdated
@@ -98,7 +98,8 @@ namespace librealsense | |||
frame_timestamp_reader* timestamp_reader, | |||
const rs2_time_t& last_timestamp, | |||
const unsigned long long& last_frame_number, | |||
std::shared_ptr<stream_profile_interface> profile); | |||
std::shared_ptr<stream_profile_interface> profile, | |||
float depth_units = -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use default values
src/media/ros/ros_writer.cpp
Outdated
@@ -190,6 +190,7 @@ namespace librealsense | |||
image.header.stamp = rs2rosinternal::Time(std::chrono::duration<double>(timestamp_ms).count()); | |||
std::string TODO_CORRECT_ME = "0"; | |||
image.header.frame_id = TODO_CORRECT_ME; | |||
image.header.depth_units = static_cast<float>(vid_frame->get_frame_depth_units()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to be retrieved using librealsense:: methods only
float get_frame_depth_units() const | ||
{ | ||
rs2_error* e = nullptr; | ||
auto r = rs2_depth_frame_get_units(frame_ref, &e); | ||
error::handle(e); | ||
return r; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To remove
src/archive.h
Outdated
@@ -41,6 +41,7 @@ namespace librealsense | |||
bool is_blocking = false; // when running from recording, this bit indicates | |||
// if the recorder was configured to realtime mode or not | |||
// if true, this will force any queue receiving this frame not to drop it | |||
float depth_units = 0.0f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment that this is a temporal solution
src/archive.h
Outdated
float get_frame_depth_units() const override | ||
{ | ||
return first()->get_frame_depth_units(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To remove
src/archive.h
Outdated
@@ -134,6 +137,7 @@ namespace librealsense | |||
rs2_timestamp_domain get_frame_timestamp_domain() const override; | |||
void set_timestamp(double new_ts) override { additional_data.timestamp = new_ts; } | |||
unsigned long long get_frame_number() const override; | |||
float get_frame_depth_units() const override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To remove
src/gl/colorizer-gl.cpp
Outdated
@@ -202,7 +202,7 @@ namespace librealsense | |||
_width = vp.width(); _height = vp.height(); | |||
|
|||
auto info = disparity_info::update_info_from_frame(f); | |||
_depth_units = info.depth_units; | |||
_depth_units = ((depth_frame*)f.get())->get_units(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be performed each iteration - move outside of the profile update clause
src/proc/colorizer.cpp
Outdated
@@ -235,32 +235,13 @@ namespace librealsense | |||
|
|||
rs2::frame colorizer::process_frame(const rs2::frame_source& source, const rs2::frame& f) | |||
{ | |||
_depth_units = ((depth_frame*)f.get())->get_units(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check frame type before dereferencing
src/proc/disparity-transform.h
Outdated
@@ -82,6 +82,7 @@ namespace librealsense | |||
librealsense::depth_stereo_sensor* dss; | |||
auto info = disparity_info::info(); | |||
float stereo_baseline_meter; | |||
info.depth_units = ((depth_frame*)f.get())->get_units(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify frame's type is depth
src/proc/synthetic-stream.cpp
Outdated
@@ -331,6 +331,7 @@ namespace librealsense | |||
data.metadata_size = 0; | |||
data.system_time = _actual_source.get_time(); | |||
data.is_blocking = original->is_blocking(); | |||
data.depth_units = original->get_frame_depth_units(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if required
@@ -1092,7 +1092,7 @@ namespace librealsense | |||
pose_frame_metadata frame_md = { 0 }; | |||
frame_md.arrival_ts = duration_cast<std::chrono::nanoseconds>(ts.arrival_ts).count(); | |||
|
|||
frame_additional_data additional_data(ts.device_ts.count(), frame_num++, ts.arrival_ts.count(), sizeof(frame_md), (uint8_t*)&frame_md, ts.global_ts.count(), 0, 0, false); | |||
frame_additional_data additional_data(ts.device_ts.count(), frame_num++, ts.arrival_ts.count(), sizeof(frame_md), (uint8_t*)&frame_md, ts.global_ts.count(), 0, 0, false, 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if can be dropped - no need to explicitly set for T265
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should stay, the API with default depth_units=-1 is not related to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual dependency injection is not clear yet.
For review
src/proc/synthetic-stream.cpp
Outdated
// For old playback sensors | ||
if (!((depth_frame*)f.get())->get_units()) | ||
{ | ||
auto snr = ((frame_interface*)f.get())->get_sensor().get(); | ||
auto depth_sensor = As< librealsense::depth_sensor >(snr); | ||
auto extendable = As< librealsense::extendable_interface >(snr); | ||
if (extendable && extendable->extend_to(TypeToExtension< librealsense::depth_sensor >::value, (void**)(&depth_sensor))) | ||
{ | ||
auto du = depth_sensor->get_depth_scale(); | ||
((depth_frame*)f.get())->set_units(du); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DU injection should occur during the frame construction in uvc_sensor::open(
or playback
src/sensor.cpp
Outdated
// generate additional data | ||
float depth_units = 0; | ||
if(_on_frame) | ||
_on_frame(depth_units);//_additional_data.depth_units; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback should get the additional_data
handle and modify its content
src/sensor.h
Outdated
@@ -30,6 +30,7 @@ namespace librealsense | |||
class option; | |||
|
|||
typedef std::function<void(std::vector<platform::stream_profile>)> on_open; | |||
typedef std::function<void(float &val)> on_frame_md; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should receive a reference to additional_data
and modify its content
src/sensor.h
Outdated
|
||
std::vector<platform::stream_profile> _internal_config; | ||
|
||
std::atomic<bool> _is_streaming; | ||
std::atomic<bool> _is_opened; | ||
std::shared_ptr<notifications_processor> _notifications_processor; | ||
on_open _on_open; | ||
on_frame_md _on_frame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to metadata_modifier
for clarity
src/sensor.h
Outdated
@@ -353,6 +354,10 @@ namespace librealsense | |||
return action(*_device); | |||
} | |||
|
|||
void update_params(on_frame_md callback) override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be called set_metadata_modifier
or similar
src/archive.h
Outdated
void set_units(float depth_units) | ||
{ | ||
additional_data.depth_units = depth_units; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be needed
src/media/ros/ros_writer.cpp
Outdated
@@ -190,7 +190,7 @@ namespace librealsense | |||
image.header.stamp = rs2rosinternal::Time(std::chrono::duration<double>(timestamp_ms).count()); | |||
std::string TODO_CORRECT_ME = "0"; | |||
image.header.frame_id = TODO_CORRECT_ME; | |||
image.header.depth_units = static_cast<float>(((depth_frame*)vid_frame)->get_units()); | |||
image.depth_units = static_cast<float>(((depth_frame*)vid_frame)->get_units()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's review the record/playback mechanims
@@ -238,6 +242,14 @@ namespace serialization | |||
stream.next(m.is_bigendian); | |||
stream.next(m.step); | |||
stream.next(m.data); | |||
try | |||
{ | |||
stream.next(m.depth_units); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a header that should be used to differentiate according to version number
os << "Encoding : " << image->encoding << std::endl; | ||
os << "Width : " << image->width << std::endl; | ||
os << "Height : " << image->height << std::endl; | ||
os << "Step : " << image->step << std::endl; | ||
//os << "Frame Number : " << image->header.seq << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the comment
src/ds5/ds5-device.cpp
Outdated
{ | ||
_metadata_modifier = callback; | ||
auto s = get_raw_sensor().get(); | ||
As< librealsense::uvc_sensor >(s)->modify_frame_metadata(callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to set_frame_metadata_modifier
src/ds5/ds5-device.cpp
Outdated
void set_depth_scale(float val) | ||
{ | ||
_depth_units = val; | ||
modify_frame_metadata([&](frame_additional_data& data) {data.depth_units = _depth_units.load(); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check _depth_units lifetime
@@ -202,7 +204,6 @@ namespace librealsense | |||
_width = vp.width(); _height = vp.height(); | |||
|
|||
auto info = disparity_info::update_info_from_frame(f); | |||
_depth_units = info.depth_units; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the field depth_units from disparity_info
src/media/ros/ros_writer.cpp
Outdated
std::string TODO_CORRECT_ME = "0"; | ||
image.header.frame_id = TODO_CORRECT_ME; | ||
std::string NEW_ROSBAG = "1"; | ||
image.header.frame_id = NEW_ROSBAG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to version + comment for maintainers
src/media/ros/ros_writer.cpp
Outdated
@@ -188,8 +188,9 @@ namespace librealsense | |||
image.header.seq = static_cast<uint32_t>(vid_frame->get_frame_number()); | |||
std::chrono::duration<double, std::milli> timestamp_ms(vid_frame->get_frame_timestamp()); | |||
image.header.stamp = rs2rosinternal::Time(std::chrono::duration<double>(timestamp_ms).count()); | |||
std::string TODO_CORRECT_ME = "0"; | |||
image.header.frame_id = TODO_CORRECT_ME; | |||
std::string NEW_ROSBAG = "1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for it
src/media/ros/ros_writer.cpp
Outdated
image.header.frame_id = TODO_CORRECT_ME; | ||
std::string NEW_ROSBAG = "1"; | ||
image.header.frame_id = NEW_ROSBAG; | ||
image.depth_units = static_cast<float>(((depth_frame*)vid_frame)->get_units()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be dynamically checked
s << indent << "depth_units: "; | ||
Printer<float>::stream(s, indent + " ", v.depth_units); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print only for depth frame
@@ -281,8 +281,11 @@ struct Printer< ::sensor_msgs::Image_<ContainerAllocator> > | |||
s << indent << " data[" << i << "]: "; | |||
Printer<uint8_t>::stream(s, indent + " ", v.data[i]); | |||
} | |||
s << indent << "depth_units: "; | |||
Printer<float>::stream(s, indent + " ", v.depth_units); | |||
if (v.depth_units) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for floating_point != zero explicitly using numeric_limits
@@ -44,7 +44,7 @@ struct Header_ | |||
_stamp_type stamp; | |||
|
|||
typedef std::basic_string<char, std::char_traits<char>, typename ContainerAllocator::template rebind<char>::other > _frame_id_type; | |||
_frame_id_type frame_id; | |||
_frame_id_type version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide comment for maintainers
src/ds5/ds5-device.cpp
Outdated
{ | ||
_metadata_modifier = callback; | ||
auto s = get_raw_sensor().get(); | ||
As< librealsense::uvc_sensor >(s)->modify_frame_metadata(callback); | ||
As< librealsense::uvc_sensor >(s)->set_frame_metadata_modifier(callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that the extension conversion is not null before dereferencing
src/media/ros/ros_writer.cpp
Outdated
@@ -207,8 +208,7 @@ namespace librealsense | |||
imu_msg.header.seq = static_cast<uint32_t>(frame.frame->get_frame_number()); | |||
std::chrono::duration<double, std::milli> timestamp_ms(frame.frame->get_frame_timestamp()); | |||
imu_msg.header.stamp = rs2rosinternal::Time(std::chrono::duration<double>(timestamp_ms).count()); | |||
std::string TODO_CORRECT_ME = "0"; | |||
imu_msg.header.frame_id = TODO_CORRECT_ME; | |||
imu_msg.header.version = "1"; // used to distinguish between old rosbag and new rosbag that contains depth units in frame metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment that the field is unused and therefore assigned for ROSbag versions control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the last comment
@@ -281,7 +281,7 @@ struct Printer< ::sensor_msgs::Image_<ContainerAllocator> > | |||
s << indent << " data[" << i << "]: "; | |||
Printer<uint8_t>::stream(s, indent + " ", v.data[i]); | |||
} | |||
if (v.depth_units) | |||
if (v.depth_units != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use std::numeric_limits<float>::min()
as in original comment
Track on DSO-16066