-
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
DDS formats conversion #11850
DDS formats conversion #11850
Conversation
…nd motion. Some unit tests fail
…ltiple sensors, to avoid concurrent device discovery problem
00f8b67
to
2daa179
Compare
{ | ||
auto converters = dds_rs_internal_data::get_profile_converters( product_id, product_line ); | ||
_formats_converter.register_converters( converters ); | ||
_profiles = _formats_converter.get_all_possible_profiles( _raw_rs_profiles ); |
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.
What is _profiles
? For some reason I don't see it in the header?
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.
_profiles
is a member from the base software_device
.
_raw_rs_profiles
hold the librealsense profiles created from the received dds profiles. _profiles
holds the profiles created afterd conversion.
src/dds/rs-dds-internal-data.cpp
Outdated
tags.push_back( { RS2_STREAM_COLOR, -1, 1920, 1080, RS2_FORMAT_RGB8, 30, profile_tag::PROFILE_TAG_SUPERSET | profile_tag::PROFILE_TAG_DEFAULT } ); | ||
} | ||
else | ||
throw std::runtime_error( "Unsupported product id or product line" ); |
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.
I don't think this is good. We need to support any DDS device. Just because we don't recognize it doesn't mean we don't want to use it...
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.
OK, will add support by identity
processing blocks. Will show raw profiles as we received them.
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.
Done
formats.push_back( RS2_FORMAT_UYVY ); | ||
break; | ||
default: | ||
throw std::runtime_error( "Format is not supported for mapping" ); |
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 this throw? Don't we want an identity somehow? E.g., what if the source is rgb8
?
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.
This function is being called internally in this file only for YUYV and UYVY formats, to shorten the code line and make it more readable. For a format like rgb8
no converter is needed so the sensor proxy will create an identity converter for it.
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.
I still think we shouldn't throw in this code, but -- knowing that this code will be removed in the PR -- let's move on...
RS2_STREAM_COLOR ); | ||
for( auto & it : tmp ) | ||
factories.push_back( std::move( it ) ); | ||
tmp = processing_block_factory::create_pbf_vector< uyvy_converter >( RS2_FORMAT_YUYV, |
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 this be yuy2_converter
?
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.
This was copied from d400-color, without the comment about it being a workaround.
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.
But isn't it a bug? The source is YUYV, not UYVY... maybe it's a bug in the original, too...
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.
Looks like a bug: uyvy_converter
unpacks UYVY and yuy2_converter
unpacks YUYV. I.e., it looks like the converter name is the SOURCE format. If we know the source is YUYV, we should use the yuy2_converter
, right?
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.
Remi says it's intended: for D457, YUYV is reported but it's UYVY in actuality.
There may be a bug in the (original) code still: if it's UYVY and create_pbf_vector
is called with a source of YUYV, then we get a combination where source==dest (YUYV) which generates an identity block rather than UYVY-to-YUYV conversion.
I'm letting this go for this PR as this code will likely be removed soon -- but this might still be an issue for rs-dds-server
, since it'll send out UYVY as YUYV (meaning the DDS client will get the wrong format).
target_formats( RS2_FORMAT_YUYV ), | ||
RS2_STREAM_COLOR ); | ||
for( auto & it : tmp ) | ||
factories.push_back( std::move( it ) ); |
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.
Why not create directly in factories
?
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.
Since create_pbf_vector
returns more than one object to insert into factories
I cannot use emplace
. I think that moving using push_back
is better than copying using insert
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.
I meant 3 lines above, just assign to factories
instead of manually moving item by item.
// temporary solution, the camera should send all this data in a designated topic. | ||
factories.push_back( { { { RS2_FORMAT_MOTION_XYZ32F, RS2_STREAM_ACCEL } }, | ||
{ { RS2_FORMAT_MOTION_XYZ32F, RS2_STREAM_ACCEL } }, | ||
[]() { return std::make_shared< identity_processing_block >(); } } ); |
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.
Since the default is to have identity, no need to specifically mention IMU -- but this will be removed in next PR, right?
factories.push_back( { { { RS2_FORMAT_W10 } }, | ||
{ { RS2_FORMAT_Y10BPACK, RS2_STREAM_INFRARED, 1 } }, | ||
[]() { return std::make_shared< w10_converter >( RS2_FORMAT_Y10BPACK ); } } ); | ||
factories.push_back( { { { RS2_FORMAT_Y8I } }, |
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.
As discussed, DDS device-server should never send out Y8I or Y12I, or any other packed format -- the unpacking needs to happen on the server side and the data split into two streams. It's harmless so we may keep it but definitely need to place a comment documenting this in in the next PR.
@@ -209,6 +209,28 @@ dds_device_proxy::dds_device_proxy( std::shared_ptr< context > ctx, std::shared_ | |||
} | |||
} ); // End foreach_stream lambda | |||
|
|||
for( auto & sensor_info : sensor_name_to_info ) | |||
{ | |||
sensor_info.second.proxy->initialization_done( dev->device_info().product_id, dev->device_info().product_line ); |
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.
I expect both arguments to the function will go away: note that, even now, both are optional. In fact, I think we can consider removing product_line
, as well...
// When using DDS the server may split one stream of data into multiple dds_streams, e.g. Y8I | ||
// infrared data. When grouping these dds_streams under one sensor we get multiple instances of the | ||
// same profile, but the server should only open one to streaming | ||
if( is_profile_in_list( raw_profile, { current_resolved_reqs.begin(), current_resolved_reqs.end() } ) ) |
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 take a look if this code is necessary: as discussed, the adapter should do the Y8I split itself (for two Y8 streams) so the client would need open profiles for both.
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.
When changing the implementation, so that
- No two converters give the same output profile (as is the case now for Y8/Y8I profiles for index 1)
- No interlaced format will be sent
then this code could be removed
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.
So you'll remove in the next PR?
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.
Yes, in the next PR I will verify it is no longer needed and remove
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.
Removed in PR#11946
dds.video_stream_profile( 30, dds.stream_format("mono8"), 424, 240 ), | ||
dds.video_stream_profile( 15, dds.stream_format("mono8"), 424, 240 ), | ||
dds.video_stream_profile( 5, dds.stream_format("mono8"), 424, 240 ) | ||
dds.video_stream_profile( 25, dds.stream_format("Y12I"), 1288, 808 ), |
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.
These are per IR stream (left/right), so should technically be already de-interlaced?
std::shared_ptr<stream_profile_interface> add_video_stream(rs2_video_stream video_stream, bool is_default=false); | ||
std::shared_ptr<stream_profile_interface> add_motion_stream(rs2_motion_stream motion_stream, bool is_default = false); | ||
std::shared_ptr<stream_profile_interface> add_pose_stream(rs2_pose_stream pose_stream, bool is_default = false); | ||
virtual std::shared_ptr<stream_profile_interface> add_video_stream(rs2_video_stream video_stream, bool is_default = false); |
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.
If we're changing this, can't we pass the rs2_video_stream
as const &
?
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.
See comments, but overall good
Converting DDS frames from camera raw format into user friendly format.
The camera raw profiles and the converters used are LibRS only information. I collected data from the various
device
classes intors-dds-internal-data
class. We can consider sending the data in the discovery notifications instead, currently, I did not want to add this very LibRS specific data to realdds.