Skip to content
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

Close camera streams on robot close and when there are no more listeners #122

Merged
merged 4 commits into from
Oct 3, 2023

Conversation

njooma
Copy link
Member

@njooma njooma commented Sep 29, 2023

Close camera streams properly -- when there are no more listeners.

We reuse camera streams. Whenever someone asks for a stream, we return a cached copy if available. This stream might be new, cached and open, cached and previously closed (needs to be reopened), etc. We don't want the implementer to have to think about this. They should be able to call close whenever they are done, and we should figure out if it's time to actually close the stream (imagine they have multiple views with the same stream open, they should be able to close one without closing all).

This PR basically says, once there are no more subscribers to this stream, then close it for real. And finally, upon closing the robot, release all resources.

@njooma njooma requested a review from a team as a code owner September 29, 2023 23:43
Copy link
Member

@clintpurser clintpurser left a comment

Choose a reason for hiding this comment

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

Overall LGTM, one nit.

lib/src/media/stream/client.dart Outdated Show resolved Hide resolved
lib/src/media/stream/client.dart Outdated Show resolved Hide resolved
lib/src/media/stream/client.dart Outdated Show resolved Hide resolved
@njooma njooma merged commit 1532f78 into viamrobotics:main Oct 3, 2023
@njooma njooma deleted the improved-stream-closing branch October 3, 2023 15:00
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