-
Notifications
You must be signed in to change notification settings - Fork 126
RSDK-12758 — Support webcam unplug replug recovery on darwin #5618
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
Conversation
|
looks reasonable to me |
|
windows build/tests were failing. It slipped past me that we do I thought about consolidating all the stubs into one file in |
|
@-ing you @abe-winter because this ended up being really build-y The context here is that I made an upstream change to add these "observer" functions you will see in the diff, but I didn't realize we do |
abe-winter
left a comment
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 + oliviamiller is a better reviewer than me for this, but I left a bunch of questions because this is a subtle system / thread thing
| // See web/cmd/server/observer_darwin.go for details on the threading requirements. | ||
| func startCameraObserver(logger logging.Logger) { | ||
| if err := mediadevicescamera.StartObserver(); err != nil { | ||
| logger.Errorw("failed to start darwin mediadevices camera observer", "error", err) |
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.
when this errors, is the system left in a bad state? (i.e. how do you know this is an ignorable error)
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 don't think the system will be left in a bad state i.e. it shouldn't hinder the rest of viam-server from behaving. But it will make it so that webcams cannot react to hot unplug/replug. Should I add that to the error?
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.
imo yes add to error -- better if errors say what to do / what to expect
| ) | ||
|
|
||
| // startCameraObserver starts the Darwin camera device observer for hot-plug support. | ||
| // This should be called after SetupObserver has been called from the main thread. |
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.
what happens if you get the order wrong? crash?
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.
StartObserver actually calls SetupObserver under the hood, but we call SetupObserver first in the entrypoint main.go to allow the objective-c NSRunLoop to set up on the main thread. It would be the "wrong" order if we only called StartObserver in the webcam code and didn't call SetupObserver in main.go
| // This should be called after SetupObserver has been called from the main thread. | ||
| // See web/cmd/server/observer_darwin.go for details on the threading requirements. | ||
| func startCameraObserver(logger logging.Logger) { | ||
| if err := mediadevicescamera.StartObserver(); err != nil { |
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.
does this need to be cleaned up ever?
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 should be cleaned up by DestroyObserver (see the deferred logic in main.go)
| // Start camera observer for hot-plug support (darwin only, no-op on other platforms). | ||
| // SetupObserver and DestroyObserver are called in RDK's entrypoint main.go. | ||
| // See web/cmd/server/observer_darwin.go for details on the threading requirements. | ||
| startCameraObserver(logger) |
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.
is it safe to call startCameraObserver multiple times?
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.
yes, I wrote it so that we can make it work like this in RDK (idempotent)
web/cmd/server/observer_darwin.go
Outdated
| // | ||
| // On Darwin/macOS, SetupObserver must be called from the main thread (not a spawned | ||
| // goroutine) because AVFoundation requires that camera device notification events and Key-Value | ||
| // Observation (KVO) updates occur on the same thread as the producer. The mediadevices |
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.
what does 'producer' mean here?
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 part of the OS that pushes the notification that a device was (dis)connected
|
your questions were helpful, I updated comments and error messages to be clearer @abe-winter |
RSDK-12758
Uses upstream pion/mediadevices#670.
Will manually test before merging. Please make a first pass review.
Manual Tests
Mac, two webcams
2026-01-06T19:25:51.254Z INFO entrypoint server/observer_darwin.go:26 DEBUG! darwin version!!!!!!Linux, two webcams
DEBUG! non-darwin no-op version!!!!!!!