Enable rotate frames for RGB camera #13733
Conversation
|
Please check CI failures |
src/proc/rotation-filter.cpp
Outdated
|
|
||
| auto src = f.as< rs2::video_frame >(); | ||
| rs2::stream_profile profile = f.get_profile(); | ||
| auto format = f.get_profile().format(); |
There was a problem hiding this comment.
We already have the profile from 1 line above, you can do profile.format() here right?
src/proc/rotation-filter.cpp
Outdated
| if( ! frame || frame.is< rs2::frameset >() ) | ||
| return false; | ||
| auto type = frame.get_profile().stream_type(); | ||
| /// change extrinsics |
| return false; | ||
| auto type = frame.get_profile().stream_type(); | ||
| /// change extrinsics | ||
| for( auto & stream_type : _streams_to_rotate ){ |
There was a problem hiding this comment.
No need for { } for one line if/for. Where you are using, { should be placed on a new line.
| register_option( RS2_OPTION_ROTATION, rotation_control ); | ||
| } | ||
|
|
||
| rotation_filter::rotation_filter( std::vector< rs2_stream > streams_to_rotate ) |
There was a problem hiding this comment.
Code duplication. Is default constructor needed? If so have it call the other constructor with an empty vector {}.
src/proc/rotation-filter.cpp
Outdated
| if( ! strong_rotation_control ) | ||
| return; | ||
|
|
||
| std::lock_guard< std::mutex > lock( _mutex ); |
There was a problem hiding this comment.
Lock after validating if valid value.
src/proc/rotation-filter.cpp
Outdated
|
|
||
| rs2::frame rotation_filter::process_frame(const rs2::frame_source& source, const rs2::frame& f) | ||
| { | ||
| if( _value == rotation_default_val ) |
There was a problem hiding this comment.
Should lock _mutex or place _value in a local variable and use it all over this function.
There was a problem hiding this comment.
Same for update_output_profile and rotate_frame.
src/proc/rotation-filter.cpp
Outdated
| { | ||
| if( _value == 90 || _value == -90 ) | ||
| { | ||
| LOG_ERROR( "Rotating YUYV format is disabled for 90 or -90 degrees" ); |
There was a problem hiding this comment.
Do we intend to support this at a future PR?
There was a problem hiding this comment.
No support for 90 and 270 degrees rotation for YUYV format
src/proc/rotation-filter.cpp
Outdated
| } | ||
|
|
||
|
|
||
| void rotation_filter::rotate_YUYV_frame(uint8_t* const out, const uint8_t* source, int width, int height) { |
There was a problem hiding this comment.
Please use formatter. Spaces, { at new line, etc...
src/proc/rotation-filter.cpp
Outdated
| break; | ||
| } | ||
| case 3: { | ||
| rotate_frame< 3 >( static_cast< uint8_t * >( const_cast< void * >( tgt.get_data() ) ), |
There was a problem hiding this comment.
Seems that using bpp as a normal function parameter instead of template will reduce code duplication.
src/proc/rotation-filter.cpp
Outdated
| if (auto tgt = prepare_target_frame(f, source, tgt_type)) | ||
| { | ||
| switch( bpp ) | ||
| if( format == RS2_FORMAT_YUYV && ( _value == 90 || _value == -90 ) ) |
There was a problem hiding this comment.
Should also use local_value
src/proc/rotation-filter.cpp
Outdated
| @@ -193,33 +188,64 @@ namespace librealsense { | |||
| int height_out = ( _value == 90 || _value == -90 ) ? width : height; // rotate by 180 will keep the values as is | |||
There was a problem hiding this comment.
Should use value, not the member
src/proc/rotation-filter.cpp
Outdated
|
|
||
| // Determine output index based on rotation angle | ||
| size_t out_index; | ||
| if( _value == 90 ) |
There was a problem hiding this comment.
Same in this if/else, use variable value
src/proc/rotation-filter.cpp
Outdated
| throw invalid_value_exception( rsutils::string::from() | ||
| << "Unsupported rotation scale " << val << " is out of range." ); | ||
|
|
||
| std::lock_guard< std::mutex > lock( _mutex ); |
There was a problem hiding this comment.
Can remove the lock, using local variable elsewhere in the file
Tracked on RSDSO-19999