Skip to content

Rotate_depth_and_ir_frames_together #13940

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

Merged

Conversation

noacoohen
Copy link
Contributor

No description provided.

@noacoohen noacoohen requested a review from Nir-Az April 15, 2025 07:26
@Nir-Az Nir-Az requested a review from OhadMeir April 15, 2025 20:00
std::map< std::pair< rs2_stream, int >, rs2::stream_profile > _target_stream_profiles;
std::map< std::pair< rs2_stream, int >, rs2::stream_profile > _source_stream_profiles;
// Define a simple structure to hold rotated dimensions
struct RotatedDims
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's align to the sdk naming convention
rotated_dims

tgt_type);
std::pair< rs2_stream, int > stream_key( f.get_profile().stream_type(), f.get_profile().stream_index() );

if( _rotated_dimensions.find( stream_key ) == _rotated_dimensions.end() )
Copy link
Collaborator

Choose a reason for hiding this comment

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

On which case will this happen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should not happen but I think it's OK to test.
There are cases where say update_output_profile returns without setting _rotated_dimensions, better to make sure we are aligned.

Copy link
Contributor

Choose a reason for hiding this comment

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

@noacoohen, now that I think of it, do we really need _rotated_dimensions? I think the information is stored in _target_stream_profiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OhadMeir removed it.

@Nir-Az
Copy link
Collaborator

Nir-Az commented Apr 16, 2025

@OhadMeir / @noacoohen I played with it a bit and it looks OK.
@OhadMeir please review the code, and let's try to merge it if no bugs inside otherwise it needs retesting again.

@@ -46,6 +46,8 @@ namespace librealsense {
if( ! strong_rotation_control )
return;

std::lock_guard< std::mutex > lock( _mutex );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this lock here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nir suggested adding the lock — we figured, similar to the decimation filter, there might be cases where we need to ensure thread safety when updating _value

Copy link
Contributor

Choose a reason for hiding this comment

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

We removed it on the last PR. We use a local copy of _value elsewhere to avoid locking.
Also, locking only here will not help, you need to lock on every use of the protected resource (_value in this case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

rs2::frame rotation_filter::prepare_target_frame( const rs2::frame & f,
const rs2::frame_source & source,
const rs2::stream_profile & target_profile,
rs2_extension tgt_type )
{
auto vf = f.as<rs2::video_frame>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to test cast success

@OhadMeir OhadMeir merged commit 3f252d9 into IntelRealSense:development Apr 20, 2025
25 checks passed
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.

3 participants