-
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
image quality test - adding comparison to laser off if depth is detected #12449
Conversation
@@ -172,10 +172,20 @@ def is_depth_meaningful(config, laser_enabled=True, save_image=False, show_image | |||
################################################################################################ | |||
|
|||
test.start("Testing depth frame - laser ON -", dev.get_info(rs.camera_info.name)) | |||
res, laser_black_pixels = is_depth_meaningful(cfg, laser_enabled=True, save_image=DEBUG_MODE, show_image=DEBUG_MODE) | |||
res = 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.
res / result is confusing, please rename and add comments
test.check(res is True) | ||
test.finish() | ||
|
||
################################################################################################ | ||
|
||
if res is True: |
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 do we condition 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 figured that if we have no depth in the frame, there is no point in comparing it to the frame without the laser, because the frame might be mostly black pixels anyway
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.
And the previous will fail?
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.
The comparison? It might fail, yes, not guaranteed but it can happen, but I don't think it indicates anything.
I can remove the condition if needed.
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 want to make sure we don't get test-pass
when actually nothing worked as expected :)
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 res is false we will get test failed
, if it is true, we will do the second test and see if it passes
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 add a comment explaining it
res, laser_black_pixels = is_depth_meaningful(cfg, laser_enabled=True, save_image=DEBUG_MODE, show_image=DEBUG_MODE) | ||
res = False | ||
max_black_pixels = float('inf') | ||
for i in range(5): |
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 add a cooment why we do this loop and maybe use a const global variable for FRAMES_TO_CHECK = 5
?
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 it really help? The 5 loop?
Do you think it is stable?
We can increase to 30
But we need it 100% stable
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.
It can help, mostly it helps if there's no depth found, but it's best to find the frame with the least black pixels anyway.
From what I've seen it is stable - if there is no depth the test will fail, if there is, they both should pass
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, lets monitor it and see if it's stable
Tracked on [LRS-902]