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

[RSDK-9458] Remove 'Stream' from Camera interface #4691

Merged
merged 92 commits into from
Jan 28, 2025

Conversation

randhid
Copy link
Member

@randhid randhid commented Jan 8, 2025

The merge conflicts on Sean's PR will likely be terrible when he gets back in 2.5 weeks, so I'm using this branch to keep it recent.

I also think we're at a point where we can merge. I'm adding the original testing Sean did to the end of this comment.

Requests: @bhaney, could I have a config to test one of the vision services with to ensure DecodeImageFromCamera does not break them?

Original PR + comment

Tests

✅ Test webcam Darwin
✅ Test webcam switch inputs Darwin
✅ Test RTSP with passthrough and without
✅ Test webcam Linux
✅ Test webcam switch inputs Linux
✅ Test transforms (especially test crop + new features) caught a bug
#4635
✅ Test ffmpeg with mediamtx
✅ Test fake
✅ Test image_file
✅ Test OAK-D

Re-testing after long time (my, Sean, vacation)

  • Test webcam Darwin (builtin)
  • Test webcam switch inputs Darwin (builtin and iPhone)
  • Test RTSP with passthrough and without
  • Test webcam Linux (stream and GetImage on Logitech C90 Pro)
  • Test webcam switch inputs Linux (random driver, Logitech C90 Pro)
  • Test transform crop, rotate, and detections
  • Test ffmpeg with mediamtx
  • Test fake
  • Test image_file
  • Bijan's config for vision (person detection and other tf model)
  • Resizing stream survives reconfigures https://www.loom.com/share/a5a386b91c81428b9a7cdb4f6e71ad6c?sid=d424247a-4eed-43f8-892f-8d12bed94bc5

hexbabe and others added 30 commits October 24, 2024 14:59
…here we convert to go image; Change default mimetypes for classifier
…ng default mimetypes for vision since we are failing unit tests with 'do not know how to encode' errors
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 18, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 22, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 23, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 23, 2025
@hexbabe
Copy link
Member

hexbabe commented Jan 23, 2025

@bhaney I like Rand's idea of testing with a vision config. Could/did you send that over?

@randhid
Copy link
Member Author

randhid commented Jan 23, 2025

@bhaney I like Rand's idea of testing with a vision config. Could/did you send that over?

He sent me a service but @bhaney it's not working with vanilla viam-server, could you send another one over?

@viamrobotics viamrobotics deleted a comment from randhid Jan 23, 2025
Copy link
Member

@bhaney bhaney left a comment

Choose a reason for hiding this comment

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

Gave the AV team configs for the following vision services and camera:

  • standard detector using mlmodel
  • object tracking module (vision module written in Go)
  • transform camera for detections

They all passed -- looks good to me

Copy link
Member

@seanavery seanavery left a comment

Choose a reason for hiding this comment

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

Looking good to me!

Just want to make sure that we have tested that live streams are ok with the stream server changes (stream wrappers working and ctx handling is not causing issues).

Also heads up that I have an open PR #4738 with a stream server diff so would like to coordinate on rebase strategy here.

@hexbabe
Copy link
Member

hexbabe commented Jan 24, 2025

Gotcha-- some clarifying q's:

Just want to make sure that we have tested that live streams are ok with the stream server changes (stream wrappers working and ctx handling is not causing issues).

What specific behavior should I look out for during testing? What are "stream wrappers"? During my first and second round of testing documented at the top of this page, I just tested live stream and refresh every 1 sec worked as expected.

Also heads up that I have an open PR #4738 with a stream server diff so would like to coordinate on rebase strategy here.

Got it. TY for the pointer. What do you suggest the strategy be?

@seanavery
Copy link
Member

What are "stream wrappers"

I am just referring to how we are now stuffing stream with camerautils.VideoSourceFromCamera in the server.

During my first and second round of testing documented at the top of this page, I just tested live stream and refresh every 1 sec worked as expected.

Sounds good! May be good to test dynamic res a bit and verify that nothing hangs when shutting down viam-server.

Got it. TY for the pointer. What do you suggest the strategy be?

Not a big deal for me to rebase -- dont want to hold back this PR.

@hexbabe
Copy link
Member

hexbabe commented Jan 24, 2025

Sounds good! May be good to test dynamic res a bit and verify that nothing hangs when shutting down viam-server.

Gotcha, I'll prob do another final round of testing later, so will watch out for hangs/errors on shutdown and also add dynamic res toggling to the list of things I check for.

@hexbabe hexbabe requested review from seanavery and hexbabe January 27, 2025 18:22
@hexbabe hexbabe removed the request for review from nicksanford January 27, 2025 18:24
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025
@hexbabe hexbabe merged commit c04cefa into viamrobotics:main Jan 28, 2025
16 checks passed
@hexbabe hexbabe deleted the sean-pr branch January 28, 2025 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.