-
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
One Viewer Context #5623
One Viewer Context #5623
Conversation
7c50995
to
c5a8d7d
Compare
@dorodnic, this compiles now and passes the unit-tests tests locally. I don't think the one remaining CI failure is related to this PR. |
@@ -354,11 +354,11 @@ namespace rs2 | |||
stbi_image_free(r); | |||
} | |||
|
|||
ux_window::ux_window(const char* title) : | |||
ux_window::ux_window(const char* title, context &ctx) : |
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.
@radfordi , can you please provide the rationale for this PR ?
I don't think that it could a performance issue. And if not - then the PR may suggests that multiple context instances are somehow incompatible with a certain HW setup which in turn implies that there is an underlying issue that need to be resolved.
To this points there were no specific limitations on the number of ctx objects except for T265, so adding the constrain is at least a potential compatibility break.
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 ran into the multiple context
s when trying to test @bfulkers-i' hotplug PR (#5492). It's annoying/challenging to debug hotplug in the context
in realsense-viewer
because we keep creating/deleting context
s every 200ms! The (longer term) goal of that PR is to move all polling, checking and looping (with sleeps), to the OS level and to make the realsense-viewer
(and tm_boot
) to rely on the device changed notification instead.
It's not a large performance issue currently because the viewer is such a resource hog as it is, but this is small step toward making it less so and making the device detection use hotplug only.
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.
Also, T265's can't currently be detected in multiple context
s at the same time. This should be fixed, but in the mean time it's trivial to have a single context and avoid the issue.
Avoid creating multiple unused
rs2::context
s.