-
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
Fix metadata #11567
Fix metadata #11567
Conversation
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 good. Can add a test setting multiple fields, say 5.
And a test that opens multiple sensors setting different values and verifying no problems from the shared parsers.
Later on when we will use erase or clear (in the DDS) we should also verify that erasing in one sensor does not affect the other 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.
Can add a test setting multiple fields, say 5.
And a test that opens multiple sensors setting different values and verifying no problems from the shared parsers.
You want these in this PR?
Later on when we will use erase or clear (in the DDS) we should also verify that erasing in one sensor does not affect the other sensor.
That's assuming we'll add an API to erase metadata :) so should be part of the task that does that.
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.
Can add a test setting multiple fields, say 5.
And a test that opens multiple sensors setting different values and verifying no problems from the shared parsers.You want these in this PR?
Yes. We should validate 2 sensors don't interfere for this 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.
See new commit.
unit-tests/sw-dev/sw.py
Outdated
w = 640 | ||
h = 480 | ||
bpp = 2 # bytes | ||
pixels = bytearray( b'\x00' * ( w * h * 2 )) # Dummy data |
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.
better to use bpp
s value instead of 2
. Does this support formats like Y8 or 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.
Right, but this was copy-paste from sw.py inside unit-tests/syncer/
. It should be able to support any format (like the DDS frames) but right now no content is actually written there...
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
unit-tests/sw-dev/test-metadata.py
Outdated
rgb.set( rs.frame_metadata_value.actual_fps, 0xf00d ) | ||
stereo.set( rs.frame_metadata_value.actual_fps, 0xbaad ) | ||
c1 = rgb.publish( color.frame() ) | ||
test.check_false( f1.supports_frame_metadata( rs.frame_metadata_value.actual_fps )) |
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.
Did you mean d1
?
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 think so...f1
is from one of the previous tests... :(
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.
Fixed
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.
LGTM
So it turns out that we cannot count on sensor-based parsers to keep validity of frame-based metadata (obviously), which is what happened after #11551.
So this brings in the validity back into the
metadata_blob
. It's still all array-based and fast, at the cost of a slightly bigger blob.Add unit-tests for it. The last test failed before the fix; now passes.