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

Open
wants to merge 90 commits into
base: main
Choose a base branch
from

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 and rotate
  • Test ffmpeg with mediamtx
  • Test fake
  • Test image_file
  • Bijan's config for vision (person detection and other tf model)

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
@@ -128,7 +111,8 @@ type Camera interface {
// err = myCamera.Close(context.Background())
//
// [camera component docs]: https://docs.viam.com/components/camera/
type VideoSource interface {
type Camera interface {
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of files changed here. Am I correct that these changes to VideoSource/Camera/StreamCamera are the only real changes and everything else is the refactor to appease the compiler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of - in the process of deprecating Stream, different methods are being used in vision, and the stream server now takes context to refresh the streams - I can break up this pr into independent pieces and remove some of sean yu's renames to make it smaller, I do think this was the diff that made the most sense for his manual testing.

Copy link
Member Author

@randhid randhid Jan 9, 2025

Choose a reason for hiding this comment

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

The camera driver changes + the new StreamCamera interface (ffmpeg, transformpipeline, webcam) are there to appease the compiler without refactoring those drivers intensely.

@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 16, 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 16, 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 16, 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 16, 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 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
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.