Skip to content

change metadata_blob to fit ALL metadata; software_sensor now uses array indexing - #11551

Merged
maloel merged 5 commits into
realsenseai:developmentfrom
maloel:metablob
Mar 12, 2023
Merged

maloel merged 5 commits into
realsenseai:developmentfrom
maloel:metablob

Conversation

@maloel

@maloel maloel commented Mar 9, 2023 •

Copy link
Copy Markdown
Contributor
  • blob no longer limited to 255 bytes, so can fit all metadata
  • meaning we can remove the extra metadata map
  • added an array-based (the constant type is still used but only in the ros reader, which I didn't want to touch) metadata parser
  • made use of it in the software sensor
  • this should also speed up metadata access (set/get) with software sensors
  • fix HUGE issue with the metadata count
  • added clear_metadata()
  • software structs are now passed by reference

Tracked on [LRS-585]

@maloel
maloel requested a review from OhadMeir March 9, 2023 17:12
Comment thread src/metadata-parser.h
RS2_FRAME_METADATA_HW_TYPE = RS2_FRAME_METADATA_COUNT +1 , /**< 8-bit Module type: RS4xx, IVCAM*/
RS2_FRAME_METADATA_SKU_ID , /**< 8-bit SKU Id*/
RS2_FRAME_METADATA_FORMAT , /**< 16-bit Frame format*/
RS2_FRAME_METADATA_WIDTH , /**< 16-bit Frame width. pixels*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not needed? d400-device uses this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ohh... I see you moved it to types.h

Comment thread src/frame.h
uint32_t metadata_size = 0;
bool fisheye_ae_mode = false; // TODO: remove in future release
std::array< uint8_t, MAX_META_DATA_SIZE > metadata_blob;
std::array< uint8_t, RS2_FRAME_METADATA_ACTUAL_COUNT * sizeof( rs2_metadata_type ) > metadata_blob;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use an array of rs2_metadata_type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because that's just one usage for software sensors. Most time it's binary data.

Comment thread src/software-device.cpp Outdated
void software_sensor::set_metadata(rs2_frame_metadata_value key, rs2_metadata_type value)
void software_sensor::set_metadata( rs2_frame_metadata_value key, rs2_metadata_type value )
{
if( ! _metadata_parsers->count( key ) )

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should test this. If setting same key again overwrite the value. This is also the current behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's necessary. But I have a better way that will be faster, I think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is it necessary? If a user wants to override a value that was previously set he should be able to do so without clearing the whole table first

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's necessary because now the only thing saying that the metadata exists is the presence of the parser. Why do you think it requires clearing the table first? The second time the parser would already exist and only the map is changed directly.

Comment thread src/software-device.cpp
throw invalid_value_exception( "invalid metadata key " + std::to_string( int( key ) ) );
// At this time (and therefore for backwards compatibility) no register_metadata is required for SW sensors,
// and metadata persists between frames (!!!!!!!) unless clear_metadata() is used...
_metadata_parsers->emplace_hint( range.second, key, std::make_shared< md_array_parser >( key ) );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For DDS devices make_shared will be called repeatedly for every metadata field on every frame.
We should allocate the parsers only once at start up and reference them on each relevant frame.

@maloel maloel Mar 12, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is development.
For DDS I already put in a erase_metadata function that would make only unset if a value isn't present, instead of the current clear_metadata.
For this PR, it only inserts a "parser" if it wasn't there before, so only the first time this function is called per metadata.

@OhadMeir OhadMeir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@maloel
maloel merged commit ba550b0 into realsenseai:development Mar 12, 2023
@maloel
maloel deleted the metablob branch March 12, 2023 09:22
This was referenced Mar 13, 2023
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.

2 participants